Repository navigation
fix(agc): place device memory in a host heap only when that heap has room - #6553
fdfawef324 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.
|
|
Windows data for this PR and #5987: RTX 4060 8 GB (NVIDIA 617.42), i5-14400F, 16 GB RAM, Windows 10 19045. Heap 0 reports a 7189 MiB budget, heap 1 (system memory) 7448 MiB. Minecraft Dungeons II (PPSA16064 01.100.100), first world load, Two of three first loads died, both with
So on Windows the refusal I saw was never of the device-local type; it was the host type, once I can't show this PR's branch catching it: in the third load and in a 3-round A/B from an in-world save (this PR on and off, AI-assisted: yes (Claude Code) |
|
Thank you for the Windows data, @parl0v. Two readings:
If you rerun anything: the first-load path is where #5987 alone died, so an on/off pair there (same AI-assisted: yes |
|
Rebase handoff — rebased onto current Verified on Linux (CachyOS, kernel 7.2.9, GCC 16.2.1, Ninja, Release,
To pick it up: Head is |
Once the device-local heap is over its VK_EXT_memory_budget budget, DeviceMemoryType placed textures, storage images and device buffers in the first allowed host memory type without reading that type's heap, so placements kept growing system memory after the host heap was past its own budget. A host type is now chosen only when its heap fits the allocation, by the test BufferMemoryType already applies to coherent buffers, and a full host heap moves the search on to the next allowed host type. When no allowed host heap has room the device-local type is kept; if the driver refuses that allocation, AllocateImageMemory and the buffer pool still place it in a host type after their existing reclaim steps. The placement line now reports the host heap's usage and budget, and keeping the device-local type is logged at the same rate when an allowed host type exists. Replay the guard onto the merged allocation recovery path and retain the offered rebase's authorship. Exercise both device buffers and images through their current fallback helpers when neither heap fits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Co-Authored-By: blawesom <blaplane@gmail.com> Co-Authored-By: Codex <noreply@openai.com>
56d44ed to
4e8edc8
Compare
|
Thanks, @blawesom. I checked your offered The rebased tests cover the current helpers: when neither heap fits, a successful device allocation stays device-local; a mocked refusal still tries device-local before host memory for both buffers and images. A separate isolated red control, changing only the host-room lookup back to the old host lookup, fails all six budget/mask assertions. The unchanged source passes. The two fresh control executables plus both test runs took 2.81 seconds of bounded child-job time, without mutating the production worktree or its cache identity. Your RX 9070 XT/RADV Linux 1338/1339 result remains attributed to your offered head. It is not a fresh result for this rebased head. The earlier Windows title evidence also stays qualified: @parl0v's A/B did not reach the guard, and this update does not establish a Demon's Souls memory or FPS improvement. The normal Release targets built, and all five selected integrated CPU/mock CTests passed in 11.58 seconds with no skipped cases: memory fallback, buffer pool, its two small-device variants and graphics. Conventions pass; the exact source remains clean with TraceRevision Fresh current-head managed-slot Vulkan batch: The updated PR is a draft while the shared-area landing order is coordinated; the guard itself has no build dependency on those branches. AI-assisted: yes. Co-Authored-By: Codex noreply@openai.com |
What
When a device-local allocation would exceed its reported heap budget,
DeviceMemoryTypecurrently selects the first mask-allowed host type without checking that host heap's budget. This change selects a host type only when its heap has room for the allocation. It shares the existing coherent-buffer fit rule throughContext::HeapFits, searches later allowed host heaps when an earlier one is full, and keeps the device-local type when no host heap fits.Placement diagnostics include the selected host heap's usage and budget. A separate rate-limited line records keeping the device-local type when allowed host types exist but none fit. A mask allowing no host type remains silent.
Rebased onto
fc623324aafter #5987 merged. This PR now contains one guard commit, preserving main'sAllocateImageMemoryrouting, buffer-pool trim, texture-cache reclaim and eventual host fallback after an actual device allocation refusal. It does not restore the supersededAllocateDeviceMemoryhelper. Thanks to @blawesom for the offered321ab70c9rebase; the author and earlier Claude credit are retained, with credit for that recovery work.This is a placement decision, not a hard memory cap. Main's existing refusal path can still allocate in host memory after the device driver refuses a device-local allocation, even when the host budget reports no room. Reports are snapshots and can lag or be shared by concurrent allocations. On Windows, WDDM can also move device-local allocations into system memory. A device buffer kept device-local may remain in the existing bounded pool after release; #4472 separately changes retention under pressure.
Demon's Souls evidence remains a hypothesis. A historical Windows run on
a5e2a80c6with #5870, #6280 and #6412 died about ten seconds after a Windows event 2004 reported 95.65 GiB committed against a 95.9 GiB limit. The app had 32.5 GiB private, and about 45.3 GiB of commit belonged to no process. There was no failing-allocation stack showing this placement path. This PR neither identifies all that memory nor establishes a title memory or FPS improvement.@parl0v's RTX 4060/Windows report identifies a host-type allocation refusal after the host heap filled. Their later on/off A/B peaked below the host budget and never logged the guard; both arms survived 3/3. It supports the failure mode, not proof that this guard prevented the failure. No claim is made to fix #5961's retained storage-image cause.
Nearby work: #4472 moves placement counters and changes pool retention; #7119 adds failure diagnostics. Their current heads already conflict with
fc623324a, including shared buffer-pool tests, and need manual composition. Preserve the host-room guard with #4472's counters and #7119's diagnostics. Proposed order is this guard first; coordination is pending on #4472 and #7119. No combined-build result or merge-order agreement is claimed here.Tested
Exact candidate:
4e8edc80e2646ad7619ffafb3bbb24c3f08949c6, based onfc623324a2ab3443cb69a6d0199141b8b2fee604, Windows Server 2025, MinGW GCC 15.2 Release/O3. Independent source review accepted the three-file change; the current fallback and multisample allocation paths remain unchanged.New CPU/mock coverage checks host room, exact fit, nearly full/over-budget/zero budgets, a second host heap and its type mask. Integration fixtures call the actual
DeviceBufferandAllocateImageMemorypaths: when neither heap fits, success stays device-local; a mocked refusal attempts device-local before host memory.An isolated control copies the exact header and
DeviceMemoryFallback.cpp. With only the host-room lookup changed back to the old host lookup, all six budget/mask assertions fail; the unchanged source passes. Fresh O3 control executables plus both test runs took 2.81 seconds of bounded child-job time. This isolated fixture uses native Windows exception handling and no driver library or GPU; it is separate from the normal project targets and does not measure game speed.The normal Release targets built successfully. The cold build reached its 900-second bound without a compiler error; the remaining 25 incremental steps completed in 86.13 seconds. All five selected CPU/mock CTests passed in 11.58 seconds with no skipped cases:
agc_device_memory_fallback,agc_buffer_pool, its two small-device variants andagc_driver_graphics. Conventions passed against the pinned base. The source remained clean throughout; generated TraceRevision is4e8edc80e264, dirty 0.Fresh current-head managed-slot Vulkan batch:
agc_driver_pooled_buffer_range,agc_driver_sampled_cache_budget,agc_driver_sampled_cache_budget_memoryandagc_driver_storage_cache_budget, 4/4 passed in 9.36 seconds, no skipped cases, withANYPS5_REQUIRE_VULKAN=1. These are real Vulkan adjacency tests, not a real both-heaps-full reproduction. Actual executables and runtime libraries were hash-checked before and after. A clean merge into main64b57d1b0was checked separately; all tests ran on the exact candidate.Historical evidence: @blawesom reported 1338/1339 tests on RX 9070 XT/RADV Linux for their offered
321ab70c9, with the stated main tessellation failure. That is their external result on the offered head, not a local run or a test of this current head. The original56d44ed2cbranch had seven selected cases pass on our NVIDIA machine; those are historical head-specific results, not fresh verification.Not tested: a title on this rebased head; a real device with both heaps unable to fit; Linux locally; full suite. No title memory decrease or FPS gain is claimed.
Checklist
main; no other open PR implements the same functions (host-room guard; nearby perf(agc): stop pooling released device buffers over the memory budget #4472/chore(agc): report heap budgets on buffer allocation failure #7119 composition described above)NotImplemented_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)Co-Authored-By: Claude Opus 5.5 noreply@anthropic.com
Co-Authored-By: blawesom blaplane@gmail.com
Co-Authored-By: Codex noreply@openai.com