Repository navigation
feat(agc): report live device memory allocations when the device is lost - #13148
Open
Prophet069 wants to merge 1 commit into
Open
Prophet069 wants to merge 1 commit into
Prophet069 wants to merge 1 commit into
Conversation
|
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.
|
This was referenced Oct 30, 2026
Collaborator
|
@Prophet069 thanks for these. One problem common to all five PRs (#13145, #13146, #13147, #13148, #13149): each commit sits on main For this PR, |
12 tasks done
MarcZX10
suggested changes
Oct 30, 2026
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
The PR introduces an unrelated regression that rejects draws with more than one merged color target, so I cannot approve this head.
What I ran
Reviewed head 6d832a9f907d6dee6a2ffe819b5d81f3b83d35b6 against current main base 8c8b9986204d5b1cbb23847b363df00ae832ec9c; the merge preview is clean (7ee5166fcedc64b4976c5ceb79aa7577656e7235). Environment: Linux container; no compiler invoked.
git merge-tree --write-tree 8c8b9986204d5b1cbb23847b363df00ae832ec9c 6d832a9f907d6dee6a2ffe819b5d81f3b83d35b6
git diff --check 8c8b9986204d5b1cbb23847b363df00ae832ec9c...6d832a9f907d6dee6a2ffe819b5d81f3b83d35b6
python tools/check_conventions.py --base 8c8b9986204d5b1cbb23847b363df00ae832ec9c --head 6d832a9f907d6dee6a2ffe819b5d81f3b83d35b6Results
| Check | Result |
|---|---|
| Merge preview | Pass; clean tree 7ee5166fcedc64b4976c5ceb79aa7577656e7235. |
git diff --check |
Pass; no output. |
| Conventions | Pass. |
| Hosted CI snapshot | Windows, Ubuntu/Windows relinker, and ASan passed; macOS relinker jobs were queued. |
Raw local command output
git merge-tree --write-tree ...:
7ee5166fcedc64b4976c5ceb79aa7577656e7235
git diff --check ...: no output (exit status 0).
python tools/check_conventions.py ...:
All conventions checks passed.
Findings
Blocking
ExportTargetMergenow returns oneTargetMergeand requiresmerge.mask == 0for each partial-write color. A draw with two eligible color targets reaches the second iteration with a nonzero mask and fails this requirement instead of emitting both merges. The current base retains the vector-returningExportTargetMergespath and its checks for multiple non-overlapping targets. This change also replaces the recompiler'stargetMergesvector with onetargetMerge, so multi-target merge information cannot be represented downstream. Keep the multi-target behavior and add a regression test with two eligible targets, or separate the live-allocation reporting change from this refactor. Evidence: PR headState.cpp, PR headRecompiler.hpp, and current-baseState.cpp.
Non-blocking
None identified in this source review.
Not verified
- I did not build or run CTest, the new allocation test, GPU validation, or a console/game workload.
- The author reports 64/64 focused tests and a Death Stranding 2 device-loss run; I did not independently reproduce them.
- At the status check, macOS relinker jobs were queued; Linux product and specialized jobs were skipped.
To get this to a good state
Preserve support for multiple merged color targets and add a focused regression test, then request another review. I will rerun the merge preview, diff check, and conventions check on the updated head.
vkAllocateMemory and vkFreeMemory are counted per device. At device loss the fault report lists the live allocations and bytes per memory type, split by size class and by the driver operation that allocated them (buffer pool operations, host imports, resident mirrors, the pixel LDS pool; the rest is unlabelled), and a warning is printed when the live count exceeds maxMemoryAllocationCount. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Prophet069
force-pushed
the
pr/agc-live-allocations
branch
from
October 30, 2026 05:19
6d832a9 to
ac75125
Compare
Contributor
Author
|
Thanks, you were right: I squashed onto a moving @MarcZX10 The single-target |
Merged
12 tasks done
Contributor
|
@Prophet069 a heads-up: this branch sets the files #12916 changed back to their contents before #12916 merged ( AI-assisted: yes (Claude Code) |
MarcZX10
suggested changes
Oct 30, 2026
MarcZX10
left a comment
MarcZX10
left a comment
Contributor
There was a problem hiding this comment.
Reviewed — changes requested
The allocation counter does not observe all allocations made by this device, so the live totals and allocation-limit warning can under-report Vulkan memory use.
What I ran
Reviewed head ac751257a9fa8571cd0051964020e97d0e35cf31 on base 8c8b9986204d5b1cbb23847b363df00ae832ec9c; the merge preview is clean (318b92639880d9b7e0c655535d6d6bea239efb03). Environment: Linux x86-64. I ran the following checks; no local build or tests were run.
git diff --check 8c8b9986204d5b1cbb23847b363df00ae832ec9c...ac751257a9fa8571cd0051964020e97d0e35cf31
git merge-tree --write-tree 8c8b9986204d5b1cbb23847b363df00ae832ec9c ac751257a9fa8571cd0051964020e97d0e35cf31
python3 tools/check_conventions.py --base 8c8b9986204d5b1cbb23847b363df00ae832ec9c
gh pr checks 13148 --repo boykopovar/AnyPS5Results
| Check | Result |
|---|---|
| Diff check | Passed; no output |
| Merge preview | Clean |
| Conventions | Passed |
| Hosted checks | Windows, conventions, eligibility, labels and report passed; Linux, register-context, relinker, relinker-asan and write-watch skipped |
| Local tests | Not run; no build performed |
Check output
git diff --check 8c8b9986204d5b1cbb23847b363df00ae832ec9c...ac751257a9fa8571cd0051964020e97d0e35cf31
(no output; exit status 0)
git merge-tree --write-tree 8c8b9986204d5b1cbb23847b363df00ae832ec9c ac751257a9fa8571cd0051964020e97d0e35cf31
318b92639880d9b7e0c655535d6d6bea239efb03
python3 tools/check_conventions.py --base 8c8b9986204d5b1cbb23847b363df00ae832ec9c
All conventions checks passed.
gh pr checks 13148 --repo boykopovar/AnyPS5
labels pass 9s
windows pass 15m57s
linux skipping 0
register-context skipping 0
relinker skipping 0
relinker-asan skipping 0
write-watch skipping 0
conventions pass 16s
eligibility pass 7s
report pass 52s
Findings
Blocking
- VulkanDevice.cpp,
DeviceFunctionand the direct marker/upload allocations:DeviceFunctionresolves throughdeviceProc, while the new wrapper is installed incontextDeviceProcat device setup. ThevkAllocateMemory/vkFreeMemorycalls for buffer markers and display uploads useDeviceFunction, so they bypasscountedAllocateMemory/countedFreeMemory. Those allocations are omitted from the live count and report, despite the PR describing per-devicevkAllocateMemory/vkFreeMemoryaccounting. Route these calls through the wrapped resolver (while preserving any existing deferred-destruction wrapper), or explicitly include them in the same accounting path. Add a focused test that exercises these direct allocations and confirms allocation and free totals change.
Non-blocking
None found in the checks above.
Sources
- Counter wrapper installation and Vulkan device-function resolution at this PR head
- Allocation-counter resolver and tracked functions at this PR head
- Counter setup at this PR head
Not verified
I did not run local tests, a build, Linux hosted jobs, specialized hosted jobs, or a game/device-loss reproduction. The author-reported Windows/GPU tests in the PR description were not independently run in this review.
To get this to a good state
Ensure all allocations owned by this Vulkan device flow through the accounting wrapper and add coverage for the marker/upload paths. I will re-run the same non-building checks on the new head and review the focused test result.
Contributor
Re-checked — convention check passed on the exact headThe convention result remains passing; the command in my review did not pin the head, so I reran it explicitly against the PR head. python3 tools/check_conventions.py --base 8c8b9986204d5b1cbb23847b363df00ae832ec9c --head ac751257a9fa8571cd0051964020e97d0e35cf31 |
Collaborator
|
@Prophet069 thanks, the report itself is useful (counts by memory type, size class and caller, the
Also found (reproduced with the exact tracker and fake-driver probes):
|
Contributor
Re-checked — current head, changes requestedThe current head still performs synchronized registry lookups on the device-procedure path and has correctness gaps in allocation tracking and deferred frees. What I ranReviewed exact PR head git diff --check upstream/main...refs/reviews/pr-13148-current
python3 tools/check_conventions.py --base upstream/main --head refs/reviews/pr-13148-current
python3 tools/check_main_sync.py --base upstream/main --head refs/reviews/pr-13148-current
git merge-tree --write-tree upstream/main refs/reviews/pr-13148-current
gh pr checks 13148 --repo boykopovar/AnyPS5Results
Local check outputFindingsBlocking
Non-blocking
Sources
Not verifiedI did not run the reported fake-driver probes, a C++ build, CTest, or a normal-path performance comparison. Linux and specialized hosted checks were skipped. To get this to a good stateAddress the four correctness/performance findings, add the lifecycle and failure-path tests, and report the matched A/B measurement. I will recheck the exact new head and rerun the non-building checks above. |
Contributor
✅ NVIDIA GPU test: passedPR head
Intermittent validation messages (1, seen once in the parallel run, not reproduced when the test was re-run alone 2 times; not counted against this PR)Unstable on main (4, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (2, not counted against this PR)Validated but not compared (1, not in the main baseline's validation pass)Validation IDs already present on main (4 IDs in 425 tests)Skipped (41)ReproduceAutomated build and test on a contributor's NVIDIA machine; it does not replace upstream CI. A new comment is posted for each new push. Full logs. |
adriadam10
requested changes
Oct 30, 2026
adriadam10
left a comment
adriadam10
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for the rebase; the diff is now only this change. Checked against the head ac751257a9, and the points @oneandonlydean and @MarcZX10 raised hold, so this stays at changes:
LiveAllocations.cpp:117-127:countingDeviceProclocks the process-wide registry mutex and does a hash lookup for every name other than the two wrapped ones, on the normal path. Resolve the realvkAllocateMemory/vkFreeMemoryonce, return them without the registry and forward everything else directly. Keep per-allocation bookkeeping cheap (no strings or map nodes) or make it opt-in. Add the alternated A/B on one title (frame p50/p99,DeviceProcLookupsper frame).LiveAllocations.cpp:139-152: the registry entry is never removed, so a second device with a recycledVkDevicevalue throws "set up twice". Erase it on the raw-resolver teardown path and add a two-devices-in-a-row test.LiveAllocations.cpp:59-83: bookkeeping can throwstd::bad_allocafter the driver returnedVK_SUCCESS. Make tracking best effort, mark the statistics incomplete, and test a failure after success.VulkanDevice.cpp:2181: the counter wraps the deferred-destruction resolver, so frees are subtracted whendeferredFreeMemoryqueues them, not when Vulkan frees. Count physical frees (compose the layers the other way) or report pending frees separately, with a composed-layer test.VulkanDevice.cpp:640,:718,:752,:988,:991:DeviceFunctionand the teardown frees go throughdeviceProc, so marker and display-upload allocations bypass the counter. Route them through the wrapper and test them.- State in the report header or description that totals are "since counting started".
The new test only uses a fake resolver; it should also cover the cases above.
12 tasks done
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
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
When the Vulkan device is lost,
[device-fault]gives no hint whether memory pressure was involved. This countsvkAllocateMemoryandvkFreeMemoryper device through a wrapper around the device proc resolver. The device-loss report then prints the live allocations and bytes per memory type, split by size class (<1 MiB … ≥256 MiB). It also lists the eight largest callers: the buffer pool operation, host imports, resident mirrors and the pixel LDS pool, with everything elseunlabelled. A warning is printed when live allocations exceedmaxMemoryAllocationCount. Labels are thread-local RAII scopes, so code without one is unaffected.tests/LiveAllocations.cppchecks counting, size classes, labels, frees and the report text against a fake resolver.Tested
ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation(VVL vulkan-sdk-1.4.321.0; neither log containswas not found but was requested by env var VK_INSTANCE_LAYERS),ctest --test-dir build -R <the 63 tests of agc_shader_memory_tests, agc_driver_cmask_fast_clear_tests, agc_driver_graphics_tests and agc_driver_sample_multisample_target_tests> -Von main8c8b998620and on this branch, same machine and GPU, plus the newagc_live_allocationstest: main 63/63 passed, 0VUID-; this branch 64/64 passed, 0VUID-.agc_driver_unclamped_normalized_blend(the feat(agc): merge a draw into more than one unclamped normalized color target #12916 multi-target regression,2 draws blending two targets each) passed on main and on this branch with the same validation settings, 0VUID-.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