WIP: perf(vk): walk the captured ranges once a scan, not a binary search a page #216

Draft
TrashPanda wants to merge 2 commits from TrashPanda/Longshot:perf/vk-dirty-page-range-walk into main
Contributor

Why

Each submit asks, for every dirty page it collects, whether that page is in the captured ranges (and take_capture asks the same of the texture storage), with a binary search per page. The pages arrive in address order, so a single forward walk over the ranges gives the same answers for a fraction of the cost.

What changed

  • vk: RangeWalk in gpu/vk/vk_memory.cpp, a cursor over sorted, disjoint ranges that only moves forward; an address below the last one asked searches again, so any order gives the binary search's answer.
  • vk: memory_collect_written and take_capture's texture-shadow loop use it; in_tex_merged goes.
  • tests: tests/test_vk_range_walk.py checks RangeWalk against the binary search for ascending, random, descending and repeated addresses over zero, one and many ranges, checks that both callers use it, and prints both methods' time per page.

Flags and settings

None

Measurements

main: 2331f46c (2026-11-13)
this PR: perf/vk-dirty-page-range-walk @ 7c06b9af
Hardware: AMD Ryzen 7 9800X3D 8-Core Processor, Linux (CachyOS)
Scene: none, the check's fixture (not the game)
Method: tests/test_vk_range_walk.py's timing, 3 runs, the median; "main" is the binary search the PR replaces, run in the same fixture
How to reproduce: python -I -B tests/test_vk_range_walk.py
Baseline: main @ 2331f46c's binary search, as cut into the fixture

main @ 2331f46c this PR @ 7c06b9af
1,169 pages over 16 ranges, ns a page 2.8 0.9
16,777 pages over 256 ranges, ns a page 4.6 0.9
a scan of 1169 pages over 16 ranges: 2.8 ns a page searched, 0.9 ns walked
a scan of 16777 pages over 256 ranges: 4.6 ns a page searched, 0.9 ns walked
PASS: RangeWalk answers as the binary search, 18045370 addresses in every order

In-game frame times are not measured yet: the saving is a few µs to tens of µs a submit, which needs RX_VK_DRAW_STATS or a frame-time capture in a busy scene to show.

Testing

Tested: python -I -B tests/run_all.py on Linux (CachyOS, Python 3.14, Vulkan headers found): 490 passed, 3 failed, 30 skipped. main @ 2331f46c's run on the same PC gave 489 passed with the same 3 failing (test_display_pointer, test_lift_cache, and test_texture_staging_publication, fixed by #215); the extra pass is the new check.

Not tested: the game. The change is on the replay's per-submit path, so it affects every GPU.

Needs other testers:

  • Any GPU: play a busy scene (Hyrule Field, Kakariko) with RX_VK_DRAW_STATS, expecting the same frames and a lower replay time per submit than main (request to open)

Screenshots

Nothing on screen

Risk and rollback

A wrong answer would skip or keep a page that should not be, which would show as stale or missing texture contents. The check compares its answers with the binary search over 18 million addresses in every order. Reverting the commit is enough.

Reviewer notes

From King Kuranes' CP25. Ported from the Hookshot branch perf/vk-dirty-page-range-walk, which never opened as a PR there.

Issues

See #

Checklist

For everyone

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main.
  • python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
  • Tested in the game, or what wasn't is named under Testing.
  • Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (Fixture only; an in-game run is still to do.)
  • No game files, dumps, build output or generated files.
  • third_party/ untouched (a new folder with a PROVENANCE.md instead).
  • The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

  • AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
  • A game-free test added for the fix, failing on main and passing here.
## Why Each submit asks, for every dirty page it collects, whether that page is in the captured ranges (and `take_capture` asks the same of the texture storage), with a binary search per page. The pages arrive in address order, so a single forward walk over the ranges gives the same answers for a fraction of the cost. ## What changed - vk: `RangeWalk` in `gpu/vk/vk_memory.cpp`, a cursor over sorted, disjoint ranges that only moves forward; an address below the last one asked searches again, so any order gives the binary search's answer. - vk: `memory_collect_written` and `take_capture`'s texture-shadow loop use it; `in_tex_merged` goes. - tests: `tests/test_vk_range_walk.py` checks RangeWalk against the binary search for ascending, random, descending and repeated addresses over zero, one and many ranges, checks that both callers use it, and prints both methods' time per page. ## Flags and settings None ## Measurements main: 2331f46c (2026-11-13) this PR: perf/vk-dirty-page-range-walk @ 7c06b9af Hardware: AMD Ryzen 7 9800X3D 8-Core Processor, Linux (CachyOS) Scene: none, the check's fixture (not the game) Method: `tests/test_vk_range_walk.py`'s timing, 3 runs, the median; "main" is the binary search the PR replaces, run in the same fixture How to reproduce: `python -I -B tests/test_vk_range_walk.py` Baseline: main @ 2331f46c's binary search, as cut into the fixture | | main @ 2331f46c | this PR @ 7c06b9af | |---|---|---| | 1,169 pages over 16 ranges, ns a page | 2.8 | 0.9 | | 16,777 pages over 256 ranges, ns a page | 4.6 | 0.9 | ``` a scan of 1169 pages over 16 ranges: 2.8 ns a page searched, 0.9 ns walked a scan of 16777 pages over 256 ranges: 4.6 ns a page searched, 0.9 ns walked PASS: RangeWalk answers as the binary search, 18045370 addresses in every order ``` In-game frame times are not measured yet: the saving is a few µs to tens of µs a submit, which needs `RX_VK_DRAW_STATS` or a frame-time capture in a busy scene to show. ## Testing **Tested:** `python -I -B tests/run_all.py` on Linux (CachyOS, Python 3.14, Vulkan headers found): 490 passed, 3 failed, 30 skipped. `main` @ `2331f46c`'s run on the same PC gave 489 passed with the same 3 failing (`test_display_pointer`, `test_lift_cache`, and `test_texture_staging_publication`, fixed by #215); the extra pass is the new check. **Not tested:** the game. The change is on the replay's per-submit path, so it affects every GPU. **Needs other testers:** - Any GPU: play a busy scene (Hyrule Field, Kakariko) with `RX_VK_DRAW_STATS`, expecting the same frames and a lower replay time per submit than main (request to open) ## Screenshots Nothing on screen ## Risk and rollback A wrong answer would skip or keep a page that should not be, which would show as stale or missing texture contents. The check compares its answers with the binary search over 18 million addresses in every order. Reverting the commit is enough. ## Reviewer notes From King Kuranes' CP25. Ported from the Hookshot branch `perf/vk-dirty-page-range-walk`, which never opened as a PR there. ## Issues See # ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (Fixture only; an in-game run is still to do.) - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here.
perf(vk): walk the captured ranges once a scan, not a binary search a page
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 6s
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
7c06b9afd8
memory_collect_written asks for every page a submit collects whether it is
in the captured ranges, and take_capture's texture shadow asks the same of
the texture storage, each with a binary search. The collections give their
pages in address order (collect_in walks hos_gpu_guest_ranges' sorted
ranges), so RangeWalk keeps a cursor that only moves forward: one pass over
the ranges for the scan. An address below the last one searches again, so
its answers are the search's in any order.

From King Kuranes' CP25.

Assisted-by: Claude Opus 5.5
HumanBean added this to the v3.0.0 milestone 2026-11-13 17:47:40 +00:00
HumanBean left a comment

The walk itself is right. RangeWalk::in keeps next equal to upper_bound(last): an ascending address moves it forward with the same first <= a test, and an address below the last one searches again. So it gives the binary search's answer for any order of addresses, not only ascending ones. Order only changes what it costs. Empty lists, a range starting at 0 (last = 0 with next = 0 still moves past it), the same address asked twice (take_capture's break and re-ask of c->copied[i]) and touching ranges (the merged lists never have any) all behave. Nothing overflows: a uint32_t page shifted by 12 fits.

The order assumption holds at every call site anyway:

  • memory_collect_written: collect_in walks hos_gpu_guest_ranges, which is qsorted and merged (ranges under 16 MB apart become one), page by page upward.
  • take_capture: c->pages comes from collect_in over capture_make's own sorted, merged mirrored, and c->copied is a subsequence of it.

Locking is as before. capture_ranges_merged() is the calling thread's thread_local list, and nothing in the loop calls it again. tex_merged is only replaced in publish_tex_spans under shadow_mu, which take_capture holds across all three loops, including the shared walk between the second and third.

On main: gpu/vk/vk_memory.cpp hasn't changed on main since the fork point (2331f46c), so this applies cleanly. #250 doesn't touch these lines. #248 adds lines right after if (!in_tex_merged(...)) continue; in take_capture's second loop, so whichever lands second gets a small textual conflict. The fix there is to use in_tex.in(...). Its added continue keeps the order, and the test's count of three in_tex.in( stays true.

Must change

  • The new check doesn't declare its compiler for run_all. tests/test_vk_range_walk.py has --cxx defaulting to g++ but no RUN_ALL_ARGS = ["--cxx", "{cxx}"], the declaration 117 of the other checks with --cxx carry. Without it, run_all runs the check with no arguments. On a PC without a plain g++ (a clang-only Linux, llvm-mingw on Windows), the check then fails with FileNotFoundError when it should be run with the compiler run_all found, or skipped.
  • "Rebased on current main" is ticked, but the PR forks from 2331f46c. main has 14 commits since then, including #198. None of them touch vk_memory.cpp, so the rebase is trivial, but the box isn't true as it stands. Rebase, or untick it.

Should change

  • One loop of the same kind is left. capture_make's staging loop (gpu/vk/vk_memory.cpp about line 1818, std::upper_bound(texr...) for every collected page) does the same binary search over the same ascending pages, and it runs on the game's thread rather than the replay's. A RangeWalk in_texr{texr} there would be the bigger win of the three.
  • The speed-up is only a fixture number. The description says so, and Measured is honestly unticked. The fixture also isn't interleaved: the search always runs first, then the walk, in one process. Alternate them (or swap the order every round) so cache warm-up doesn't favour one side. For the game, the saving should show in vk: capture: a submit ... and the replay's per-submit time with RX_VK_DRAW_STATS, on main and this PR alternately in the same scene.
  • The struct comment cites the wrong source for take_capture's pages. It says the pages come from "collect_in walks hos_gpu_guest_ranges' sorted ranges". That's true for memory_collect_written. take_capture's pages come from collect_in over capture_make's mirrored list. Worth naming both, since that ordering is what keeps the walk cheap.
  • A zig-zag order would be slower than the old search. A descending address costs one search, but each ascending step after it walks forward one range at a time, so input alternating between far-apart addresses costs O(ranges) a page where the old search cost O(log ranges). No caller does this today, as shown above. If you want it bounded anyway, a step forward of more than a few ranges could switch to upper_bound(r.begin() + next, r.end(), ...).
  • Checks the fixture could add. A range starting at address 0, and adjacent ranges (hi == next lo), are never generated, because ranges() always leaves a gap of at least a page. They're correct by the argument above, but cheap to cover.
  • Description. "Issues: See #" is an empty placeholder: fill it in or remove it. "Ported from the Hookshot branch perf/vk-dirty-page-range-walk" would be better as a full URL to the branch.

I didn't run the checks (this review is read-only) and didn't verify the 490 passed / 3 failed claim. Nothing was run in the game.

The walk itself is right. `RangeWalk::in` keeps `next` equal to `upper_bound(last)`: an ascending address moves it forward with the same `first <= a` test, and an address below the last one searches again. So it gives the binary search's answer for any order of addresses, not only ascending ones. Order only changes what it costs. Empty lists, a range starting at 0 (`last = 0` with `next = 0` still moves past it), the same address asked twice (take_capture's break and re-ask of `c->copied[i]`) and touching ranges (the merged lists never have any) all behave. Nothing overflows: a `uint32_t` page shifted by 12 fits. The order assumption holds at every call site anyway: - `memory_collect_written`: `collect_in` walks `hos_gpu_guest_ranges`, which is qsorted and merged (ranges under 16 MB apart become one), page by page upward. - `take_capture`: `c->pages` comes from `collect_in` over capture_make's own sorted, merged `mirrored`, and `c->copied` is a subsequence of it. Locking is as before. `capture_ranges_merged()` is the calling thread's thread_local list, and nothing in the loop calls it again. `tex_merged` is only replaced in `publish_tex_spans` under `shadow_mu`, which take_capture holds across all three loops, including the shared walk between the second and third. On main: `gpu/vk/vk_memory.cpp` hasn't changed on main since the fork point (2331f46c), so this applies cleanly. https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/250 doesn't touch these lines. https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/248 adds lines right after `if (!in_tex_merged(...)) continue;` in take_capture's second loop, so whichever lands second gets a small textual conflict. The fix there is to use `in_tex.in(...)`. Its added `continue` keeps the order, and the test's count of three `in_tex.in(` stays true. ### Must change - **The new check doesn't declare its compiler for run_all.** `tests/test_vk_range_walk.py` has `--cxx` defaulting to `g++` but no `RUN_ALL_ARGS = ["--cxx", "{cxx}"]`, the declaration 117 of the other checks with `--cxx` carry. Without it, run_all runs the check with no arguments. On a PC without a plain `g++` (a clang-only Linux, llvm-mingw on Windows), the check then fails with FileNotFoundError when it should be run with the compiler run_all found, or skipped. - **"Rebased on current `main`" is ticked, but the PR forks from 2331f46c.** main has 14 commits since then, including https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/198. None of them touch vk_memory.cpp, so the rebase is trivial, but the box isn't true as it stands. Rebase, or untick it. ### Should change - **One loop of the same kind is left.** capture_make's staging loop (`gpu/vk/vk_memory.cpp` about line 1818, `std::upper_bound(texr...)` for every collected page) does the same binary search over the same ascending `pages`, and it runs on the game's thread rather than the replay's. A `RangeWalk in_texr{texr}` there would be the bigger win of the three. - **The speed-up is only a fixture number.** The description says so, and Measured is honestly unticked. The fixture also isn't interleaved: the search always runs first, then the walk, in one process. Alternate them (or swap the order every round) so cache warm-up doesn't favour one side. For the game, the saving should show in `vk: capture: a submit ...` and the replay's per-submit time with `RX_VK_DRAW_STATS`, on main and this PR alternately in the same scene. - **The struct comment cites the wrong source for take_capture's pages.** It says the pages come from "collect_in walks hos_gpu_guest_ranges' sorted ranges". That's true for memory_collect_written. take_capture's pages come from collect_in over capture_make's `mirrored` list. Worth naming both, since that ordering is what keeps the walk cheap. - **A zig-zag order would be slower than the old search.** A descending address costs one search, but each ascending step after it walks forward one range at a time, so input alternating between far-apart addresses costs O(ranges) a page where the old search cost O(log ranges). No caller does this today, as shown above. If you want it bounded anyway, a step forward of more than a few ranges could switch to `upper_bound(r.begin() + next, r.end(), ...)`. - **Checks the fixture could add.** A range starting at address 0, and adjacent ranges (`hi == next lo`), are never generated, because `ranges()` always leaves a gap of at least a page. They're correct by the argument above, but cheap to cover. - **Description.** "Issues: See #" is an empty placeholder: fill it in or remove it. "Ported from the Hookshot branch `perf/vk-dirty-page-range-walk`" would be better as a full URL to the branch. I didn't run the checks (this review is read-only) and didn't verify the 490 passed / 3 failed claim. Nothing was run in the game.
@ -671,0 +670,4 @@
namespace {
/// (longshot) Whether addresses asked in ascending order fall in sorted, disjoint [lo, hi) ranges: a cursor that only
/// moves forward, one pass over the ranges for a whole scan, where a binary search for every page cost log2(ranges)
/// compares each (the collections give their pages in address order: host/rx_backend.c's collect_in walks
Owner

This order claim holds for memory_collect_written, whose pages come from collect_in over hos_gpu_guest_ranges. take_capture's c->pages come from collect_in over capture_make's own mirrored list, which is also sorted and merged. Worth naming both, since that ordering is what keeps the walk cheap. (Correctness doesn't depend on it, thanks to the search when an address goes backwards.)

This order claim holds for memory_collect_written, whose pages come from collect_in over hos_gpu_guest_ranges. take_capture's `c->pages` come from collect_in over capture_make's own `mirrored` list, which is also sorted and merged. Worth naming both, since that ordering is what keeps the walk cheap. (Correctness doesn't depend on it, thanks to the search when an address goes backwards.)
@ -671,0 +677,4 @@
size_t next = 0; // (the first range that starts past the last address asked)
uint64_t last = 0;
bool in(uint64_t a) {
if (a < last) next = (size_t)(std::upper_bound(r.begin(), r.end(), std::make_pair(a, ~uint64_t(0))) - r.begin());
Owner

Correct for any order: next stays upper_bound(last). One cost edge case: after a step back, each forward step walks one range at a time, so zig-zag input costs O(ranges) a page instead of O(log ranges). No caller does that today. If you want it bounded anyway, a forward step of more than a few ranges could switch to upper_bound(r.begin() + next, r.end(), ...).

Correct for any order: `next` stays `upper_bound(last)`. One cost edge case: after a step back, each forward step walks one range at a time, so zig-zag input costs O(ranges) a page instead of O(log ranges). No caller does that today. If you want it bounded anyway, a forward step of more than a few ranges could switch to `upper_bound(r.begin() + next, r.end(), ...)`.
@ -1974,9 +1985,10 @@ bool take_capture(std::unique_ptr<Capture> c, uint64_t value, uint64_t completed
static thread_local std::vector<CopyPair> pairs; // (the copies, made together once the slots are known)
pairs.clear();
size_t i = 0;
RangeWalk in_tex{tex_merged}; // (c->copied in address order too: a subset of c->pages)
Owner

Fine: tex_merged only changes in publish_tex_spans under shadow_mu, which is held here through the third loop too, so sharing the walk across the break is safe. Note that #248 adds lines right after the test below, so a small textual conflict comes with whichever lands second. Also, capture_make's staging loop (about line 1818, std::upper_bound(texr...) for every page, on the game's thread) has the same pattern over the same ascending pages and could use a RangeWalk too.

Fine: `tex_merged` only changes in publish_tex_spans under shadow_mu, which is held here through the third loop too, so sharing the walk across the `break` is safe. Note that https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/248 adds lines right after the test below, so a small textual conflict comes with whichever lands second. Also, capture_make's staging loop (about line 1818, `std::upper_bound(texr...)` for every page, on the game's thread) has the same pattern over the same ascending pages and could use a RangeWalk too.
@ -0,0 +13,4 @@
import subprocess
import tempfile
ROOT = Path(__file__).resolve().parents[1]
Owner

Missing RUN_ALL_ARGS = ["--cxx", "{cxx}"] (117 other checks with --cxx declare it). Without it, run_all runs this check with its g++ default, so it fails instead of using the compiler run_all found, or being skipped, on a PC without a plain g++.

Missing `RUN_ALL_ARGS = ["--cxx", "{cxx}"]` (117 other checks with `--cxx` declare it). Without it, run_all runs this check with its `g++` default, so it fails instead of using the compiler run_all found, or being skipped, on a PC without a plain g++.
@ -0,0 +46,4 @@
static Ranges ranges(std::mt19937_64& rng, int n) { // (sorted, disjoint, page-aligned, gaps between)
Ranges r;
uint64_t at = (rng() % 64) << 12;
Owner

at always moves by at least a page before the first range and between ranges, so a range starting at 0 and touching ranges (hi == next lo) are never generated. Cheap to add, e.g. one round with at = 0 and no gap.

`at` always moves by at least a page before the first range and between ranges, so a range starting at 0 and touching ranges (`hi == next lo`) are never generated. Cheap to add, e.g. one round with `at = 0` and no gap.
@ -0,0 +102,4 @@
long a = 0, b = 0;
const auto t0 = std::chrono::steady_clock::now();
for (int k = 0; k < 50; k++)
for (uint64_t p : pages) a += searched(r, p);
Owner

The timing always runs the search first and then the walk, in one process, so cache warm-up can favour the walk. Alternate them (or swap the order every round) so the fixture's numbers are interleaved.

The timing always runs the search first and then the walk, in one process, so cache warm-up can favour the walk. Alternate them (or swap the order every round) so the fixture's numbers are interleaved.
HumanBean force-pushed perf/vk-dirty-page-range-walk from 7c06b9afd8
Some checks failed
PR description / Description follows the template (pull_request_target) Successful in 6s
CI / Conventional Commit title (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled
CI / Workflow lint (actionlint) (pull_request) Has been cancelled
Third-party parts / As their notes say (pull_request) Has been cancelled
Third-party parts / The same as upstream (pull_request) Has been cancelled
to b748c0f3fe
All checks were successful
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been skipped
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been skipped
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been skipped
Third-party parts / The same as upstream (pull_request) Has been skipped
Website / Website on Cloudflare (pull_request) Has been skipped
PR description / Description follows the template (pull_request_target) Successful in 8s
CI / Conventional Commit title (pull_request) Successful in 3s
CI / Workflow lint (actionlint) (pull_request) Successful in 7s
Third-party parts / As their notes say (pull_request) Successful in 10s
2026-11-13 18:04:42 +00:00
Compare
Owner

I rebased this onto main @ fbd42024 (no conflicts) and addressed my review in one commit of mine on top of yours. It's pushed as b748c0f3, and your commit is unchanged apart from the rebase.

Light re-review of that commit: the earlier points are resolved.

  • RUN_ALL_ARGS = ["--cxx", "{cxx}"] is added to tests/test_vk_range_walk.py, where the other checks declare it.
  • capture_make's staging loop now uses RangeWalk in_texr{texr} instead of an upper_bound per page. It gives the same answers as before: the same sorted, merged texr; pages in address order from collect_in over mirrored; and the late-page continue only skips asks. That loop runs on the game's thread. The check now cuts capture_make and requires the walk there.
  • The RangeWalk comment now names the real sources of the pages: hos_gpu_guest_ranges for memory_collect_written, and capture_make's mirrored list for a Capture's pages.
  • The fixture adds three range shapes: from address 0 with gaps, from 0 with every range touching the next, and from 0 mixed. It also asks address 0 and every range's ends. Every answer is still compared with the binary search.

No tests were run here.

Still yours:

  • The description:
    • fill in or remove the empty "See #";
    • make the Hookshot branch a full URL;
    • mention that capture_make's staging is walked too;
    • redo the run_all box at b748c0f3.
  • A speed-up in the game: it's only measured in the fixture so far. Please add an in-game A/B, ideally interleaved.
I rebased this onto main @ fbd42024 (no conflicts) and addressed my review in one commit of mine on top of yours. It's pushed as b748c0f3, and your commit is unchanged apart from the rebase. **Light re-review of that commit: the earlier points are resolved.** - **`RUN_ALL_ARGS = ["--cxx", "{cxx}"]`** is added to `tests/test_vk_range_walk.py`, where the other checks declare it. - **`capture_make`'s staging loop** now uses `RangeWalk in_texr{texr}` instead of an `upper_bound` per page. It gives the same answers as before: the same sorted, merged `texr`; pages in address order from `collect_in` over `mirrored`; and the late-page `continue` only skips asks. That loop runs on the game's thread. The check now cuts `capture_make` and requires the walk there. - **The `RangeWalk` comment** now names the real sources of the pages: `hos_gpu_guest_ranges` for `memory_collect_written`, and `capture_make`'s `mirrored` list for a Capture's pages. - **The fixture** adds three range shapes: from address 0 with gaps, from 0 with every range touching the next, and from 0 mixed. It also asks address 0 and every range's ends. Every answer is still compared with the binary search. No tests were run here. **Still yours:** - **The description:** - fill in or remove the empty "See #"; - make the Hookshot branch a full URL; - mention that `capture_make`'s staging is walked too; - redo the run_all box at b748c0f3. - **A speed-up in the game:** it's only measured in the fixture so far. Please add an in-game A/B, ideally interleaved.
First-time contributor

Code review only; not built or run.

Edited: the Kokiri profile quoted below drew a 5:4 picture, 1800×1440, not 2048×1152 as I first wrote: the PC's display mode had changed that hour. At the usual 16:9 picture the same route runs at about 74 fps. A rerun of the profile there is queued, and I'll update the numbers if they move.

Thanks, this is a tidy change. HumanBean's review covers why the walk is correct and the order at each call site. I have two additions.

1. The cursor's index can't go stale. The one thing a cursor adds over a fresh search is an index kept across the loop, so I checked that the vector it points into can't change underneath it:

  • tex_merged is only replaced in publish_tex_spans, under the same non-recursive shadow_mu that take_capture holds through all three loops. A publish from inside them would already deadlock on main.
  • texr is capture_make's thread_local, set before its loop.
  • capture_ranges_merged()'s list belongs to the calling thread, and is rebuilt only by that thread's next call.

Nothing inside the loops (shadow_erase, the two sweeps, copy_pairs) touches any of the three, so r[next - 1] stays in bounds.

2. What it can save in the game. These are self times from a sampled profile of Kokiri Forest on Longshot 1ef93e79 (i7-13700K, RTX 5090, Windows, 1800×1440 (5:4), uncapped, about 79 fps; all threads, named with the build's linker maps). The binary searches are inlined, so they are part of these numbers:

function sead::MainThread, ms a frame Presentation Thread, ms a frame
capture_make 0.022 0.006
take_capture 0.026 0.011
memory_collect_written 0.017 0.013

So on that PC the walk can save at most about 0.06 ms a frame on the game's thread and 0.03 ms on the Presentation Thread, and the searches are only part of those self times. That is under 0.5 % of a 12.6 ms frame, too small to show in fps.

The in-game A/B HumanBean asked for would show it better as these functions' samples than as frame times: HOS_SAMPLE_ALL with scripts/cpu_report.py, as above. A Steam Deck would show it larger. For scale, in the same run collect_in (the scan of the write marks that the pages come from) costs 0.32 ms a frame on each of the two threads, about five times these three functions' whole self time on the game's thread.

No further concerns from me.

Code review only; not built or run. *Edited:* the Kokiri profile quoted below drew a 5:4 picture, 1800×1440, not 2048×1152 as I first wrote: the PC's display mode had changed that hour. At the usual 16:9 picture the same route runs at about 74 fps. A rerun of the profile there is queued, and I'll update the numbers if they move. Thanks, this is a tidy change. HumanBean's review covers why the walk is correct and the order at each call site. I have two additions. **1. The cursor's index can't go stale.** The one thing a cursor adds over a fresh search is an index kept across the loop, so I checked that the vector it points into can't change underneath it: - `tex_merged` is only replaced in `publish_tex_spans`, under the same non-recursive `shadow_mu` that `take_capture` holds through all three loops. A publish from inside them would already deadlock on main. - `texr` is `capture_make`'s `thread_local`, set before its loop. - `capture_ranges_merged()`'s list belongs to the calling thread, and is rebuilt only by that thread's next call. Nothing inside the loops (`shadow_erase`, the two sweeps, `copy_pairs`) touches any of the three, so `r[next - 1]` stays in bounds. **2. What it can save in the game.** These are self times from a sampled profile of Kokiri Forest on Longshot 1ef93e79 (i7-13700K, RTX 5090, Windows, 1800×1440 (5:4), uncapped, about 79 fps; all threads, named with the build's linker maps). The binary searches are inlined, so they are part of these numbers: | function | sead::MainThread, ms a frame | Presentation Thread, ms a frame | |---|---|---| | `capture_make` | 0.022 | 0.006 | | `take_capture` | 0.026 | 0.011 | | `memory_collect_written` | 0.017 | 0.013 | So on that PC the walk can save at most about 0.06 ms a frame on the game's thread and 0.03 ms on the Presentation Thread, and the searches are only part of those self times. That is under 0.5 % of a 12.6 ms frame, too small to show in fps. The in-game A/B HumanBean asked for would show it better as these functions' samples than as frame times: `HOS_SAMPLE_ALL` with `scripts/cpu_report.py`, as above. A Steam Deck would show it larger. For scale, in the same run `collect_in` (the scan of the write marks that the pages come from) costs 0.32 ms a frame on each of the two threads, about five times these three functions' whole self time on the game's thread. No further concerns from me.
All checks were successful
CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been skipped
CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been skipped
CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been skipped
Third-party parts / The same as upstream (pull_request) Has been skipped
Website / Website on Cloudflare (pull_request) Has been skipped
PR description / Description follows the template (pull_request_target) Successful in 8s
CI / Conventional Commit title (pull_request) Successful in 3s
CI / Workflow lint (actionlint) (pull_request) Successful in 7s
Third-party parts / As their notes say (pull_request) Successful in 10s
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u 'perf/vk-dirty-page-range-walk:TrashPanda-perf/vk-dirty-page-range-walk'
git switch 'TrashPanda-perf/vk-dirty-page-range-walk'
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
MagicBeanDev/Longshot!216
No description provided.