Repository navigation
perf(libc): keep the guest allocation lease snapshot until the registry changes - #6917
Conversation
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
CnRJay
left a comment
There was a problem hiding this comment.
The cause is clear (the snapshot lived only as long as one lease, so the next lease rebuilt it by walking the whole registry; _Rb_tree_increment 9.9% and GuestAllocationsAcquire 9.7% of samples in MXGP 26) and the change is small. Questions:
- The registry now holds a
shared_ptrto the snapshot, so everyRangein it stays alive until the next registry change, including ranges whose owner memory the title has already released through a path that doesn't mutate the registry. What destroys aRange's backing when its last lease ends? The test changeuse_count() == 2("a failed import pins its owner beyond the registry and its kept lease snapshot") shows that the kept snapshot now holds owners that nothing held before. Please state which owners can be pinned this way and for how long, and that none of them are waited on by the pin wait (use_count() != 1is the check). - Anything that changes
readablefor a range withoutBegin/Endhas to reset the snapshot, or a lease gets the old readability.Mutation::Protectis used in the #6921 test; add a case here where a range turns read-only and the next lease must not return the old snapshot. - No before/after. Please post the summary lines of main and this PR from the same scene in PPSA30839 (no
APS5_PROFILE_*) and theperfshares after the change, instead of the shares before it.
|
Data point from a CPU-bound scene on Windows: RTX 3060 12 GB (NVIDIA 617.42), i5-12400F, Windows 10 19045, MinGW GCC Release. Minecraft Dungeons II (PPSA16064 01.100.100), title screen, no input, 200 s rows,
Within the run-to-run spread (about ±1.5 % here), no regression, 0 fatal. AI-assisted: yes (Claude Code) |
|
Independent CPU-side checks for the first two review questions, on exact head Ownership detail: Observed in five fresh-process runs:
Negative controls discriminate the reset paths: deleting only the two resets inside the pin-wait loop makes the reentrant callback case fail as still pinned; deleting only the This supplies registry-level protection/pinning evidence only. It does not establish concurrent GPU lifetime safety, resolve the previously reported unretained-mode title crash, or supply the requested unprofiled PPSA30839 FPS/after-change |
|
Thanks for the review, and thanks @simonbbby for the standalone probe. It answers the first two points independently and includes the negative controls. Which owners the kept snapshots hold, and for how long. A Every registry writer is a
They never hold a record the registry has already dropped. Memory the title releases without a registry change keeps its record in the registry anyway, so the kept snapshot adds no lifetime there. A lease someone still holds keeps its snapshot alive, as it did before. In The pin wait. Protect. 602b464 adds the case to
Before/after.
After the change, the largest share is AI-assisted: yes (Claude Code) |
|
A second title, for scale. Metal Gear Solid 4 (PPSA18194), Linux 6.8, RTX 4070 Ti (595.99.02), Release, pad input. Four interleaved rounds of 300 s, no
That's 8% higher on average, but at four runs it's within the run-to-run spread (Welch t ≈ 1.5). So I'd read it as no regression on a second title, not as a measured gain. The costs this PR targets dropped in every run (share of samples in a 12 s
They make up only 1–3% of MGS4's samples to begin with, against about 22% in MXGP, which fits the small difference in presents. AI-assisted: yes (Claude Code) |
…ry changes The registry cached its lease snapshot only while a lease held it, so the first lease taken after the others ended rebuilt the snapshot from the whole registry: every address-based build and import refresh paid a walk of every range and a reference to each. The registry now keeps the snapshot until the next registry change, which drops it as before; the pin check of a mutation drops it before counting the leases that hold a range. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A range turned unreadable must leave the next readable lease and show as unreadable in the next lease of all ranges, the kept snapshots must not count as a pin when the protection change waits for leases, and the replaced range must not outlive the change. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
602b464 to
8f0bd51
Compare
|
A third title: Forspoken (PPSA03984). Linux 6.8, RTX 4070 Ti (595.99.02), Release, warm shader and pipeline caches, scripted pad input. Where the time goes. An 8 s
With the PR, the largest new share in that window is shader registration ( Summary lines, no One window went the other way. With Whole runs, 120 s with
AI-assisted: yes (Claude Code) |
|
Tested on Onimusha's menu: no measurable change in this scene. Setup: Onimusha: Way of the Sword (PPSA27836), boot to the language menu with no input,
This scene may simply not exercise the path this PR speeds up. Summary lines and traces available on request. AI-assisted: yes. |
|
Thanks @simonbronk, and sorry for the slow reply. A clear "no measurable change" in a menu scene is useful, and it's in the description now. This PR saves the work of rebuilding the lease snapshot when the allocation registry hasn't changed, so it shows mainly in scenes with many guest-memory checks per frame. A menu may not do many. AI-assisted: yes (Claude Code) |
What
GuestAllocationsAcquire(andGuestAllocationsAcquireAll) shares one snapshot of the registry between the leases taken while it is unchanged. The registry kept that snapshot only through aweak_ptr, though, so it lived only as long as some lease held it. Most leases are short: the driver takes one per address-based build, import refresh and range check. So the first lease after the previous one ended rebuilt the snapshot from scratch, walking the whole registry map and taking a reference to every readable range.In MXGP 26 (PPSA30839) on Linux that rebuild is a large part of the queue workers' time. The run uses the PRs the title needs to get this far (#6814, #6094, #6160) plus #6308, #6184 and #6455, and both workers are at about 99% busy.
perf recordon the process putsstd::_Rb_tree_incrementat 9.9% of all samples,GuestAllocationsAcquire_nid_postfixat 9.7% and the snapshot vector'sshared_ptrcopies at 3.1%.The registry now keeps the snapshot (
shared_ptr) until the next registry change, where it is dropped exactly as before:GuestAllocationsBegin,GuestAllocationsEndand the pin wait reset it, under the registry mutex, as they already reset the weak pointer.use_count() != 1), so a kept snapshot never counts as a lease.The test that required "a snapshot outlived its leases" to be false now states the new contract instead. After the leases end, the snapshot is still the registry's, and the next lease reuses it without a rebuild. Any registry change, even an empty mutation, drops it.
Tested
Ubuntu 24.04, GCC 13.3, Release, RTX 4070 Ti (NVIDIA 595.99.02).
agc_driver_graphics(leaseSnapshotTestsandRunGuestLeaseWaitTests): withmain'sGuestAllocations.cppit fails (a lease taken after the others ended, without a registry change, rebuilt the snapshot), and with the change it passes 3 of 3. Since 602b464,leaseSnapshotTestsalso makes a range unreadable throughMutation::Protectwhile both snapshots are kept: the call must not wait on them, the replaced range must expire, and the next readable lease must leave the range out while the next lease of all ranges shows it unreadable. Passes 3 of 3.BdaResources' failed-import check now expects the kept snapshot's reference along with the registry's (use_count() == 2instead of 1). Its other checks are unchanged, including that a removed owner expires. All 391guest_*and libc tests pass, and the 501 tests matching^agc_driver_|^agc_vertex_barrier|recompiler|shader_cache|request_serializer|spirv|^agc_shader|bda|journalpass exceptagc_driver_store_over_untracked, which fails onmaintoo.APS5_TRACE_SECONDS=100:10(and the trace directory and pad input file) was set: noAPS5_PROFILE_*switch.maindoes not reach this scene, so the base is the stack above. The commits are local test merges:7b386bafd265is the stack (with perf(agc): time compute pipeline creation #6896's one-line compute pipeline trace timer), and5f469824dc57adds this PR'sGuestAllocations.cppchange (the same library diff as this branch).7b386bafd2655f469824dc577b386bafd2655f469824dc57tools/trace_digest.py:Summary lines (
aps5-summary.jsonl, base 1, PR 1, base 2, PR 2):{"commit":"7b386bafd265","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":369,"ms":10200.783},"presents":29,"frame_ms":{"p50":338.431,"p90":415.518,"p99":459.807,"max":459.807},"gpu_busy_ms_per_present":118.884,"worker_busy_pct":{"queue worker 0x0":99.611,"queue worker 0x20":99.536},"top_phases_ms_per_frame":{"Driver.Packet":700.083,"Driver.Packet.dispatch":298.609,"Driver.Dispatch":297.638,"Driver.Packet.memory_wait":284.109,"Vulkan.Dispatch":149.898,"Resources.Complete":144.679,"Vulkan.Dispatch.shader_resources":141.290,"Driver.Dispatch.capture_resources":60.594},"trace_dropped":false,"trace":"aps5-trace-7b386bafd265-3.json.gz"} {"commit":"5f469824dc57","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":477,"ms":10188.146},"presents":44,"frame_ms":{"p50":230.477,"p90":239.478,"p99":247.269,"max":247.269},"gpu_busy_ms_per_present":115.561,"worker_busy_pct":{"queue worker 0x0":99.417,"queue worker 0x20":99.300},"top_phases_ms_per_frame":{"Driver.Packet":459.173,"Driver.Packet.dispatch":179.724,"Driver.Dispatch":178.887,"Driver.Packet.memory_wait":169.497,"Driver.Dispatch.capture_resources":70.947,"Resources.Prepare":50.093,"Driver.Packet.draw":48.342,"Driver.Draw":48.119},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.825,"events_lost":77892,"totals_full":false}},"trace":"aps5-trace-5f469824dc57-3.json.gz"} {"commit":"7b386bafd265","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":368,"ms":10140.142},"presents":30,"frame_ms":{"p50":338.075,"p90":343.082,"p99":346.269,"max":346.269},"gpu_busy_ms_per_present":118.599,"worker_busy_pct":{"queue worker 0x0":99.617,"queue worker 0x20":99.539},"top_phases_ms_per_frame":{"Driver.Packet":672.674,"Driver.Packet.dispatch":286.752,"Driver.Dispatch":285.828,"Driver.Packet.memory_wait":271.222,"Vulkan.Dispatch":141.910,"Resources.Complete":137.169,"Vulkan.Dispatch.shader_resources":133.907,"Driver.Dispatch.capture_resources":61.058},"trace_dropped":false,"trace":"aps5-trace-7b386bafd265-3.json.gz"} {"commit":"5f469824dc57","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":".","APS5_TRACE_SECONDS":"100:10"},"gpu":"NVIDIA GeForce RTX 4070 Ti","driver":"NVIDIA 595.99.02","os":"Linux 6.8.0-147-generic","resolution":"3840x2160","window":{"spec":"seconds 100:10","first_present":441,"ms":10005.818},"presents":43,"frame_ms":{"p50":231.870,"p90":241.967,"p99":244.206,"max":244.206},"gpu_busy_ms_per_present":119.206,"worker_busy_pct":{"queue worker 0x0":99.442,"queue worker 0x20":99.515},"top_phases_ms_per_frame":{"Driver.Packet":462.388,"Driver.Packet.memory_wait":177.861,"Driver.Packet.dispatch":177.430,"Driver.Dispatch":176.601,"Driver.Dispatch.capture_resources":69.747,"Resources.Prepare":49.194,"Driver.Packet.draw":46.248,"Driver.Draw":46.035},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":7.890,"events_lost":69526,"totals_full":false}},"trace":"aps5-trace-5f469824dc57-3.json.gz"}Traces: https://gist.github.com/milescoviello/336bdc50227a65c6ea50f86e84ffbe1d, files
stack-7b386bafd265-run{1,2}.json.gz(base) andlease-5f469824dc57-run{1,2}.json.gz(this PR). They open in ui.perfetto.dev.APS5_PROFILE_DRAW=1(which costs time, so they are not the speed claim): 90 s, interleaved, four runs each. Presents per 10 s:In separate 130 s runs with
APS5_TRACE_SECONDS=100:10(also withAPS5_PROFILE_DRAW=1), the frame p50 is 342 and 345 ms without the change (2 runs) and 240 ms with it (1 run; the other is the crash below). A perf profile of the same phase with the change no longer has_Rb_tree_increment,GuestAllocationsAcquire_nid_postfixor the vector copies among its top entries; the largest is nowPageSpan::runEnd(13.8%).@simonbronk ran it on Windows with Onimusha: Way of the Sword (PPSA27836) at its language menu (RTX 5070, 617.42, 3840x2160, 3 runs against 6 base runs). Frame p50 stays within the base's run-to-run range (84.4, 82.5 and 84.4 ms against 77.9–84.5 ms), so that scene shows no measurable change.
Not checked: macOS, AMD/Intel.
#6565 changes the same two lines in
GuestAllocationsAcquire/GuestAllocationsAcquireAll, to leave retiring ranges out of new leases and skip storing the snapshot while a range retires. #6499 adds aRegistryfield right after the two snapshot members that this PR changes, sotools/pr_overlap.pyreports an adjacent-line conflict there. #6951 changes the cache-hit and the finalreturnlines ofGuestAllocationsAcquireto pass the generation into the lease. The changes compose; whichever of them merges later needs a small rebase.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)🤖 Generated with Claude Code