Skip to content

fix(vulkan): release modules from rejected cache records - #1082

Merged
hckrman101 merged 1 commit into
NotRexed:mainfrom
Trycky64:resubmit/vulkan-cache-module-cleanup
Nov 9, 2026
Merged

hckrman101 merged 1 commit into
NotRexed:mainfrom
Trycky64:resubmit/vulkan-cache-module-cleanup

Conversation

@Trycky64

@Trycky64 Trycky64 commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

Update

Rebased this change onto origin/main at 8b2e1d3b895a5f890dcb886d904b62d6b38df2c7, preserving the current indexed cache reader and record loading path.

When a later stage in a cache record fails, earlier VkShaderModule handles are now destroyed and reset before the record is discarded. The cache format is unchanged. The regression fixture covers failure on the first or later module, malformed and truncated records, valid records, and independent records continuing to load.

Validation

  • Windows and Linux targeted runner: cache cleanup, cache reader, and cache index checks passed (3 passed, 0 failed on each OS).
  • The six synthetic cache cleanup cases passed with AddressSanitizer and UndefinedBehaviorSanitizer on Linux.
  • Full Windows hookshot_vk.dll build passed: 278 seconds, zero compilation failures.
  • The full strict runner on this branch reported 116 passed, 7 failed, 12 skipped. A clean origin/main run reported 115 passed, 7 failed, 12 skipped. The same seven checks failed on both: nx2/test_nup_verify.py, nx2/test_nup_codec.py, nx2/test_mesh_first_types.py, nx2/test_nup_paths.py, tests/test_fps_overlay_order.py, tests/test_precompile_compute_guard.py, and tests/test_program_answer_domains.py. The added regression test passed; no additional failure was observed.

No game data or in-game test was used. No real-GPU validation was performed after the rebase.

This resubmission replaces PR #720, following the maintainer's request to rebase and reopen it. GitHub refused to reopen the closed original PR, so this contribution is submitted as a new independent PR.

@hckrman101 hckrman101 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. The cleanup is correct: a rejected record's Program is local to program_cache_read and never reaches variants, so only modules created in that record get destroyed, each once (make_module returns null on failure, and the handle is reset afterwards). The records that are rejected before any stage is built (bad head, size mismatch) never create modules, so nothing leaks there. It's also fine for the worker threads in program_cache_load_some, since each worker only touches its own made[i]. No module is shared with an accepted program or with a pipeline being built.

The test goes through the real loader with mocked Vulkan and checks every handle is destroyed exactly once. It's a .py file so run_all.py picks it up without an ARGS entry.

Nits, none blocking:

  • The branch is based on an older main. #1051 (put_head/get_head in the read path) has since merged, and the PR still reports mergeable, but the test builds by slicing vk_shader.cpp between text markers, so worth a rebase and a rerun to make sure it still compiles against current main. No CI ran on this PR.
  • The same destroy loop already exists in a few other places in this file; a small helper would be nice later, not needed here.

Comment thread gpu/vk/vk_shader.cpp
if (!ok) {
static std::atomic<int> logged{0};
// Program owns the stage containers, but their Vulkan handles are not RAII-managed.
for (Stage& st : p->stages) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: same loop is already written out at the program teardown sites in this file; a tiny destroy_stage_modules(Program&) helper could be shared later. Fine as is.

@hckrman101
hckrman101 merged commit 5d1fe99 into NotRexed:main Nov 9, 2026
bronze-ff added a commit to bronze-ff/Hookshot that referenced this pull request Nov 9, 2026
…cache_load

The check NotRexed#1082 added (2838e89) lifts program_cache_load from
gpu/vk/vk_shader.cpp and compiles it against its own stand-ins. That
function has called PROF_EXACT() since f226108, which the fixture does not
define, so the check stops at compile time on main: "'PROF_EXACT' was not
declared in this scope". The fixture now defines it as a no-op beside its
other stand-ins.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants