WIP: perf(vk): walk the captured ranges once a scan, not a binary search a page #216
No reviewers
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!216
Loading…
Reference in a new issue
No description provided.
Delete branch "TrashPanda/Longshot:perf/vk-dirty-page-range-walk"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Why
Each submit asks, for every dirty page it collects, whether that page is in the captured ranges (and
take_captureasks 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
RangeWalkingpu/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.memory_collect_writtenandtake_capture's texture-shadow loop use it;in_tex_mergedgoes.tests/test_vk_range_walk.pychecks 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 @
7c06b9afHardware: 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 fixtureHow to reproduce:
python -I -B tests/test_vk_range_walk.pyBaseline: main @ 2331f46c's binary search, as cut into the fixture
2331f46c7c06b9afIn-game frame times are not measured yet: the saving is a few µs to tens of µs a submit, which needs
RX_VK_DRAW_STATSor a frame-time capture in a busy scene to show.Testing
Tested:
python -I -B tests/run_all.pyon 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, andtest_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:
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
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.The walk itself is right.
RangeWalk::inkeepsnextequal toupper_bound(last): an ascending address moves it forward with the samefirst <= atest, 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 = 0withnext = 0still moves past it), the same address asked twice (take_capture's break and re-ask ofc->copied[i]) and touching ranges (the merged lists never have any) all behave. Nothing overflows: auint32_tpage shifted by 12 fits.The order assumption holds at every call site anyway:
memory_collect_written:collect_inwalkshos_gpu_guest_ranges, which is qsorted and merged (ranges under 16 MB apart become one), page by page upward.take_capture:c->pagescomes fromcollect_inover capture_make's own sorted, mergedmirrored, andc->copiedis 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_mergedis only replaced inpublish_tex_spansundershadow_mu, which take_capture holds across all three loops, including the shared walk between the second and third.On main:
gpu/vk/vk_memory.cpphasn't changed on main since the fork point (2331f46c), so this applies cleanly. #250 doesn't touch these lines. #248 adds lines right afterif (!in_tex_merged(...)) continue;in take_capture's second loop, so whichever lands second gets a small textual conflict. The fix there is to usein_tex.in(...). Its addedcontinuekeeps the order, and the test's count of threein_tex.in(stays true.Must change
tests/test_vk_range_walk.pyhas--cxxdefaulting tog++but noRUN_ALL_ARGS = ["--cxx", "{cxx}"], the declaration 117 of the other checks with--cxxcarry. Without it, run_all runs the check with no arguments. On a PC without a plaing++(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.main" is ticked, but the PR forks from2331f46c. 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
gpu/vk/vk_memory.cppabout line 1818,std::upper_bound(texr...)for every collected page) does the same binary search over the same ascendingpages, and it runs on the game's thread rather than the replay's. ARangeWalk in_texr{texr}there would be the bigger win of the three.vk: capture: a submit ...and the replay's per-submit time withRX_VK_DRAW_STATS, on main and this PR alternately in the same scene.mirroredlist. Worth naming both, since that ordering is what keeps the walk cheap.upper_bound(r.begin() + next, r.end(), ...).hi == next lo), are never generated, becauseranges()always leaves a gap of at least a page. They're correct by the argument above, but cheap to cover.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 walksThis order claim holds for memory_collect_written, whose pages come from collect_in over hos_gpu_guest_ranges. take_capture's
c->pagescome from collect_in over capture_make's ownmirroredlist, 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());Correct for any order:
nextstaysupper_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 toupper_bound(r.begin() + next, r.end(), ...).@ -1974,9 +1985,10 @@ bool take_capture(std::unique_ptr<Capture> c, uint64_t value, uint64_t completedstatic 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)Fine:
tex_mergedonly changes in publish_tex_spans under shadow_mu, which is held here through the third loop too, so sharing the walk across thebreakis 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.@ -0,0 +13,4 @@import subprocessimport tempfileROOT = Path(__file__).resolve().parents[1]Missing
RUN_ALL_ARGS = ["--cxx", "{cxx}"](117 other checks with--cxxdeclare it). Without it, run_all runs this check with itsg++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;atalways 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 withat = 0and 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);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.
7c06b9afd8b748c0f3feI rebased this onto main @
fbd42024(no conflicts) and addressed my review in one commit of mine on top of yours. It's pushed asb748c0f3, 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 totests/test_vk_range_walk.py, where the other checks declare it.capture_make's staging loop now usesRangeWalk in_texr{texr}instead of anupper_boundper page. It gives the same answers as before: the same sorted, mergedtexr; pages in address order fromcollect_inovermirrored; and the late-pagecontinueonly skips asks. That loop runs on the game's thread. The check now cutscapture_makeand requires the walk there.RangeWalkcomment now names the real sources of the pages:hos_gpu_guest_rangesformemory_collect_written, andcapture_make'smirroredlist for a Capture's pages.No tests were run here.
Still yours:
capture_make's staging is walked too;b748c0f3.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_mergedis only replaced inpublish_tex_spans, under the same non-recursiveshadow_muthattake_captureholds through all three loops. A publish from inside them would already deadlock on main.texriscapture_make'sthread_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, sor[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:capture_maketake_capturememory_collect_writtenSo 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_ALLwithscripts/cpu_report.py, as above. A Steam Deck would show it larger. For scale, in the same runcollect_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.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.