Repository navigation
fix(agc): count copied guest buffers and draw snapshots in the batch byte budget - #2283
Conversation
|
I reviewed the complete six-file diff. The byte accounting follows the retained |
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the bound makes sense. Build and the libSceAgcDriver ctest scope are green here (the new assertions fail to compile without the change). Points to address:
tests/BdaResources.cpp:431:CopiedBytes() == 0 || CopiedBytes() == sizeof(guest)accepts both outcomes. Pin the expected value for the case the test sets up (copy vs. import), or build one case for each.- The test only covers the accessor. Reverting any of the three
Keepcalls (VulkanDevice.cpp:3318,Draw.cpp:1176,ShaderResources.cpp:3159) leaves it green. Please add a test showing that the kept bytes reach the recorder's budget (see the existingagc_recorder_kept_bytes_testsfixture). - Please confirm a
ShaderResourcesretained by several draws/dispatches is not counted once per keep, otherwise the batch is submitted earlier than the budget says.
|
Thanks for the review. Pushed 26d5926 (and 55b09c0, which drops comments the conventions check rejects): BdaResources: the check now pins CopiedBytes() == sizeof(guest). guest is stack memory no allocation registers, so no host import serves it and the two views merge into one copied region (the WriteBack publishing the aliased write below already depends on that copy). New device test agc_driver_recorded_kept_bytes (tests/execution/RecordedKeptBytes.cpp). It reads the open batch's kept bytes (new Recorder::OpenKeptBytes) after a recorded dispatch writing a copied 64 KiB buffer, after a recorded draw reading a copied 64 KiB buffer (into a resident target, so the draw is recorded rather than synchronous), and after two draws reading a registered, write-watched 64 KiB buffer in place, which take a draw snapshot. I checked it against each revert separately: removing the dispatch keep, the draw keep or the snapshot keep each fails it, and so does counting a reused snapshot twice. It takes about 0.2 s on lavapipe. The mock fixture gets SharedOwnerCountsOncePerBatch for the counting rule itself. Tested on Linux with lavapipe, with the PR merged into current main: the libSceAgcDriver ctest scope gives 320/320 passing (27 skipped because llvmpipe has subgroup size 8). |
|
Additional platform validation on standalone PR head Linux: passed — Arch Linux in WSL2 (kernel 6.6.87.2), GCC 16.2.1 20260810; CMake 4.3.1 / Ninja. Release / Tests: Reproduce after building the test targets: Vulkan: Mesa lavapipe 26.2.4 software Vulkan; no native Linux GPU/retail-title validation. Internal limitation: write-watched guest-heap mirror subcases returned early because WSL userfaultfd async write protection is unavailable. The other graphics assertions ran; zero CTest skips does not include this internal early return. |
|
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.
|
|
With that call changed to AI-assisted: yes (Claude Code Fable 5.1 xhigh) |
|
Windows cross-test of #2283 head 55b09c0, applied to the 007 integration stack at Release MinGW GCC 15.2, Windows 11, RTX 5070 Ti, SPIRV-Tools OFF, Vulkan required: 4/4 selected CTest checks pass ( This is an integration-stack result, not a standalone build of #2283 on main. A fresh 007 run is next; no game-result claim for this head yet. My standalone prototype AI-assisted (Codex). |
|
Linux cross-test of 55b09c0. Setup: CachyOS (kernel 7.2.9, GCC 16.2.1), RTX 3080 (NVIDIA 615.71.09), Release. Merged onto
007 First Light (PPSA11386) An earlier trace on the 75e319f stack from #2523, which has the same With the three commits cherry-picked:
Combined with #2992 there's no jump either: the lowest available memory is 27.2 GiB. One thing to know about that combination: AI-assisted: yes (Claude Code) |
|
Follow-up to #2283: I reproduced a buffer-only enforcement gap and pushed 0280ac3 on The existing bound checks are in sampled/storage texture lookups. A dispatch with no images can keep adding counted buffers without reaching either check. The new Vulkan regression records one buffer-only dispatch, seeds that open batch at the existing byte budget using a kept test holder, then dispatches to a distinct buffer. It requires that the preceding batch has submitted. The helper holds Negative control: with #2283's byte accounting but no entry checks, the regression fails: @MathiasDevelopes, happy for you to fold the follow-up into #2283 with authorship preserved if useful. It depends on #2283's recorded-kept-bytes fixture; the integration also has the already reported fifth The currently running 007 process still has f4d4d04, not this follow-up, so I am not attributing its behavior to these entry checks. At 369 flips it remains black, paused/resumed under the memory guard during the same large NVIDIA pipeline compilation. No playability claim. AI-assisted (Codex). |
|
Linux check of @Scramblrrrr's follow-up 0280ac3. Setup: CachyOS (kernel 7.2.9, GCC 16.2.1), RTX 3080 (NVIDIA 615.71.09), Release, SPIRV-Tools on. Tested on
AI-assisted: yes (Claude Code) |
…byte budget The recorder bounds the bytes a batch keeps alive (KeptBytesBudget) and waits for older batches past twice that. A recorded dispatch or draw kept its ShaderResources, and a draw its snapshot buffers, with a size of 0, so the host-visible copies of guest buffers they own never counted. A title that binds a 124 MiB writable guest buffer in every draw then held about ninety copies, 11 GB, until the GPU caught up, and aborted with VK_ERROR_OUT_OF_HOST_MEMORY from vkAllocateMemory. GuestBufferMemory reports the bytes of its copied regions, and the dispatch and draw keeps and the draw snapshots pass their bytes to Recorder::Keep, so the existing bound applies to them. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
…ree keep sites A ShaderResources from the resource caches is the same object for every draw or dispatch that uses it, and Recorder::ReusableDrawSnapshot hands the next draw the same snapshot buffer, so passing their bytes with each Keep counted one allocation once per use and submitted the batch before it held the budget. Recorder::KeepBytes counts an owner's bytes once per batch, and the dispatch, draw and snapshot keeps use it. The recorded kept bytes test runs a dispatch and a draw that copy a guest buffer and two draws that snapshot one on the device, and fails when any of the three sites stops counting or a reused snapshot counts twice. The BdaResources check pins the copied region it sets up. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HZHkb8kUwnHAuBBbHywvJf
Comments are reserved for technical debt (docs/dev/TechnicalDebt.md), so the ones the previous commit added to the recorder, the snapshot keep and the tests go. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HZHkb8kUwnHAuBBbHywvJf
Co-authored-by: orishamir <orishamir@users.noreply.github.com> Co-Authored-By: Claude <noreply@anthropic.com>
55b09c0 to
d731f50
Compare
adriadam10
left a comment
There was a problem hiding this comment.
Thanks, the new tests address the earlier review. Checked on current main:
- The new execution test did not build against
main:GuestAllocations::Mutation().Addnow takes a fifthgpuargument (tests/execution/RecordedKeptBytes.cpp:97, found by @orishamir). Added as our own commitAdd(block, bytes, true, true, true). - Build and ctest of
bda_resources,recorder_kept_bytesandrecorded_kept_bytespass. Without the change the tests don't compile; removing theKeepBytescall atVulkanDevice.cpp:3489orDraw.cpp:1436makesagc_driver_recorded_kept_bytesfail. - Cross-tests by @Scramblrrrr (Windows) and @SleglJan (Linux, RTX 3080) agree.
Merging once the full Build workflow is green. The buffer-only follow-up (0280ac3) can go in its own PR.
What
The [recorder](https://github.com/boykopovar/AnyPS5/blob/main/core/libs/prx/libSceAgcDriver/Graphics/src/Recorder.cpp) bounds the bytes a batch keeps alive:
BoundKeptBytessubmits an open batch atKeptBytesBudget(512 MiB) and waits for the oldest in-flight batches past twice that. Only bytes passed toRecorder::Keep(object, bytes)count, and a recorded dispatch or draw kept itsShaderResources(and a draw itsDrawBindingssnapshots) with a size of 0. The host-visible copies of guest buffers that these objects own were never counted.Planet Coaster (PPSA01736) binds a 124 MiB writable guest buffer in nearly every draw after its intro. The process went from 8.7 GB to 20.7 GB private memory within three seconds. 98 buffers of 124 MiB were created and 12 released, until:
GuestBufferMemory::CopiedBytes()now reports the bytes of its regions that are served by a copy,ShaderResources::CopiedBytes()forwards it, and the dispatch keep, the draw keep and the draw snapshots pass their bytes toRecorder::Keep. The existing bound then applies to them; nothing else about the bound changes.Related open PRs: #1364 (and #1366, which contains it) counts retained draw upload pages in the same budget; it does not touch these keeps. #1997 passes sizes for its own image merge buffers. The call sites above are untouched by both.
This is a bound, not a fix of the copies themselves: the title still re-copies that buffer for each draw (see the related region fix in a separate PR).
Tested
Windows 11, MinGW-w64 15.2.0, RTX 3060.
agc_driver_bda_resources:CopiedBytes()is 0 before an upload and, after it, either 0 (served by an import) or the one copied region's size.tools/check_conventions.py --base origin/mainpasses.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt.mdfiles or images added to the repository🤖 Generated with [Claude Code](https://claude.com/claude-code)