Repository navigation
fix(agc): let heap mirrors replace host imports when the import budget is full - #12074
Merged
oneandonlydean merged 4 commits intoOct 29, 2026
Merged
oneandonlydean merged 4 commits into
oneandonlydean merged 4 commits into
Conversation
github-actions
Bot
added
the
unblocks title
MarcZX10
suggested changes
Oct 27, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
The two new regression scenarios target the reported failure modes, and the implementation only yields single-range, releasable, currently watched imports to mirrors. Exact-head conventions checks report no errors (with the existing debt-length warning), git diff --check is clean, and the branch merges cleanly with current main. The hosted Windows build has not finished, so I have not assessed its result yet.
Please resolve the remaining contribution-rule items before merge:
- The PR reports 5 fps versus 11–17 fps, but provides neither matched
main/change summary lines nor attached traces.CONTRIBUTING.mdrequires the performance-report artifacts for speed claims. Please add a current-main comparison from the same title, scene and machine with the traces and digest, or remove the speed comparison and keep the claim focused on correctness/avoiding the abort and repeated copies. - The body identifies a real debt-file conflict with open #7844, but says only that whoever lands second must rebase;
Depends on: nonedoes not record an agreed order. Please coordinate which PR lands first with its author and declare that dependency/order in the checklist, while preserving both debt updates.
Contributor
Author
|
Thanks for the review. On point 1: I removed the fps comparison from the body. The PR now claims only correctness: the copy cap abort is gone, and refusals per run drop from thousands to a few hundred. It makes no speed claim. On point 2, I'm coordinating the order with the author of #7844 and will update the checklist once we've agreed. |
|
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.
|
Contributor
|
A second title this gets past the import-budget wall: RESIDENT EVIL requiem (PPSA31246), Windows 11, RX 9070 XT (AMD 26.6.2), 32 GB RAM. Without this PR, loading the area after the opening cutscene ends the same way every time:
With this PR's
In the log after about 25 minutes:
The budget is still full ( Not tested: I haven't run this PR's ctests; I only tested the title. AI-assisted: yes (Claude Code). |
Contributor
Author
|
Thanks for testing it on RESIDENT EVIL requiem (PPSA31246) on AMD. A second title and a second vendor getting past the same budget wall is very useful. The refusals you still see once the budget is full match what is left on Astro Bot: this PR stops the abort and the full-range copies, but it doesn't make the budget any bigger. That's a separate change. |
Contributor
Author
2 of 3 tasks
…or can replace An address-based build that copies a range the budget refused and no mirror serves (a range outside the write watch, or not releasable) evicts idle imports when the copy would pass APS5_ADDRESS_COPY_MAX_MIB. Imports used in the current or the previous frame (before two frames have ended, within the last 256 lookups) never count as idle, so once a build uses an import every frame, as it does for the pieces boykopovar#8380 imports of a heap over the budget or a whole heap imported before, nothing can be evicted and the build aborts. That eviction now also takes imports the build has not taken yet whose single range is releasable and write-watched: the build then serves that range from a heap mirror instead.
… imports a mirror can replace A cached address space keeps a heap mirror whose range has left the write watch while the budget refuses the range's import, and such a mirror is copied in full at every build. When the budget is held by imports a build uses every time, as the pieces boykopovar#8380 imports of a heap over the budget are, the import was refused at every build, each refusal counted, and every build copied the whole range (about 1.1 GiB in Astro Bot). The check now evicts for that import as the copy cap does: idle imports, and imports a heap mirror can replace that no build in progress holds.
…h serial moves on Linux
…red swap keeps each one inside its mapping
adriadam10
force-pushed
the
fix/agc-import-yield-mirrorable-pr
branch
from
October 29, 2026 07:24
db3a5cd to
c283a72
Compare
MarcZX10
suggested changes
Oct 29, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Re-review — changes requested
The latest description now explicitly makes no speed claim and focuses on the correctness result: the copy-cap abort is avoided and repeated full-range copies/import refusals are reduced. That resolves my earlier request for matched performance-report artifacts or removal of the speed comparison. The author reports the two new regression tests pass with this change and fail on main; I did not rerun them locally.
Two integration items remain:
- The reported test baseline and PR base are
14332cd8d0, while current upstream main isaaccd0ba39b7. Please refresh onto current main and rerun the focused address-copy/import tests on the exact resulting head. - The debt-file conflict with open #7844 is still unresolved. The author says either order is acceptable and leaves the choice to maintainers, but the checklist still says
Depends on: none, and neither PR records an agreed order. Please get that order selected and recorded on both sides; if #7844 lands first, preserve its debt edit while rebasing this PR as described.
The authored Windows/AGC suite report is useful, but the hosted Linux product job is skipped and I did not run a local GPU build. I am holding approval for the current-main and overlap items, not for the removed speed claim.
12 tasks done
12 tasks done
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
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
Since #8380 imports a heap over the host import budget in pieces, the budget (8132 MiB here) can fill with imports that every address-based build uses. #11357 then protects every import used in the current or the previous frame, so none of them is evictable. Astro Bot (PPSA21567) has a working set of about 12 GiB and hits two problems:
APS5_ADDRESS_COPY_MAX_MIB, the evicting retry finds nothing to evict, and the title stops withFATAL: an address-based build copies ~1.1 GiB ... import over the host import budget, heap not fully write-watched.Many of the imports holding the budget cover a single releasable range that is still write-watched, so a heap mirror can serve that range just as well. This PR lets both paths evict those imports:
AcquireRegistered): the evicting retry for a range only an import serves may also evict imports whose single range is releasable and write-watched, even inside the frame protection, as long as the current build has not taken them yet. The build then serves those ranges from heap mirrors. Remembered refusals are not used for this retry.importsNow): for a cached heap mirror that left the write watch, the import attempt now evicts the same way, with the imports of earlier builds no longer held, instead of the non-evicting attempt that was refused every time.Other requests keep the current eviction rules. The debt entry for host import windows and budget describes the new eviction.
Checked against open and merged PRs:
Neither fixes it.
Tested
Windows 11 Pro, MinGW GCC (winlibs), Release, NVIDIA GeForce RTX 4070 Ti.
mainis 14332cd; each tree was built in its own build directory.New tests (each runs with 0, 1 and 2 frames ended, so both the lookup-count rule and the two-frame rule are covered):
agc_address_copy_cap_mirror: a build past the copy cap needs a range outside the write watch while the budget is held by a recently used, write-watched import.mainwith only this PR's test changes: aborts withFATAL: an address-based build copies 2 MiB of 1 registered ranges, past 1 MiB (APS5_ADDRESS_COPY_MAX_MIB); ... (2.0 MiB committed, import over the host import budget, heap not fully write-watched), the same abort as in Astro Bot.agc_address_space_unwatched_mirror_import: a cached heap mirror leaves the write watch while the budget is held by another heap's import.mainwith only the test changes: fails (a heap whose mirror left the write watch was not imported in place of an import a mirror can replace).agc_heap_mirror_unwatchnow takes its idle and busy ranges out of the write watch on Windows, so they are outside it as on Linux (where only the heap is registered) and a mirror cannot stand in for them. It passes onmainand with this PR.AGC suite,
ctest -R "agc|shader_cache" -j 4:main: 1107 of 1108 pass.agc_driver_mesh_barycentric_emulation(the pixel shader did not emulate barycentrics). It does not touch host imports.Validation layer:
ANYPS5_REQUIRE_VULKAN=1 ANYPS5_NO_SHADER_CACHE=1 VK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation ctest -R "agc_address|agc_heap_mirror|host_import" -Vmain: 19 of 19 pass.agc_host_import_sparseis skipped on both (udmabuf imports are Linux only).VUID-messages over 7 real devices. The new tests run against the mock Vulkan device, so the layer does not see them.Astro Bot (PPSA21567), Windows, RTX 4070 Ti. Measured on our integration tree with the pin fix (#6182's change) and #11307, not on plain
main:Black Ops (BO1) output is identical on the same tree (byte-equal frames and draw records in a strict frame replay). This PR makes no speed claim.
python3 tools/check_conventions.py --base origin/mainreports no errors and one warning: the host import debt entry, already over 1000 characters onmain, grows by about 340.python3 tools/pr_overlap.pyreports one conflict: #7844 edits the same debt entry (docs/dev/debt/libSceAgcDriver.mdL261). Either order is fine with me; the maintainers can choose. If #7844 lands first, this PR rebases on it, keeps both debt updates, and setsDepends on: #7844.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)