Repository navigation
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
github-actions
Bot
added
perf
|
This pull request is labeled |
MarcZX10
reviewed
Oct 21, 2026
| if (begin >= served) continue; | ||
| const auto bytes = mirror.buffer->Bytes().subspan(static_cast<std::size_t>(begin - mirror.base), static_cast<std::size_t>(served - begin)); | ||
| heapReferences.emplace_back(begin, std::vector<std::byte>(bytes.begin(), bytes.end())); | ||
| auto storage = ReferenceStorage::Shared().Take(bytes.size()); |
Contributor
There was a problem hiding this comment.
When Take() reuses a previously larger vector, assign() updates its contents but does not reduce its size. WriteBack() later requires reference->second.size() == length for each merged heap range, so a smaller subsequent range can hit heap mirror write-back has no reference even though the begin address matches. Could this resize the vector to bytes.size() after ensuring capacity (or resize before copying), and add a regression that returns a larger buffer then takes it for a smaller reference and exercises the size invariant?
Contributor
Author
There was a problem hiding this comment.
Thanks for checking this. The size does follow the new reference: std::vector::assign(first, last) replaces the whole contents, so size() becomes last - first while the larger capacity is kept ([sequence.reqmts]). The vector is not resized to its capacity anywhere on this path, so WriteBack's size() == length check holds.
317d528 adds checks for it on the production path:
agc_heap_mirror_reference_size(--heap-mirror-reference-size, Linux and Windows) makes writable heap-mirror builds of 8 KiB, then 2 KiB, then 8 KiB throughAddWritable/Upload/WriteBack, with a GPU store in each, and checks that every store reaches guest memory. The 2 KiB reference takes the 8 KiB storage the first write-back returned.agc_reference_storagegainsLargerStorageTakesTheReferenceSize: a smallerassigninto larger reused storage keeps the same allocation, has the smaller size and holds the reference's bytes.
Negative control: with takeHeapReferences changed to keep the storage at its capacity (resize(capacity()) plus a copy instead of assign), agc_heap_mirror_reference_size fails with AGC graphics: heap mirror write-back has no reference, which is the failure you describe. With the PR's assign it passes, as do agc_driver_graphics and agc_reference_storage (Linux 7.2.9, GCC, Release).
|
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.
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
12 tasks done
Contributor
Author
|
A matched-seed capture pair on a real title. It does not show a measurable gain from this change, so the PR stays in draft. Setup
Summary lines
Reading
So: no regression and no demonstrated improvement on this title. The PR needs a capture from a title where heap-mirror reference allocation shows up in the profile before it can leave draft. |
12 tasks done
Contributor
Author
|
Third title, with a matched comparison: Code Violet (PPSA26528) on Linux/RADV. Shader-resource time is about 14% lower with this PR on the menu scene. Setup
Why this title. A DWARF profile of the control at 200–220 s puts
AI-assisted: yes (Claude Code); results above were run on this machine. |
Contributor
Author
|
Draft status, per the current readiness rule: this stays a draft for one specific evidence blocker, not for queue reasons. It is a
It becomes ready once a present-matched pair, from a title whose profile shows heap mirror reference allocation ( |
Contributor
Author
|
Correction to my Code Violet comment above: its setup said the runs used "identical shader-cache and save seeds". That was wrong. I have edited the comment.
The measured numbers are unchanged. The AI-assisted: yes (Claude Code); results above were run on this machine. |
Contributor
Author
|
Protocol note on my measurement above (comment 6332826974): it does not meet the performance-report protocol's "do not redirect the log" rule. The game's stdout and stderr were redirected to AI-assisted: yes (Claude Code) |
Contributor
Author
|
Controlled AMD/Linux run on Code Violet (PPSA26528). It replaces my earlier, unconstrained Code Violet numbers above, which remain observations only. Scope: integration-relative, not
Protocol
AI-assisted: yes (Claude Code); results above were run on this machine. |
12 tasks done
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
GuestBufferMemory::takeHeapReferencescopies the served bytes of every written heap-mirror range into a newstd::vectoron each build, andWriteBackdrops it. References are as large as the written heap range (hundreds of MiB in the scene below), so each build faulted in and zeroed fresh anonymous pages and unmapped them again: in a DWARF profile of the scene below (integration base8a251ef745),takeHeapReferencesheld 16.6 % of the process samples, 7.9 % of them in anonymous page faults. A profile of an earlier candidate of this change (4165273dc7, same reuse with an allocating store) put it at 5.5 % with no page faults; that profile is of the earlier candidate, not of this commit.Released reference buffers now go to
ReferenceStorage, a store of at most eight idle buffers and 2 GiB, and the next reference takes the smallest one that fits. The bytes copied and compared are unchanged.TakeandReturnarenoexceptand allocate nothing (vector moves only; the byte count changes after the move; evicted storage is destroyed after the lock is released), so the release in~GuestBufferMemoryandWriteBackcannot throw. Storage above the limit, or without capacity, is destroyed as before.Tested
main9f4ddf7; this branch has the changecc53b45601and a test commit317d528af7.agc_reference_storage(CPU): empty store, reuse of the same allocation, smallest fit, eviction of the oldest by byte limit and by slot count (checked against the identities of the retained buffers), oversize storage, andReturn/eviction/Takewith the allocation-failure countdown armed (still armed afterwards, byte count consistent). A store whose eviction allocates terminates the test withstd::bad_alloc.ctest -R 'heap_mirror|sparse_heap|reference_storage|bda|guest_memory_lifetime|sparse' -Vonmainand on this PR, same machine and GPU,ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1,VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(layer 1.4.363 loaded; neither log reportswas not found but was requested): main (9f4ddf7) 28/29, PR (cc53b45601) 29/30 (the extra test is the new one). Both failagc_driver_guest_memory_lifetimethe same way (radv/amdgpu: The CS has been rejected (-14),guest prefix unmap failed), and both logs hold the same validation errors: 2 ×VUID-vkCmdCopyBuffer-srcBuffer-00118, 2 ×VUID-vkResetFences-pFences-01123. No newVUID-.Before (integration
8a251ef745):seconds 40:60 from present 340: 11 presents, frame p50 5867.4 / p90 7221.6 / p99 7377.7 / max 7377.7 ms, GPU busy 543.2 ms per present
seconds 40:60 from present 351: 11 presents, frame p50 5427.9 / p90 6189.5 / p99 7141.4 / max 7141.4 ms, GPU busy 497.8 ms per present
After (integration + this PR,
6e0a219cbe):seconds 40:60 from present 343: 15 presents, frame p50 4069.4 / p90 5171.4 / p99 5460.5 / max 5460.5 ms, GPU busy 497.8 ms per present
seconds 40:60 from present 348: 16 presents, frame p50 4004.1 / p90 4933.0 / p99 5394.2 / max 5394.2 ms, GPU busy 479.7 ms per present
The title still loads at well under 1 FPS on both sides. The captures ran on an integration with the earlier base
33a6d6bbfb; the PR commit was cherry-picked onto it (6e0a219cbe). Traces, logs, screenshots, profiles and validation logs: evidence archive (SHA-25600bbfd328cc0ecbed29aedd3925217dc6e39a9a018170bd941549b5146af27be).APS5_TRACE_SECONDS=150:30, one run each, separate relinked runtimes, initial shader/pipeline cache state not reported as matched. Base maine0e57a3270+ fix(libSceVideodec2): answer the generic picture query for VP9 pictures #8839 (641fce5c84), change = base + this PR (91d0f400ba):Before: seconds 150:30 from present 26: 6 presents, frame p50 4885.1 / p90 7341.3 / p99 7341.3 / max 7341.3 ms, GPU busy 186.3 ms per present
After: seconds 150:30 from present 40: 6 presents, frame p50 5074.1 / p90 5685.3 / p99 5685.3 / max 5685.3 ms, GPU busy 178.7 ms per present
Resources.Complete5.29 vs 5.14 ms per dispatch: no measurable change in that title, where the per-dispatch cost is the copy and theWriteChangedcompare that this PR keeps. Traces (SHA-2564412bd6c74fc8493e765d3d33b970a6b947ef900e09e6a38384d0ebc4f659c25).agc_heap_mirror_reference_size(317d528) checks a smaller reference written back after a larger one reused its storage; withtakeHeapReferenceskeeping the storage's capacity as its size it fails withheap mirror write-back has no reference.python3 tools/check_conventions.py --base origin/mainpasses. The overlap tool reports fix(agc): fall back to device-local heap mirrors #8016 and fix(agc): bind heap mirrors only where the guest holds bytes #7647, which rewrite the same reference construction for device-local and resident heap mirrors; they do not reuse storage, and whichever lands later rebases onto the other.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