Repository navigation
Conversation
… has a device-only type The staging pool keeps released device-only buffers for reuse in its device tier, up to an eighth of the largest device-local heap (512 MiB at least), or up to a sixty-fourth (128 MiB at least) when that heap is system memory. A heap counted as system memory only when every memory type on it is host-visible. MoltenVK on Apple GPUs reports a single device-local heap, the whole system memory, with a device-only type beside the host-visible ones, so the tier took the budget of dedicated video memory: 3,594 MiB on a 36 GB M3 Max. The device-local heap of an integrated GPU is system memory whatever its memory types, so the context records an integrated device and the device tier takes the system-memory budget for it: 449 MiB on that Mac. Discrete GPUs keep their budget, and APS5_STAGING_POOL_MIB still overrides it.
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Reviewed — changes requested
The changed-build evidence includes an unexplained SIGBUS during the Reanimal run, so I cannot approve the cache-eviction change until its relationship to the new behavior is investigated.
What I ran
Reviewed head 24303071a21de966fb22d9fa90962de7aafa1d82 against the PR base 0f302813cebfc09e3e0b6f8fced15262d186879e. The stack check also previewed the head cleanly against live main ca073ec3ad0d766ea9f4658e418a5c977001a353. Environment: Debian Linux 6.1, Python 3.11.2, GCC 12.2.0; no build or CTest run.
python3 tools/pr_precheck.py --repo boykopovar/AnyPS5 --pr 14076 --output-dir /tmp/anyps5-14076-review
git diff --check 0f302813cebfc09e3e0b6f8fced15262d186879e...24303071a21de966fb22d9fa90962de7aafa1d82
python3 tools/pr_stack_check.py 14076 --repo boykopovar/AnyPS5 --output-dir /tmp/anyps5-14076-review/stackResults
| Check | Result |
|---|---|
| Patch and conventions | Passed. |
| Dependency and merge preview | No declared dependencies; clean preview against live main ca073ec3ad0d766ea9f4658e418a5c977001a353. |
| Source and added budget assertions | The change applies the smaller budget to integrated GPUs and adds two corresponding DeviceBudget assertions. I inspected these but did not execute the C++ test. |
| Precheck helper steps | The ABI and test-quality helpers could not launch because those reviewer-only scripts are absent from the isolated PR checkout; no ABI files changed. |
| Author-reported runtime evidence | The PR description reports one changed-build SIGBUS after 61 seconds, during a run with 1,428 misses and 1,547 evictions; none of five comparison runs crashed. The other three changed runs reached the 170-second limit. This is reported evidence, not independently reproduced, and does not establish causation. |
| Hosted CI | Conventions, library portability, intake, labels and report passed; Windows was in progress at the last check; Linux and specialized jobs were skipped. |
Raw outputs
git diff --check produced no output:
Conventions check:
All conventions checks passed.
Precheck helper errors:
/usr/bin/python3: can't open file '/tmp/anyps5-14076-review/head-24303071a21d-3ea03f0d/tools/pr_abi_audit.py': [Errno 2] No such file or directory
/usr/bin/python3: can't open file '/tmp/anyps5-14076-review/head-24303071a21d-3ea03f0d/tools/pr_test_quality.py': [Errno 2] No such file or directory
Stack check output:
/tmp/anyps5-14076-review/stack/stack-report.md
/tmp/anyps5-14076-review/stack/stack-report.json
Hosted checks at the last status query:
windows pending 0 https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613602926
library-portability pass 1m21s https://github.com/boykopovar/AnyPS5/actions/runs/41227149380/job/124613577360
linux skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613603974
register-context skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613604459
relinker skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613604379
relinker-asan skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613604607
write-watch skipping 0 https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613603893
conventions pass 18s https://github.com/boykopovar/AnyPS5/actions/runs/41227149244/job/124613577638
eligibility pass 4s https://github.com/boykopovar/AnyPS5/actions/runs/41227149523/job/124613577900
intake pass 18s https://github.com/boykopovar/AnyPS5/actions/runs/41227149325/job/124613577968
labels pass 6s https://github.com/boykopovar/AnyPS5/actions/runs/41227149332/job/124613578035
report pass 38s https://github.com/boykopovar/AnyPS5/actions/runs/41227149367/job/124613577068
Negative control
| Assertion | Mutation or change | Did the new test fail? |
|---|---|---|
| An integrated GPU with a device-only heap gets the smaller staging budget. | The author reports removing the integratedGpu budget condition. |
Author reports the added assertion fails; I did not run this mutation locally. |
Findings
Blocking
The PR's own comparison reports one SIGBUS in a changed-build Reanimal run and none in five runs without the change. The crash occurred after substantial cache churn (1,428 misses and 1,547 evictions); the cause is unknown, and these counts do not prove the new budget caused it. Because the changed behavior sharply increases eviction frequency, this is a material unresolved stability signal. Please investigate the crash with the available system/crash diagnostics and a repeated matched run, determine whether the new eviction behavior is involved, and fix it if so. If it is unrelated, include reproducible evidence that isolates it from this change. Until then, the runtime behavior is not sufficiently verified for approval.
Non-blocking
The PR includes summary records but says the corresponding traces are available on request. If retaining the frame-time comparison as evidence for the “no cost” statement, please attach the traces and tools/trace_digest.py output for the matched runs so the comparison can be reproduced.
Sources
- Budget calculation at this PR head
- Added integrated-GPU budget assertions at this PR head
- Repository rule for trace summaries
Not verified
No local build, CTest, macOS run, GPU run, or console run was performed. The author-reported SIGBUS and performance measurements were not independently reproduced. Windows CI was still running; Linux and specialized CI jobs were skipped.
To get this to a good state
Investigate the SIGBUS with repeated matched runs and diagnostics, fix any issue tied to the more frequent evictions, and attach trace/digest evidence if retaining the frame-time comparison. I will rerun the same no-build checks on the updated head.
|
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.
|
|
@atinseau Please investigate the reported candidate-only |
|
Thanks for the review. I ran the investigation you asked for; here are the results. The SIGBUS does not depend on the budget. I used one binary: our macOS branch with this commit reverted. I added a local probe to the write watch's fault handler, where an unhandled
Two other runs, one per arm, stopped with Both
In both cases the guest reads its own memory, which the macOS runtime maps without access. Nothing on that path touches the staging pool: its device tier only holds device-only Vulkan buffers, which the CPU never maps. GPU hangs. Two of the seven 449 MiB runs lost the device, at about 110 and 130 s. The system log records a GPU hang caused by a WindowServer command buffer ( Memory. The default arm also shows the problem this PR addresses: in one run the device tier held 2,632 MiB of idle buffers when the title's footprint passed 15 GiB, and the guard stopped it at 123 s. The probe is a local diagnostic and is not part of this PR; its logs are available if useful. |
|
A Windows run on an AMD integrated GPU, since your description says you had none to measure on. It has the shape you describe. AMD Radeon iGPU of a Ryzen 9 7900 ( Memory layout, from What that means for the budget, worked out from
So this PR does change behaviour on this kind of GPU, by a factor of 8. Focused tests, your set of 13 (
The layer was confirmed loaded by a control run of Not tested: a title on this GPU. So this doesn't show whether the smaller tier costs frame time through more buffer churn here, which is the question @oneandonlydean raised for your Mac. AI-assisted: yes (Claude Code) |
✅ NVIDIA GPU test: passedPR head
Unstable on main (1, these also failed in recent main runs on this machine; not counted against this PR)Failing on main too (4, not counted against this PR)Validation IDs already present on main (5 IDs in 476 tests)Skipped (44)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. |
|
Thanks @ycbuzi, that is the case I could not test. It is the one #5893 aimed at: the 10.5 GiB device-local heap of that iGPU is system memory, and |
|
I ran a second series for the GPU hangs of my previous comment. They reproduce, and only with the smaller budget. Setup. One binary (our macOS branch with this commit reverted, plus the local fault probe), the same Reanimal scene (170 s from boot into a new game load, warm caches), 20 runs alternating the default budget (3,594 MiB on this Mac) and
The memory guard stops a run when the title's footprint passes 14 GiB or the system's wired memory passes 22 GiB, so the default runs could not be followed to 170 s. Every hang happened after 100 s, so I compared the time each arm was observed from 100 s on. The default runs were observed for 339 s in total, with no hang. The 449 MiB runs were observed for 365 s, counting each run up to its hang or its end, and lost the device six times. With the first series and the runs in the description, the smaller budget now shows 8 hangs in 20 runs, against 0 in 22 runs with the default one. What the hangs look like. Each one is a GPU recovery. Reanimal's command buffers are discarded as innocent victims, then MoltenVK reports My previous comment left this open. It now goes against this PR: on this Mac, the smaller budget comes with device loss. I do not know the mechanism yet. The candidates are the allocation churn itself, or the way MoltenVK and the Apple GPU handle frequent private allocations near the wired memory limit. I will look into it before asking for another review. The A first attempt of this series crashed at boot in all 20 runs, in both arms, within six minutes. After that the title booted normally again, so those runs are not counted. One 449 MiB run of the counted series also crashed at boot and is left out. |
|
Windows GPU check of head
🤖 Generated with Claude Code |
|
Thanks @JJenkx. The RX 6700 XT is a discrete GPU, so this change leaves its staging pool tier at an eighth of the device-local budget, as on The open point is still the device loss I measured on the Mac with the smaller budget (my comment of 31 October). I'm looking into its cause before asking for another review. |
|
I looked for the cause of the device losses on the Mac. They do not depend on the budget, and my previous comment was wrong to tie them to the smaller one. Why the second series pointed at the budget. Its memory guard stopped a run when the system's wired memory passed 22 GiB. The default runs keep about 2 GiB more in the device tier, so the guard stopped nine of them between 120 and 157 s, the window where every hang fell. They were never observed where the 449 MiB runs hung. Two more series with the guard raised. Same binary (our macOS branch with this commit reverted), same Reanimal scene, 170 s, warm caches, guard at 18 GiB footprint and 26 GiB wired, runs alternating the two budgets. One of them also caps both texture caches with
Three more runs stopped with What loses the device. In every loss, Reanimal's command buffers are discarded as innocent victims of a GPU recovery. The system log names the command buffer that caused it: WindowServer's twice, and twice one of the iOS Simulator's Metal host ( That settles the two open points of the reviews on this Mac: the |
|
@MarcZX10 @oneandonlydean, could you take another look? Both points from your reviews now have an answer:
The head is still |
The requested point is answered: atinseau showed the SIGBUS is independent of this change (comments 6548050915 and 6605046004: 16 alternating runs of the same Reanimal scene with a fault probe; both SIGBUS faults came with the DEFAULT budget, on guest reads of a non-resident region). Head 2430307 is unchanged since that evidence.
What
The staging pool's device tier keeps released device-only buffers for reuse. Since #5893 it keeps a sixty-fourth of the largest device-local heap (at least 128 MiB) when that heap is system memory, and an eighth (at least 512 MiB) otherwise. It takes a heap for system memory when every memory type on it is host-visible, on the ground that a discrete GPU always has a device-only type. MoltenVK on Apple GPUs does not fit that rule: it reports one device-local heap, the whole unified memory, with a device-only type beside the host-visible ones. The tier then gets the budget of a dedicated card, 3,594 MiB of idle buffers on a 36 GB M3 Max.
buildContextnow recordsContext::integratedGpufrom the device type, andBufferPool::DeviceBudgetgives an integrated GPU the system-memory budget whatever its memory types: 449 MiB on that Mac. A discrete GPU keeps the heap check, andAPS5_STAGING_POOL_MIBstill overrides both. An Intel or AMD integrated GPU that reports a device-only type on its device-local heap takes the smaller budget too. ycbuzi measured one: the Radeon iGPU of a Ryzen 9 7900 on Windows has a 10.52 GiB device-local heap with device-only types, so the tier goes from 1.25 GiB to 160 MiB there (see Tested).Tested
macOS 27.0.1, Apple M3 Max (36 GB), MoltenVK 1.4.2 from Vulkan SDK 1.4.363.0, Release build with
-DBUILD_TESTING=ON.maindoes not build its libraries on macOS, so both sides of the tests below aremain4bc9e779awith the macOS port (the squash of #8658, which its author is upstreaming in parts), #13220 and three local build fixes; the change is this commit on top. This branch is that commit rebased onmain0f302813c, whereBufferPool.cpp,BufferPool.hppandstagingPoolBudgetTestsare unchanged.agc_driver_graphics:stagingPoolBudgetTests(mock memory properties, no GPU) gains two cases on a 16 GiB device-local heap with a device-only type: an integrated GPU gets 256 MiB, and 192 MiB under a reported budget of 12 GiB. With theintegratedGpucondition taken out ofDeviceBudgetthe test fails with "an integrated GPU's device-local heap with a device-only memory type was not taken for system memory"; with it, it passes.Focused tests on both sides with
VK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1(ctest --test-dir build-macos -R '<pattern>' -Voveragc_buffer_pool*,agc_device_memory_fallback,agc_driver_graphics,agc_driver_pooled_buffer_range,agc_driver_multisample_retirement,agc_driver_host_import_failure_handles,agc_storage_write_back,agc_driver_sampled_cache_budget*andagc_driver_storage_cache_budget, 13 tests): neither log reports the layer missing, every test keeps its status (12 pass,agc_driver_multisample_retirementfails on both sides) and each test has the sameVUID-errors on both sides (8 in all,VUID-VkDeviceCreateInfo-pProperties-04451, from MoltenVK's portability subset).Reanimal (PPSA23372), the
device:part of the[bufferpool]line (APS5_PROFILE_DRAW=1): at boot the tier's budget reads 3594 MiB on the base and 449 MiB with the change. Past that, both stop at the title's first 64-bit image atomic, whichmaincannot run on MoltenVK yet.Reanimal on our local macOS branch (the bring-up described in fix(relinker): keep moved instructions off a function entry after padding #11413), the same build with and without this commit, warm caches, 170 s from boot through the main menu into a new game load:
The change keeps about 400 MiB less of idle buffers in this window. The cost is the one perf(agc): keep a smaller staging pool on system-memory heaps #5893 measured on its Arc iGPU: the title's working set of these buffers is larger than the budget, so the tier misses and evicts about ten times a second (misses and evictions are those of the three runs with the change that reached 170 s). One of the four runs with the change stopped with
SIGBUS61 s in; none of the five without it did. The investigation shows that this fault does not depend on the budget. The investigation ran 14 runs on one binary, alternating the default budget andAPS5_STAGING_POOL_MIB=449. BothSIGBUSin them came with the default budget, and both are a guest read of guest memory that the macOS runtime maps without access, a path that does not reach the staging pool. The same 14 runs had two GPU hangs, both with the 449 MiB budget. Two later series, with the memory guard raised so the default runs could be followed to 170 s, show that these hangs do not depend on the budget either: the device was lost in 3 of 7 runs with the default budget and 3 of 8 with 449 MiB, and for each loss the system log names a command buffer of WindowServer or of the iOS Simulator's Metal host as the cause, or none, with Reanimal's command buffers discarded as innocent victims.Frame time, Reanimal's main menu, 20 s captured with
APS5_TRACE_SECONDS=45:20after the menu came up, two runs of each build, alternated: frame p50 252.0 and 242.1 ms without the change, 242.2 and 247.4 ms with it, GPU busy 172 to 181 ms per present in all four. The tier is at its budget and evicting in this scene; the frame rate is bound by the GPU at about 4 FPS, so these runs show no cost at this frame rate, nothing finer. Summary lines below; the traces are available on request.AMD Radeon iGPU of a Ryzen 9 7900 (
1002:164e), driver 25.10.30.02, Windows 11, run by ycbuzi on this head and its parent: the 13 focused tests above, withVK_LAYER_KHRONOS_validation,ANYPS5_REQUIRE_VULKAN=1andANYPS5_NO_SHADER_CACHE=1, pass 13 of 13 on both sides with noVUID-error. The layer was confirmed loaded by a control test. No title was run there.NVIDIA RTX 2000 Ada, driver 595.99.02, Ubuntu 26.04.1, NVIDIA GPU test on this head merged onto
main500d37a: the build passes, and of the 2,444 tests the change can reach, 2,395 pass with no new failure and no new validation ID againstmain.python3 tools/check_conventions.py --base origin/mainpasses.tools/pr_overlap.pycould not compare 42 open pull requests that conflict withmainand share a file with this one (VulkanDevice.cpp,Context.hpp,tests/Graphics.cpp); reading their diffs, only perf(agc): stop pooling released device buffers over the memory budget #4472 touches the same code. It adds the sameContext::integratedGpufield, set the same way inbuildContext, so that its VRAM budget leaves integrated GPUs alone; whichever of the two merges second keeps the other's field and drops its own two lines.Not tested: Intel integrated GPUs, and a title on Linux, on Windows or on the AMD integrated GPU. On discrete GPUs the path is unchanged, and
stagingPoolBudgetTestskeeps its cases for them.Summary lines, main menu, in run order: commit 42411a4d0 is our branch with this commit reverted, caa2308d0 the same branch with it
{"commit":"42411a4d0019","dirty":false,"env":{"APS5_LOG_TIMESTAMPS":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"45:20"},"gpu":"Apple M3 Max","driver":"MoltenVK 1.4.2","os":"Darwin 27.0.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":14,"memory_mib":36864,"title_id":"PPSA23372","title":"REANIMAL","content_version":"01.000.006","resolution":"3840x2160","window":{"spec":"seconds 45:20","first_present":204,"ms":20040.125},"presents":79,"frame_ms":{"p50":252.019,"p90":261.268,"p99":291.679,"max":291.679},"gpu_busy_ms_per_present":181.048,"worker_busy_pct":{"queue worker 0x0":99.169,"queue worker 0x20":20.511},"top_phases_ms_per_frame":{"Driver.Packet":298.309,"Driver.Packet.dispatch":205.177,"Driver.Dispatch":176.660,"Vulkan.Dispatch":159.855,"Recorder.Sync":159.282,"Vulkan.Dispatch.shader_resources":159.115,"Driver.Packet.memory_wait":40.810,"Recorder.Submit":28.232},"trace_dropped":false,"trace":"aps5-trace-42411a4d0019-33550.json.gz"} {"commit":"42411a4d0019","dirty":false,"env":{"APS5_LOG_TIMESTAMPS":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"45:20"},"gpu":"Apple M3 Max","driver":"MoltenVK 1.4.2","os":"Darwin 27.0.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":14,"memory_mib":36864,"title_id":"PPSA23372","title":"REANIMAL","content_version":"01.000.006","resolution":"3840x2160","window":{"spec":"seconds 45:20","first_present":205,"ms":20020.089},"presents":82,"frame_ms":{"p50":242.068,"p90":247.660,"p99":290.461,"max":290.461},"gpu_busy_ms_per_present":172.131,"worker_busy_pct":{"queue worker 0x0":99.131,"queue worker 0x20":20.402},"top_phases_ms_per_frame":{"Driver.Packet":286.974,"Driver.Packet.dispatch":199.308,"Driver.Dispatch":171.462,"Vulkan.Dispatch":155.309,"Vulkan.Dispatch.shader_resources":154.549,"Recorder.Sync":149.681,"Driver.Packet.memory_wait":36.980,"Recorder.Submit":28.028},"trace_dropped":false,"trace":"aps5-trace-42411a4d0019-35890.json.gz"} {"commit":"caa2308d0b7a","dirty":false,"env":{"APS5_LOG_TIMESTAMPS":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"45:20"},"gpu":"Apple M3 Max","driver":"MoltenVK 1.4.2","os":"Darwin 27.0.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":14,"memory_mib":36864,"title_id":"PPSA23372","title":"REANIMAL","content_version":"01.000.006","resolution":"3840x2160","window":{"spec":"seconds 45:20","first_present":212,"ms":20160.226},"presents":83,"frame_ms":{"p50":242.170,"p90":245.502,"p99":276.725,"max":276.725},"gpu_busy_ms_per_present":174.090,"worker_busy_pct":{"queue worker 0x0":99.170,"queue worker 0x20":19.760},"top_phases_ms_per_frame":{"Driver.Packet":283.933,"Driver.Packet.dispatch":191.668,"Driver.Dispatch":160.661,"Recorder.Sync":150.937,"Vulkan.Dispatch":145.253,"Vulkan.Dispatch.shader_resources":144.498,"Driver.Packet.memory_wait":35.229,"Recorder.Submit":29.226},"trace_dropped":false,"trace":"aps5-trace-caa2308d0b7a-34772.json.gz"} {"commit":"caa2308d0b7a","dirty":false,"env":{"APS5_LOG_TIMESTAMPS":"1","APS5_PAD_INPUT_FILE":"<path>","APS5_TRACE_DIR":"<path>","APS5_TRACE_SECONDS":"45:20"},"gpu":"Apple M3 Max","driver":"MoltenVK 1.4.2","os":"Darwin 27.0.0","cpu":"VirtualApple @ 2.50GHz","cpu_threads":14,"memory_mib":36864,"title_id":"PPSA23372","title":"REANIMAL","content_version":"01.000.006","resolution":"3840x2160","window":{"spec":"seconds 45:20","first_present":211,"ms":20196.191},"presents":81,"frame_ms":{"p50":247.408,"p90":253.241,"p99":307.403,"max":307.403},"gpu_busy_ms_per_present":175.321,"worker_busy_pct":{"queue worker 0x0":99.137,"queue worker 0x20":20.481},"top_phases_ms_per_frame":{"Driver.Packet":292.565,"Driver.Packet.dispatch":200.276,"Driver.Dispatch":169.883,"Vulkan.Dispatch":153.607,"Recorder.Sync":153.304,"Vulkan.Dispatch.shader_resources":152.823,"Driver.Packet.memory_wait":39.772,"Recorder.Submit":29.083},"trace_dropped":false,"trace":"aps5-trace-caa2308d0b7a-37031.json.gz"}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)Context
Found in the Reanimal bring-up on macOS that #11413 describes. On 2026-10-24, on an older form of our local branch, a memory guard stopped Reanimal's new game load at 175 s with 2,481 MiB of idle buffers in this tier (budget 3,594 MiB); with this commit the tier stayed at its 449 MiB budget and the same load ran until the guard stopped it at 441 s, for other memory. The load does not finish on this Mac yet, with or without the change.