fix(vk): replace menu upload buffers only after successful growth #1453

Merged

Arkex-ai merged 1 commit from fix/menu-upload-growth into main

2026-11-10 05:48:16 +00:00

Arkex-ai commented

2026-11-10 03:21:05 +00:00

(Migrated from github.com)

When an Insert menu, FPS or startup bitmap outgrows a frame slot's upload buffer, copy_over passes the published handles directly to the allocator. Successful growth loses the previous allocation without destroying it; a failed allocation can overwrite those handles while leaving the old capacity, mapping and version metadata.

Construct the replacement locally, release the previous buffer only after allocation succeeds, then publish the new upload with its copy version reset. Failure preserves the old upload and skips the oversized copy. The existing per-slot fence wait before menu_record makes the old buffer safe to release; no extra device-idle wait or memory-budget policy is added.

Validation:

CPU fixture executes the production Bitmap/Upload/copy_over: buffer ownership, clobbered allocator outputs on failure, retry, capacity reuse, current pixel bytes and keyed-run offsets. Both compiled negative controls fail on main; candidate passes. Four compiled mutations are detected.
Linux GCC and ASan/UBSan passed growth and allocation-failure cases.
Current-base local strict suite: candidate189 passed/4 failed/16 skipped versus main188/4/16, with the same four local toolchain/environment failures.
Official integrated host/backend build passed. RTX5070Ti checkpoint smoke exercised menu growth from1720x720 to3440x1440 borderless, the larger bindings page, and repeated windowed/fullscreen reuse, then exited0. This validates integration, not actual driver memory reclamation or genuine GPU allocation failure.

Based on main 9a2e80e. Five source/test/doc files only; no game data, generated guest code or private fault switches. Menu-related PRs were checked for this function; #1389/#1330/#1352 merge cleanly at the fetched heads. This is separate from #1445's interpolation resources and #1448's source-crop validation.

Current-head validation

Current head 7575ed2, based on main b2e1c5a. Earlier local/runtime results describe their recorded source hashes.

The standalone contribution patch is unchanged after the audited main refresh; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash.

Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

When an Insert menu, FPS or startup bitmap outgrows a frame slot's upload buffer, `copy_over` passes the published handles directly to the allocator. Successful growth loses the previous allocation without destroying it; a failed allocation can overwrite those handles while leaving the old capacity, mapping and version metadata. Construct the replacement locally, release the previous buffer only after allocation succeeds, then publish the new upload with its copy version reset. Failure preserves the old upload and skips the oversized copy. The existing per-slot fence wait before `menu_record` makes the old buffer safe to release; no extra device-idle wait or memory-budget policy is added. Validation: - CPU fixture executes the production Bitmap/Upload/copy_over: buffer ownership, clobbered allocator outputs on failure, retry, capacity reuse, current pixel bytes and keyed-run offsets. Both compiled negative controls fail on main; candidate passes. Four compiled mutations are detected. - Linux GCC and ASan/UBSan passed growth and allocation-failure cases. - Current-base local strict suite: candidate189 passed/4 failed/16 skipped versus main188/4/16, with the same four local toolchain/environment failures. - Official integrated host/backend build passed. RTX5070Ti checkpoint smoke exercised menu growth from1720x720 to3440x1440 borderless, the larger bindings page, and repeated windowed/fullscreen reuse, then exited0. This validates integration, not actual driver memory reclamation or genuine GPU allocation failure. Based on main `9a2e80e`. Five source/test/doc files only; no game data, generated guest code or private fault switches. Menu-related PRs were checked for this function; #1389/#1330/#1352 merge cleanly at the fetched heads. This is separate from #1445's interpolation resources and #1448's source-crop validation.  ### Current-head validation Current head `7575ed2`, based on main `b2e1c5a`. Earlier local/runtime results describe their recorded source hashes. The standalone contribution patch is unchanged after the audited main refresh; exact-head build/source checks are refreshed below. Earlier runtime evidence remains tied to its recorded hash. - [Linux AppImage](https://github.com/Arkex-ai/Hookshot/actions/runs/42762679520): failure. - [Windows builder](https://github.com/Arkex-ai/Hookshot/actions/runs/42762676646): success. - [Game-free source regressions](https://github.com/Arkex-ai/Hookshot/actions/runs/42762672910): failure. Fork runs are separate from upstream Actions approval; no maintainer approval gate is bypassed.

hckrman101 (Migrated from github.com) approved these changes

2026-11-10 04:14:41 +00:00

hckrman101 (Migrated from github.com) left a comment

Thanks, this looks right to me.

On main, vmaCreateBuffer writes straight into u.buf/u.alloc, so a successful grow loses the old buffer, and a failed one nulls the handles while u.size stays the old value. The next frame then skips the grow and copies from VK_NULL_HANDLE. Building into a local Upload and swapping only on success fixes both.
In-flight use: I checked present_image in vk_replay.cpp. It waits on frames[frame_index].done before menu_record(..., log_slot), and log_slot == frame_index, so ups[*][slot] only belongs to the submission that fence covers. The early returns (acquire timeout, out of date) come before vkResetFences, so the fence stays signaled and the next wait still holds. Each upload is used once per menu_record, so the command buffer being recorded doesn't reference the old buffer either. Freeing it right away is safe.
next.version = 0 forces the memcpy into the new mapping, which is what you want.
The test is registered in run_all.py with {vulkan_headers}, and it builds from the real copy_over/Upload.

Nit, from before this PR and not blocking: nothing ever destroys ups[3][3] at shutdown. That isn't a problem today, but a small teardown loop would be worth adding if VMA teardown ever starts checking for leaks.

No upstream checks have run yet (fork PR, waiting on Actions approval), so CI on this head is still pending.

Thanks, this looks right to me. - On main, `vmaCreateBuffer` writes straight into `u.buf`/`u.alloc`, so a successful grow loses the old buffer, and a failed one nulls the handles while `u.size` stays the old value. The next frame then skips the grow and copies from `VK_NULL_HANDLE`. Building into a local `Upload` and swapping only on success fixes both. - In-flight use: I checked `present_image` in vk_replay.cpp. It waits on `frames[frame_index].done` before `menu_record(..., log_slot)`, and `log_slot == frame_index`, so `ups[*][slot]` only belongs to the submission that fence covers. The early returns (acquire timeout, out of date) come before `vkResetFences`, so the fence stays signaled and the next wait still holds. Each upload is used once per `menu_record`, so the command buffer being recorded doesn't reference the old buffer either. Freeing it right away is safe. - `next.version = 0` forces the memcpy into the new mapping, which is what you want. - The test is registered in run_all.py with `{vulkan_headers}`, and it builds from the real `copy_over`/`Upload`. Nit, from before this PR and not blocking: nothing ever destroys `ups[3][3]` at shutdown. That isn't a problem today, but a small teardown loop would be worth adding if VMA teardown ever starts checking for leaks. No upstream checks have run yet (fork PR, waiting on Actions approval), so CI on this head is still pending.