Repository navigation
perf(agc): read APS5_NO_QUEUED_DRAW_READ_ORDER once in the held-read check - #9490
Conversation
|
This pull request is labeled |
MarcZX10
left a comment
There was a problem hiding this comment.
The report has useful comparative timing measurements, but it does not yet meet the repository-required evidence for a speed claim. CONTRIBUTING.md requires the aps5-summary.jsonl summary lines from main and the change for the same title, scene, and machine, with both traces attached and the output of tools/trace_digest.py. The PR explicitly says it has no trace pair or digest yet, and the automated report check requested those artifacts. Please add the required summaries, traces, and digest before this performance change is considered ready.
|
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.
|
|
@MarcZX10 Thanks, added to the description:
|
MarcZX10
left a comment
There was a problem hiding this comment.
The new report is much more complete: it includes same-machine Spirit of the Island traces, digests and multiple comparison pairs. Two repository-required checks are still missing. First, the shown main/change trace heads both identify 0c6a5bd59 as dirty; CONTRIBUTING.md requires valid summary lines with different commits, and the automated perf numbers missing label depends on that pair. Please capture main and this committed change from the same scene and machine with distinct commit IDs, and ensure the traces correspond to those summaries. Second, this modifies the AGC driver, so please add the same focused Vulkan tests on main and this change with ANYPS5_REQUIRE_VULKAN=1, ANYPS5_NO_SHADER_CACHE=1, and VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation on the same GPU, confirm the layer loaded in both logs, and compare for new VUID- lines. No local tests were run here; hosted product/relinker jobs are skipped.
|
The submitted trace archive and digest are useful. One detail blocks the required performance evidence: the displayed digest heads for both arms report commit |
5c0e20e to
cfca4e5
Compare
|
Another data point for this change, from ASTRO BOT (PPSA21564) in the Crash Site hub. Here the per-call Setup
Before profile: with the per-call version, a steady-state Results: per-commit times come from the driver's own committer timer (mean of the last three 10 s windows).
|
|
@SoRadGaming thank you. A second title, a different GPU and a committer profile pinning the time on |
|
A third title: GTA V (PPSA04264), open world from a resumed save. Windows 11, RTX 4090 616.92, i9-13900K, WinLibs GCC 15.2 Release. Tree:
GTA's share is lower than Spirit of the Island (7.3 %) and ASTRO BOT. It is the same cost on a third title and a different CPU vendor, and a static read removes it. AI-assisted: yes (Claude Code) |
|
Thanks @tibzejoker. A third title on a different CPU vendor and GPU, with the cost in the same On my side, the Windows validation-layer run on head |
adriadam10
left a comment
There was a problem hiding this comment.
The change itself looks right: APS5_NO_QUEUED_DRAW_READ_ORDER is read once into a static in HeldReads, as WaitMemory.cpp already does, and the cost is confirmed by three independent profiles. The merge queue refused it only because @MarcZX10's CHANGES_REQUESTED is still open, and an approval from us doesn't clear another reviewer's request. Two things are left, as he asked:
- A main/change trace pair from distinct, non-dirty commits (
aps5-summary.jsonllines andtrace_digest.pyoutput, same title, scene and machine), captured after the change is committed. The pair in the description reports0c6a5bd59 (dirty)for both arms, and the one you said was pending on 2026-10-23 isn't posted yet. - The focused AGC validation comparison on the current head
cfca4e5d4a: the 26/26 and 0 VUID you posted is for89e66ea537. Please post the logs for both arms showing the validation layer loaded (nowas not found but was requested by env var VK_INSTANCE_LAYERS), and theVUID-comparison.
Once both are in the PR, @MarcZX10 can clear his review and this can go to the queue.
…check DrawPipeline::HeldReads called std::getenv on every call. It runs for each StorageTexture::FlushPending, which InPlaceDrawInput calls while the draw committer records a draw. On Windows, UCRT's getenv takes the environment lock and scans the block on each call: on the title screen of Spirit of the Island (PPSA13934), 375 of the draw committer's 5124 steady-state samples (7.3 %) were in ucrtbase under HeldReads, against under 0.3 % for the range scan itself. The variable is now read once into a static, as WaitMemory.cpp already does for the same variable. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cfca4e5 to
29b6cc4
Compare
|
Refreshed on current main as This addresses the stale-head Vulkan qualification part of the review. The clean committed native performance pair/digest remains outstanding, and the description now clearly excludes the earlier dirty-head comparison from qualifying evidence. No FPS gain or new Windows run is claimed. Logs, JUnit, pins and patch, SHA-256 AI-assisted: yes (Codex). |
|
Added a clean committed native pair for the remaining trace-evidence request: Afterdream PPSA18391 opening dialogue, warmed caches, same machine/scene/settings and window state, main This Linux scene is60FPS-capped: median16.683ms in both arms, p99 16.705/16.716ms. One pair and differing background CPU load do not establish a speed or CPU-cost improvement. No new Windows run is claimed; the independent Windows profiles remain separately attributed. Traces, full digests, pins and native reports, SHA-256 AI-assisted: yes (Codex). |
|
A Windows data point from Persona 5 Royal (PPSA05110, own dump), casino gameplay: Windows 11 26200, Ryzen 5 5600X, RX 6700 XT (AMD 32.0.21045.16010), WinLibs GCC 15.2.0 r7, Release. The title needs four open PRs to load and run (#10887, #10614, #10913, #7844), so every build below is
This isn't an argument against the change: the lookup is still per call wherever AI-assisted: yes (Claude Code) |
|
@MarcZX10 @oneandonlydean The two evidence requests are now covered on published head
Both archives' hashes and the clean summary fields were rechecked for this follow-up. These remain the previously posted runs, with their original source pins. The Linux scene is capped at about 60 FPS (p50 16.683ms on both arms), and differing background CPU load prevents a CPU-cost improvement claim. No new speed or Windows result is claimed. Please re-review these attachments and update the earlier changes-requested review if they satisfy the two requests, so the queue can proceed. AI-assisted: yes. |
Dismissing a stale review: the requested changes were addressed after it (new head) and the merge queue blocks on it. Please request changes again if anything is still open.
|
Checked the diff against the description: |
What
DrawPipeline::HeldReadslooked upAPS5_NO_QUEUED_DRAW_READ_ORDERfor each nonempty range query. It now caches whether the option is present once per process, asWaitMemory.cppalready does for this option. The range checks and queued-read ordering remain intact.Independent Windows profiles in the earlier comments identify repeated
getenvcalls in this hot path. They motivate the change; they are not a new before/after frame-rate result.Tested
Refreshed as
29b6cc46afed2d6dadc6191986cb07ac6693eae1, one commit directly onmaine7e3e9c1984c293f06bff08428b8c89b7b932ac2, with no merge commits. The complete source file equals current main plus the original two-line lookup change.Fresh Linux qualification on AMD Radeon RX 6650 XT (RADV NAVI23), stock Mesa26.2.4, Arch Linux7.2.9, GCC/Release:
libs,relinkerand focused test builds pass for main and the change.VK_LAYER_KHRONOS_validationinsertion. The other9 are no-device regression paths.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1and the requested layer are set for both arms; Mesa's shader cache is disabled.The earlier dirty-head trace pair remains excluded. The clean native pair below provides distinct-commit summaries/traces and digests, with no Linux speedup claim.
Download exact-head Linux builds, per-group logs/JUnit, source pins and reviewed patch. SHA-256
047c4840394a85c94ceeaf063fd0aa118c842802a37909c4f96755adf5fdb5cb.Clean native performance capture
Afterdream PPSA18391, content02.000.000, opening dialogue held after the same scripted Cross presses at20/45/70s. Same Linux machine/session, RX6650XT/RADV, 3840x2160 guest output in the same1536x864 window on DP-2. Both windows were inactive in all10 focus samples during the80-100s capture. Both scenes were run before capture to warm their private caches and restarted with empty private writable state. Converted app/module bytes are identical between arms; only current built libraries differ. Game logs stayed attached to a terminal. Only
APS5_TRACE_SECONDS=80:20and trace capacity were enabled, with no diagnostic GPU dumps/profiling and no concurrent builds.Distinct clean commits: main
e7e3e9c1984c293f06bff08428b8c89b7b932ac2and PR29b6cc46afed2d6dadc6191986cb07ac6693eae1. Both exit normally, restore configuration and report zero dropped trace events. The scene remains capped at about60FPS: p50 16.683ms on both arms, p99 16.705ms main /16.716ms change. This single pair shows no Linux FPS benefit. Estimated other-host CPU load averaged15.94% main /11.49% change, so it is not evidence of a CPU-cost improvement either. The independent Windows profiles above remain separate supporting evidence; our Windows host is user-paused.Summary lines, main then change:
{"commit":"e7e3e9c1984c","dirty":false,"env":{"APS5_DRAW_CACHE_ENTRIES":"4096","APS5_HOST_IMPORT_MIB":"16384","APS5_PAD_INPUT_FILE":"<path>","APS5_PIPELINED_DRAWS":"1","APS5_PLAYGO_UNLISTED_CHUNKS":"absent","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"80:20"},"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":"PPSA18391","title":"Afterdream","content_version":"02.000.000","resolution":"3840x2160","window":{"spec":"seconds 80:20","first_present":4727,"ms":20003.310},"presents":1199,"frame_ms":{"p50":16.683,"p90":16.691,"p99":16.705,"max":16.743},"gpu_busy_ms_per_present":10.322,"worker_busy_pct":{"queue worker 0x0":98.894},"top_phases_ms_per_frame":{"VideoOut.Flip":16.641,"Driver.Packet":16.453,"VideoOut.Flip.vblank_wait":15.751,"Driver.Packet.rendering_wait":10.538,"Driver.Packet.draw":3.837,"Driver.Draw":3.807,"Graphics.Draw":3.401,"Graphics.Draw.draw_and_resource_release":2.915},"trace_dropped":false,"trace":"aps5-trace-e7e3e9c1984c-2104047.json.gz"} {"commit":"29b6cc46afed","dirty":false,"env":{"APS5_DRAW_CACHE_ENTRIES":"4096","APS5_HOST_IMPORT_MIB":"16384","APS5_PAD_INPUT_FILE":"<path>","APS5_PIPELINED_DRAWS":"1","APS5_PLAYGO_UNLISTED_CHUNKS":"absent","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"80:20"},"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":"PPSA18391","title":"Afterdream","content_version":"02.000.000","resolution":"3840x2160","window":{"spec":"seconds 80:20","first_present":4759,"ms":20003.318},"presents":1199,"frame_ms":{"p50":16.683,"p90":16.690,"p99":16.716,"max":16.738},"gpu_busy_ms_per_present":10.589,"worker_busy_pct":{"queue worker 0x0":98.939},"top_phases_ms_per_frame":{"VideoOut.Flip":16.639,"Driver.Packet":16.461,"VideoOut.Flip.vblank_wait":16.031,"Driver.Packet.rendering_wait":11.181,"Driver.Packet.draw":3.414,"Driver.Draw":3.388,"Graphics.Draw":3.101,"Graphics.Draw.draw_and_resource_release":2.684},"trace_dropped":false,"trace":"aps5-trace-29b6cc46afed-2140796.json.gz"}Both clean traces, full
tools/trace_digest.pyoutputs, native reports, source pins and harness, SHA-25621635a6ab0611b7f58c6caa5e8954fe1393871b2523740f82c0e4f802c9de66c.Digest excerpts, main then change (full output in the archive):
seconds 80:20 from present 4759: 1199 presents, frame p50 16.7 / p90 16.7 / p99 16.7 / max 16.7 ms, GPU busy 10.6 ms per present
AMD Radeon RX 6650 XT (RADV NAVI23), radv Mesa 26.2.4-arch1.1, Linux 7.2.9-arch1-1, 3840x2160, commit 29b6cc4, dropped: no
{"commit":"29b6cc46afed","dirty":false,"env":{"APS5_DRAW_CACHE_ENTRIES":"4096","APS5_HOST_IMPORT_MIB":"16384","APS5_PAD_INPUT_FILE":"<path>","APS5_PIPELINED_DRAWS":"1","APS5_PLAYGO_UNLISTED_CHUNKS":"absent","APS5_TRACE_EVENTS":"1048576","APS5_TRACE_SECONDS":"80:20"},"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":"PPSA18391","title":"Afterdream","content_version":"02.000.000","resolution":"3840x2160","window":{"spec":"seconds 80:20","first_present":4759,"ms":20003.318},"presents":1199,"frame_ms":{"p50":16.683,"p90":16.69,"p99":16.716,"max":16.738},"gpu_busy_ms_per_present":10.589,"worker_busy_pct":{"queue worker 0x0":98.939},"top_phases_ms_per_frame":{"VideoOut.Flip":16.639,"Driver.Packet":16.461,"VideoOut.Flip.vblank_wait":16.031,"Driver.Packet.rendering_wait":11.181,"Driver.Packet.draw":3.414,"Driver.Draw":3.388,"Graphics.Draw":3.101,"Graphics.Draw.draw_and_resource_release":2.684},"trace_dropped":false,"trace":"aps5-trace-29b6cc46afed-2140796.json.gz"}