Repository navigation
Conversation
|
This pull request is labeled |
MarcZX10
left a comment
There was a problem hiding this comment.
The new synthetic test and the hosted eligibility, conventions, intake, labels, and report checks are useful. I cannot approve this performance PR yet because the repository performance-report rule is not met: the matched-seed APS5_TRACE_SECONDS=290:120 summaries compare different scene stretches (the PR notes presents 516–664 versus 559–672), and the report lacks the required attached base/change traces plus tools/trace_digest.py output. The profile runs with APS5_PROFILE_DRAW/APS5_TRACE_BIGBUF are diagnostic runs, not compliant performance captures under the report form. Please provide a warm-cache capture pair on the same title state/settings/machine with the required summary lines, traces and digests, or remove the speed claim/perf title and retain the measured observations as non-comparative diagnostics. Separately, AGC changes require the same focused Vulkan tests on main and this commit with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, and the Khronos validation layer, confirming no new VUIDs and that the requested layer was found; this report does not include that comparison. Hosted GPU jobs were skipped, so I have not treated them as run. I did not run local tests.
|
The AGC rules require the same focused Vulkan test on |
|
@MarcZX10 thanks, both points are right. Coming next:
One thing a reviewer should know: Echoes does not reach this scene on plain If that is not acceptable for the |
|
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.
|
|
Present-matched capture, as asked. Setup
Summary lines (
The gain is on the CPU side, as expected from removing the per-use copies of the heap. One pair, so p90 and the tails are within run-to-run variance. Files (both The focused |
|
Thanks for adding the present-matched capture details. Two required pieces are still outstanding in the PR itself: (1) the |
|
Focused tests under the validation layer, on Setup
Result
Logs (paths redacted): https://github.com/JJenkx/AnyPS5/tree/evidence/pr9551/vvl |
|
The Baseline (without this change): {"commit":"20f928c642f7","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"560:100"},"gpu":"AMD Radeon RX 6650 XT (RADV NAVI23)","driver":"radv Mesa 26.2.4-arch1.1","os":"Linux 7.2.9-arch1-1","cpu":"AMD Ryzen 9 7950X 16-Core Processor","cpu_threads":32,"memory_mib":127901,"title_id":"PPSA25636","title":"Echoes of the End","content_version":"01.012.000","resolution":"3840x2160","window":{"spec":"frames 560:100","first_present":560,"ms":92408.448},"presents":100,"frame_ms":{"p50":748.353,"p90":1091.598,"p99":6985.317,"max":6985.317},"gpu_busy_ms_per_present":412.483,"worker_busy_pct":{"queue worker 0x0":99.794,"queue worker 0x20":96.700},"top_phases_ms_per_frame":{"Driver.Packet":1775.286,"Driver.Packet.dispatch":983.421,"Driver.Dispatch":913.871,"Vulkan.Dispatch":547.906,"Vulkan.Dispatch.shader_resources":539.003,"Resources.Complete":530.227,"Driver.Packet.memory_wait":471.162,"GpuMutex.wait.indirect":143.345},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":31.770,"events_lost":417448,"totals_full":false}},"trace":"aps5-trace-20f928c642f7-4193406.json.gz"}With this change: {"commit":"0560946d6da2","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"560:100"},"gpu":"AMD Radeon RX 6650 XT (RADV NAVI23)","driver":"radv Mesa 26.2.4-arch1.1","os":"Linux 7.2.9-arch1-1","cpu":"AMD Ryzen 9 7950X 16-Core Processor","cpu_threads":32,"memory_mib":127901,"title_id":"PPSA25636","title":"Echoes of the End","content_version":"01.012.000","resolution":"3840x2160","window":{"spec":"frames 560:100","first_present":560,"ms":78426.489},"presents":100,"frame_ms":{"p50":630.016,"p90":1299.811,"p99":2373.513,"max":2373.513},"gpu_busy_ms_per_present":400.723,"worker_busy_pct":{"queue worker 0x0":99.763,"queue worker 0x20":96.499},"top_phases_ms_per_frame":{"Driver.Packet":1521.026,"Driver.Packet.dispatch":823.749,"Driver.Dispatch":745.761,"Driver.Packet.memory_wait":418.504,"Vulkan.Dispatch":410.589,"Vulkan.Dispatch.shader_resources":402.112,"Resources.Complete":387.016,"GpuMutex.wait.indirect":111.581},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":25.439,"events_lost":428881,"totals_full":false}},"trace":"aps5-trace-0560946d6da2-163082.json.gz"} |
|
@MarcZX10 your review's points are all addressed now. Please take another look when you have time.
|
|
Windows cross-test completed for c83c5ea on integration 152c7a8 (base 163fd9b): Windows 11 / RTX 5070 Ti 617.42 / Ryzen 7800X3D / 32 GiB, WinLibs 15.2r7 Release, SPIRV Tools OFF. 85 focused cases passed, one expected Linux-only skip (36.81 s), with the actual Khronos 1.4.363 layer loaded for native Vulkan cases. Two existing 02275 validation errors in sampled_storage_base_level_usage match the baseline; no new validation errors. The graphics regression itself uses the mock device, so its passing result is not a native Vulkan validation claim. Production-only negative: restoring baseline 163 GuestBufferMemory.cpp while retaining the new fixture makes graphics fail with “a sparse heap whose committed span crosses an adjacent range was not mirrored.” Restoring the exact candidate passes; fixture executable hashes were unchanged. Restored focused import/mirror controls: 16 pass, one expected skip, 8.78 s, zero validation errors. 007 observation, not a controlled performance claim: six contained summary intervals in the observed warm moving offline menu gave 78 presents / 63.204949 s = 1.2341 FPS, GPU busy 191.76 ms/present. Previous 163 menu observation was 1.2788 FPS. This does not establish an improvement or regression; the diagnostic startup settings and scene timing are not a strict performance pair. Large CPU buffer copies persist, so I have not established that the dominant title copies are this adjacent-range sparse case. No gameplay was reached in this run; it ended at the bounded 30-minute deadline. #9652 remains excluded pending isolation of a separate startup failure. Original author commits retained, no duplicate implementation. Raw title code/requests/captures remain local. AI-assisted: Codex. |
|
Added While profiling the joined mirror's rebuilds, I found an interaction with the existing release rule:
The fix releases a heap mirror only when the imported range covers its reserved span. For a single-range mirror these are the same range, so its behaviour does not change. Echoes (PPSA25636), one 480 s run each, same settings (
The mock-device tests cannot exercise this path, because host imports fail there and nothing is released. The focused validation-layer tests on this new head are queued, and I will post them. |
|
I reviewed the new c65f593 change as well. The |
MarcZX10
left a comment
There was a problem hiding this comment.
The added partial-import guard addresses a real lifecycle issue, but its new behavior lacks a targeted test: the sparse joined heap case never imports only one member range after creating the mirror. Please add that regression. Also coordinate with open #7647, which edits the same mirror implementation and tests, and record whether either depends on the other. This head is 3 ahead/119 behind current upstream/main 4777cac (merge base a6dcea9). Diff-check, conventions and merge-tree against main pass, but the GPU evidence and performance pair in the PR cover earlier head c83c5ea on older base a6dcea9. The required AGC validation comparison and report artifacts must be refreshed on current main and this head, with the layer confirmed loaded and VUIDs compared. I did not run GPU tests locally.
|
Focused validation-layer tests on the new head Setup:
Result:
|
c65f593 to
318ad53
Compare
|
@MarcZX10 this covers your latest review, on head
|
|
Focused gate receipt for the current head, Build: one Ninja build directory for every arm, Environment, the same for every arm:
"Layer inserted" counts the loader's Skipped cases. Every arm prints the same two lines from
These are existing cases that need the Windows guest arena; they do not run on Linux on main either. Neither of this PR's new cases is skipped. Their skip messages ("sparse heap mirrors over adjacent ranges not tested" and "joined heap mirrors under a partial import not tested") appear in no log, and both of their assertions fire in the negative arms above. Logs, with local paths redacted, in the
|
98de596 to
caaed79
Compare
|
Rebased on
Exact-head focused gate: one build directory, the same command for every arm, submodules synced per arm. Command:
The performance evidence in the description is unchanged and keeps its limits: one warm integration pair, with dropped trace events, measured before this rebase. |
caaed79 to
a5331de
Compare
|
Rebased again, onto
Exact-head gate: same command and environment as the receipt above, with submodules synced per arm.
Only the two existing Windows guest-arena skips appear. Logs: There is no new test for the piece-import path itself. That path's existing #7292 tests pass with this change. |
adriadam10
left a comment
There was a problem hiding this comment.
Checked the joined-range lifetime, the partial-import guard, the piece-import byte sum and the two new bda_resources cases on the current head, merged onto 11d6efa06; the code compiles against the moved main (no stale imported call sites). Requesting changes on the process side, not the code:
- Performance label: please attach to this PR the two
aps5-summary.jsonllines, both traces, and thetools/trace_digest.pyoutput formainand for the change, from the same title, scene and machine. The fork branchevidence/pr9551is not an attachment, and the description's summary hasperformance_compareoutput but notrace_digestoutput. (AGENTS.md: a speed claim showstrace_digestsummaries of both arms.) - Your last review, @MarcZX10, is CHANGES_REQUESTED on 2026-10-22 11:27 and its points were answered on later heads: please re-review the current head or retire the request.
- The description already says the warm pair measured p50 +3.3 % (worse) and mean -3.0 %, and that FPS is not established; keep that wording unless the attached traces show otherwise.
|
@adriadam10 @MarcZX10 Added a “Direct links to the existing capture evidence” section with both summary files, both traces and both trace_digest outputs as individual GitHub release asset links, plus the two digest summary lines inline. I downloaded the linked captures and regenerated both digests with the current db6af9e tool; each matches its previously published digest byte for byte. These remain the already-reported integration runs (6784938b0095 vs1551d42a177d), not new captures. Both traces dropped events; the p50 +3.3% regression, mean -3.0% and FPS-not-established limits remain explicit. Implementation head a5331de is unchanged. Please re-review the current head and evidence links when convenient. |
… ranges A sparse heap the guest grows by mapping an adjacent range is no longer covered by one leased range, so bindSparseMirror refused it and every use copied the heap's committed pieces on the CPU. The mirror now takes a joined range over the run of continuing ranges (crossedRun), kept while its member ranges stay the same. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… ranges Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An address-based build releases the heap mirror at the base of each range it imports. A mirror over a joined run of ranges starts at its first range but spans the others too, so importing that range alone released it, and the next descriptor bind rebuilt it: in Echoes of the End, 140 releases and rebuilds of a 544 MiB mirror in 480 s. A heap mirror is now released only when the imported range covers its reserved span. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…e of its ranges Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
a5331de to
bc7d801
Compare
|
Rebased on Exact-head gate, Linux / RX 6650 XT / RADV (Mesa 26.2.4),
The validation layer was inserted in 4 tests per arm, with 0 VUIDs. Logs: Note for whichever of this PR and #8874 merges second: both add one line at the start of |
What
A sparse heap mirror needs one leased range that covers the heap's committed span:
bindSparseMirrorcallscontainingRange(lease, begin, neededEnd). When the guest grows a sparse heap by mapping a second range directly after the first, two adjacent leased ranges cover the span instead of one. The bind is then refused, and every use of the heap falls back tocopyRegion, which copies every committed piece on the CPU and diffs it at write-back.Echoes of the End (PPSA25636) does this with its PRT heap:
0x8fc0000000+0x21000000, then0x8fe1000000+0x1000000. Its committed span ends at0x8fe2000000. The first 8 binds succeed while one range covers the span. After that, each use copies about 550 MiB.With this change,
bindSparseMirrorfalls back to a joined range when no single range covers the span. The joined range covers the run of continuing ranges (crossedRun, the same rule the import path uses).joinedHeapRangekeeps it for as long as the run's member ranges are the same objects, so the mirror's identity check (sameRange) holds across builds. When a member changes, the joined range is replaced: the old mirror's weakrangeexpires and the sweep drops it, as it does for a single range that goes away.sweepMirrorsfirst releases the joined ranges whose members have expired, and forces a full sweep when it releases one, so a mirror over ranges the guest released is freed even if no bind follows.A joined mirror also needs a change in
releaseImportedMirrors. An address-based build (AcquireRegistered) releases the heap mirror at the base of each range it imports. A joined mirror starts at its first range, so importing that range alone released the whole mirror, and the next descriptor bind rebuilt it: in Echoes, 140 releases and rebuilds of a 544 MiB mirror in 480 s. A heap mirror is now released only when the imported range covers its reserved span, which for a single-range mirror is unchanged.Tested
a5331de72donmain12f7e1cd3f. The head was ported onto fix(agc): bind heap mirrors only where the guest holds bytes #7647's resident heap mirrors. After fix(agc): import registered ranges larger than the device's largest allocation in pieces #7292, a piece import records the summed length of its pieces, so a mirror is released only when the pieces cover it whole.ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation VK_LOADER_DEBUG=layer ctest -V -j1 -R '^(agc_driver_graphics|agc_sparse_.*|agc_heap_mirror_.*|agc_host_import.*)$'. Results:main16/16; this PR with the partial-import fix (5c7c0ea870) reverted 15/16, failing only the intended "an address build that imported one range of a joined heap mirror released the mirror"; this PR 16/16.vvl9551h-*.login https://github.com/JJenkx/AnyPS5/releases/tag/pr9551-evidence. Earlier heads' gates (98de5968c0,caaed795da) are in the comments.agc_driver_graphics_tests, new casejoinedHeapMirrorPartialImportTests: a heap bound across two committed ranges is mirrored over both, and an address build that imports the first range keeps the mirror (same buffer, no rebuild). With only the release fix reverted, it fails with "an address build that imported one range of a joined heap mirror released the mirror".agc_driver_graphics_tests, new casesparseJoinedHeapMirrorTests(mock device; the guest arena write watch on Windows,GuestWriteWatchon Linux). A sparse heap over two adjacent registered ranges, with one committed page in each, binds one mirror. A store to the second range is written back. A second bind over unchanged ranges reuses the mirror without a rebuild. Removing the ranges frees the mirror at the next sweep. On Linux (GuestWriteWatch), the wholeagc_driver_graphics_testsrun passes with this change. With only this change reverted, the new case fails with "a sparse heap whose committed span crosses an adjacent range was not mirrored".APS5_PROFILE_DRAW=1 APS5_TRACE_BIGBUF=1, 420 s each). Before:APS5_TRACE_SECONDS=290:120(420 s per arm):Driver.Presenttimestamps), with this change: present 560 at 290.7 s (330.8 s without), 600 at 317.1 s (355.4), 640 at 343.5 s (379.7), 660 at 364.1 s (403.4). That is 36-40 s earlier, the same title state about 10 % sooner.seconds 290:120 from present 516: 149 presents, frame p50 644.0 / p90 1389.8 / p99 2297.5 / max 2380.6 ms, GPU busy 386.7 ms per present; withseconds 290:120 from present 559: 114 presents, frame p50 672.1 / p90 1541.4 / p99 7793.8 / max 7978.6 ms, GPU busy 386.8 ms per present. The slowest frames with the change (presents 668-672) come after the last present the other arm reached.APS5_PROFILE_DRAW=1 APS5_TRACE_BIGBUF=1, 420 s per arm, 0 amdgpu kernel events in either:External Windows cross-test
Scramblrrrr reports testing this head
c83c5ea1c9in integration152c7a80ac70, based on163fd9b52c, on Windows 11 / RTX 5070 Ti 617.42 / Ryzen 7800X3D / 32 GiB / WinLibs GCC 15.2r7 Release, with SPIRV Tools off. The fetched integration commit has exactly those base/topic parents and contains this topic head.The external report records 85 focused passes and one expected Linux-only skip, with Khronos 1.4.363 loaded for native Vulkan cases. Two
02275errors insampled_storage_base_level_usagealso occurred on that tester's baseline; no new validation errors were reported. The graphics regression uses a mock device. Reverting onlyGuestBufferMemory.cppto that integration's baseline made it fail at the adjacent-range sparse-heap assertion; restoring the candidate passed with the fixture executable unchanged. The restored focused import/mirror controls passed 16 cases with one expected skip and zero validation errors.This is external correctness evidence, not the required controlled performance comparison or a native Vulkan claim for the mock graphics case. The tester reached a moving offline menu, not gameplay, and explicitly did not establish an FPS improvement or regression. The requested controlled same-scene performance comparison remains open. A later focused Linux gate receipt records main
4777caca34and candidate98de5968c0passing 15/15 on the same machine, with required Vulkan, shader cache disabled, four actual validation-layer instance insertions and zero VUIDs per arm. The test-only and release-fix-reverted controls each fail the intended assertion (14/15). Two existing Windows guest-arena cases are skipped on Linux in every arm; neither new joined-range case is skipped. These are results on the pinned base above, not a claim of testing every subsequent main revision. The receipt also corrects the oldervvl9551e-ctest-change.logprovenance: it is from318ad5365b;vvl9551f-ctest-change.logcovers98de5968c0.Summary lines
Measured at head
98de5968c0(the same joined-range and partial-import logic, before the #7647 port and the #7292 piece-import fix; not re-measured ona5331de72d), present-matched warm pair:APS5_TRACE_FRAMES=450:100, 540 s per arm, same seed. Both arms are the integration tree; the baseline has this PR's code reverted. Files: https://github.com/JJenkx/AnyPS5/releases/tag/pr9551-evidence (thecurrent-*assets).Baseline (without this PR):
{"commit":"6784938b0095","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"450:100"},"gpu":"AMD Radeon RX 6650 XT (RADV NAVI23)","driver":"radv Mesa 26.2.4-arch1.1","os":"Linux 7.2.9-arch1-1","cpu":"AMD Ryzen 9 7950X 16-Core Processor","cpu_threads":32,"memory_mib":127901,"title_id":"PPSA25636","title":"Echoes of the End","content_version":"01.012.000","resolution":"3840x2160","window":{"spec":"frames 450:100","first_present":450,"ms":114396.035},"presents":100,"frame_ms":{"p50":1003.787,"p90":1681.364,"p99":2596.762,"max":2596.762},"gpu_busy_ms_per_present":472.414,"worker_busy_pct":{"queue worker 0x0":99.817,"queue worker 0x20":97.747},"top_phases_ms_per_frame":{"Driver.Packet":2229.130,"Driver.Packet.dispatch":1204.279,"Driver.Dispatch":1107.533,"Vulkan.Dispatch":711.504,"Resources.Complete":705.573,"Vulkan.Dispatch.shader_resources":701.175,"Driver.Packet.memory_wait":636.887,"GpuMutex.wait.indirect":143.947},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":37.231,"events_lost":478449,"totals_full":false}},"trace":"aps5-trace-6784938b0095-4118577.json.gz"}With this PR:
{"commit":"1551d42a177d","dirty":false,"env":{"APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_FRAMES":"450:100"},"gpu":"AMD Radeon RX 6650 XT (RADV NAVI23)","driver":"radv Mesa 26.2.4-arch1.1","os":"Linux 7.2.9-arch1-1","cpu":"AMD Ryzen 9 7950X 16-Core Processor","cpu_threads":32,"memory_mib":127901,"title_id":"PPSA25636","title":"Echoes of the End","content_version":"01.012.000","resolution":"3840x2160","window":{"spec":"frames 450:100","first_present":450,"ms":110954.006},"presents":100,"frame_ms":{"p50":1036.931,"p90":1601.300,"p99":2162.815,"max":2162.815},"gpu_busy_ms_per_present":453.019,"worker_busy_pct":{"queue worker 0x0":99.816,"queue worker 0x20":97.643},"top_phases_ms_per_frame":{"Driver.Packet":2153.495,"Driver.Packet.dispatch":1166.453,"Driver.Dispatch":1074.981,"Vulkan.Dispatch":681.489,"Resources.Complete":675.709,"Vulkan.Dispatch.shader_resources":671.282,"Driver.Packet.memory_wait":617.489,"GpuMutex.wait.indirect":162.506},"trace_dropped":true,"trace_dropped_threads":{"queue worker 0x0":{"events_full_after_s":36.961,"events_lost":463551,"totals_full":false}},"trace":"aps5-trace-1551d42a177d-112044.json.gz"}tools/performance_compare.py: mean frame -3.0 %, p50 +3.3 %, p90 -4.8 %, p99 -16.7 %, GPU busy -4.1 %. 0 amdgpu kernel events in either.c83c5ea1c9measured mean -15.1 %; its files are the non-currentassets of the same release.Direct links to the existing capture evidence
These are the already-published integration captures summarized above, not a new title run or a plain-main benchmark. Their digest output has been regenerated from the linked traces and matching summary files with
tools/trace_digest.pyatdb6af9e0fd; both outputs match the existing release digests byte for byte.Baseline integration without this PR: summary line, trace, trace_digest output.
Integration with this PR: summary line, trace, trace_digest output.
Both traces dropped events. The existing limits still apply: median frame time is 3.3% worse, mean frame time is 3.0% lower, and a repeatable FPS gain is not established. This evidence-link update does not change the implementation or claim new GPU or title validation.
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 (no unimplemented path added).mdfiles or images added to the repository (attach them to this PR)git merge-treeshows two mechanical conflicts inGuestBufferMemory.cpp, described on fix(agc): bind heap mirrors only where the guest holds bytes #7647.🤖 Generated with Claude Code