fix(vk): one pipeline play file a session, and the next start puts them together

NotRexed commented

2026-11-12 03:10:46 +00:00

(Migrated from github.com)

The bug (#2579). Every save in play (every 32 new pipelines) wrote its own mkw_vk_pipelines_play_<n>.bin. The files are folded into the main pipeline cache only once a start finds 16 of them, so a long session can leave more than that: the reporter had 22, and the owner's test folder had 13.

The fix (gpu/vk/vk_draw.cpp):

During a session: each save merges the generation it takes into one session cache and writes that again, to one file with the session's own number. A write that fails is kept and tried again at the next save, even with nothing new.
At start: the play files found are merged into pcache as before, and also put together into one play file in their place. At 16 they are still written into the main cache, as before.

So the folder holds at most the last start's combined file plus the current session's. The 2.5 GB main cache is still not rewritten during play.

Tests: tests/test_pipeline_play_cache.py and its C++ model are updated:

one file per session that grows with each save;
a failed write retried at the next save;
the start putting two play files together into one, numbered after the highest.

All 9 cases pass, and test_pipeline_cache_save passes.

Fixes #2579

🤖 Generated with Claude Code

**The bug (#2579).** Every save in play (every 32 new pipelines) wrote its own `mkw_vk_pipelines_play_<n>.bin`. The files are folded into the main pipeline cache only once a start finds 16 of them, so a long session can leave more than that: the reporter had 22, and the owner's test folder had 13. **The fix** (`gpu/vk/vk_draw.cpp`): - **During a session:** each save merges the generation it takes into one session cache and writes that again, to one file with the session's own number. A write that fails is kept and tried again at the next save, even with nothing new. - **At start:** the play files found are merged into pcache as before, and also put together into one play file in their place. At 16 they are still written into the main cache, as before. So the folder holds at most the last start's combined file plus the current session's. The 2.5 GB main cache is still not rewritten during play. **Tests:** `tests/test_pipeline_play_cache.py` and its C++ model are updated: - one file per session that grows with each save; - a failed write retried at the next save; - the start putting two play files together into one, numbered after the highest. All 9 cases pass, and `test_pipeline_cache_save` passes. Fixes #2579 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Arkex-ai commented

2026-11-12 04:06:47 +00:00

(Migrated from github.com)

I am handling the separate file-backed I/O scope in #2623: remove whole-cache temporary heap vectors from base/play reads and writes, preserving the current policy. Your one-file-per-session and startup-consolidation behavior stays in this PR.

I reviewed 1b644a20: it still goes through pcache_file_read and play_file_write, so the storage helper can remain underneath your changes. The extraction fixtures overlap too; whichever lands second should retain both the session/retry assertions here and the native I/O failure/actual-length checks there.

The file-backed candidate has passed a local three-process load/save/reload cycle on Windows, including a forced base save and a fresh read of that output. Its PR is not open yet; I will link it when the current full suites and final overlap audit finish.

I am handling the separate file-backed I/O scope in #2623: remove whole-cache temporary heap vectors from base/play reads and writes, preserving the current policy. Your one-file-per-session and startup-consolidation behavior stays in this PR. I reviewed `1b644a20`: it still goes through `pcache_file_read` and `play_file_write`, so the storage helper can remain underneath your changes. The extraction fixtures overlap too; whichever lands second should retain both the session/retry assertions here and the native I/O failure/actual-length checks there. The file-backed candidate has passed a local three-process load/save/reload cycle on Windows, including a forced base save and a fresh read of that output. Its PR is not open yet; I will link it when the current full suites and final overlap audit finish.

github-sync referenced this pull request

2026-11-12 11:36:09 +00:00

perf(vk): persist pipeline caches without full-size heap copies #2649

github-sync commented

2026-11-12 14:43:32 +00:00

Owner

@Joey-Magnus reviewed on GitHub · 2026-11-12 14:33 UTC · original

I merged this into current main (it merges cleanly) and ran tests/run_all.py on Linux with the Vulkan headers. 457 checks pass. The only failures are test_lift_cache, which fails on main here, and a test_recorder_sync timeout under load that passes when run alone. test_pipeline_play_cache and test_pipeline_cache_save pass. The red CI is main's own breakage from that morning, so a re-run should clear it. Three things to know, none blocking:

Each save now rewrites the whole session file, every 32 new pipelines. The disk writes over a session grow with the square of what it compiled, where before each file held only its own generation. It happens on pipeline_cache_save's detached thread, so it costs no frames. Still, a long first visit could be worth a cap: start a new session file past some size.
The session cache keeps every generation's data in memory until exit. Before, a generation was destroyed once it was written.
If vkMergePipelineCaches into the session cache fails, that generation stays in unsaved. The per-file fallback loop only runs when there is no session cache, so that generation is never written.
This overlaps #2649, which rewrites play_file_write and pipeline_play_cache_test.cpp, and #2456. Whichever lands second keeps both sets of assertions, as Arkex-ai said.

> [@Joey-Magnus](https://github.com/Joey-Magnus) reviewed on GitHub · 2026-11-12 14:33 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2625#pullrequestreview-5876223575) I merged this into current main (it merges cleanly) and ran tests/run_all.py on Linux with the Vulkan headers. 457 checks pass. The only failures are test_lift_cache, which fails on main here, and a test_recorder_sync timeout under load that passes when run alone. test_pipeline_play_cache and test_pipeline_cache_save pass. The red CI is main's own breakage from that morning, so a re-run should clear it. Three things to know, none blocking: - Each save now rewrites the whole session file, every 32 new pipelines. The disk writes over a session grow with the square of what it compiled, where before each file held only its own generation. It happens on pipeline_cache_save's detached thread, so it costs no frames. Still, a long first visit could be worth a cap: start a new session file past some size. - The session cache keeps every generation's data in memory until exit. Before, a generation was destroyed once it was written. - If vkMergePipelineCaches into the session cache fails, that generation stays in unsaved. The per-file fallback loop only runs when there is no session cache, so that generation is never written. This overlaps #2649, which rewrites play_file_write and pipeline_play_cache_test.cpp, and #2456. Whichever lands second keeps both sets of assertions, as Arkex-ai said.

github-sync commented

2026-11-12 16:55:23 +00:00

Owner

@hckrman101 commented on gpu/vk/vk_draw.cpp line 829 on GitHub · 2026-11-12 16:34 UTC · original

With this branch, a start finds at most two play files (the last combined file and the last session's), so the >= kPlayFilesKept branch above can't be reached any more, and the play files never go into mkw_vk_pipelines.bin. The combined file then grows with every session and is rewritten whole here at every start. Could the fold trigger on size instead (e.g. merged_kb past 64 MB, like the in-play threshold), so it still fires?

> [@hckrman101](https://github.com/hckrman101) commented on `gpu/vk/vk_draw.cpp` line 829 on GitHub · 2026-11-12 16:34 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2625#discussion_r4553300591) With this branch, a start finds at most two play files (the last combined file and the last session's), so the `>= kPlayFilesKept` branch above can't be reached any more, and the play files never go into `mkw_vk_pipelines.bin`. The combined file then grows with every session and is rewritten whole here at every start. Could the fold trigger on size instead (e.g. `merged_kb` past 64 MB, like the in-play threshold), so it still fires?

github-sync commented

2026-11-12 16:55:23 +00:00

Owner

@hckrman101 commented on gpu/vk/vk_draw.cpp line 898 on GitHub · 2026-11-12 16:34 UTC · original

Small, and the old per-save numbers had the same issue: a second instance started from the same folder takes the same play_seq + 1, so the two sessions overwrite each other's file (and share its .tmp). If it's cheap, picking the first number whose file doesn't exist yet (or creating the .tmp exclusively) would keep two sessions apart. A nit, not a blocker.

> [@hckrman101](https://github.com/hckrman101) commented on `gpu/vk/vk_draw.cpp` line 898 on GitHub · 2026-11-12 16:34 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2625#discussion_r4553300608) Small, and the old per-save numbers had the same issue: a second instance started from the same folder takes the same `play_seq + 1`, so the two sessions overwrite each other's file (and share its `.tmp`). If it's cheap, picking the first number whose file doesn't exist yet (or creating the `.tmp` exclusively) would keep two sessions apart. A nit, not a blocker.

github-sync commented

2026-11-12 16:55:23 +00:00

Owner

@hckrman101 reviewed on GitHub · 2026-11-12 16:34 UTC · original

Thanks, this goes at the right thing: one file per session, rewritten through the same .tmp + rename, is a clear improvement over a new file per save, and the tests follow the new behavior well.

Crash safety looks fine. Every write still goes through .tmp and a rename, so killing the game mid-session leaves the session file as of its last good save (only the generation since then is lost, as before). A kill at start between writing the combined file and removing the originals just leaves duplicates, which the next start merges again. Two instances at once: both take play_seq + 1 for their session and share one .tmp name, so they overwrite each other's session file. That was already true of the per-save numbers before this PR, so I wouldn't block on it, but see the inline note for a cheap guard.

One real problem: the fold into mkw_vk_pipelines.bin can no longer happen (inline at the start merge). After this change the folder holds at most two play files at any start, so merged.size() >= kPlayFilesKept is never reached again, except for folders that already have 16 or more. The combined play file then keeps every session's pipelines forever. It is read, merged, held in memory and rewritten whole at every start, and it grows without limit. That is the same pattern of rewriting a big file again and again that the play files were added to avoid, only moved to startup. It also puts all play data in one file, so a single bad write loses all of it. The 16-file trigger needs a replacement that still fires. I'd go by size: fold into the main cache when the play files merged at a start total more than some threshold, for example the 64 MB that RX_VK_PLAY_SAVE_START_MB already uses in play. A test that goes through several sessions and still ends with the main cache written would pin this down.

Joey-Magnus's three points still stand and I agree with all of them: the session file is rewritten whole at each save (writes grow with the square of the session), the session cache stays in memory until exit, and a generation whose merge into the session cache fails is never written because the per-file fallback only runs without a session cache. The last one is a small fix: on a failed merge, write that generation to a file of its own as before.

Evidence: so far there are only the model tests. For a fix, our rule is that it has to be shown gone in the game. Please add the log lines and a folder listing from three or more starts in a row, showing the play file count staying at one or two and the fold into the main cache happening. Because of the rewrite-per-save cost, please also add the session file's size and save times over a long first visit (from the existing play file %u saved lines), with the GPU named.

CI: all five failures come from main, not this PR. The base commit's own push run failed the same way: the AppImage job fails on a std::min type mismatch in vk_texture.cpp, and the source checks fail on unrelated tests (audio, dlss, env_on, wayland, etc.). test_pipeline_play_cache and test_pipeline_cache_save pass on both OSes. A merge of current main should turn it green. It merges cleanly, and main hasn't touched these files since.

Overlaps: #2649 (pipeline cache persistence via mappings) and #2456 (Intel saves: play_file_write signature, the drain wait in play_cache_save) both rewrite the same functions and pipeline_play_cache_test.cpp, so expect textual conflicts. #2649 is still a draft with open Linux issues, so this one should land first and #2649 rebase onto it. One thing to watch when #2649 rebases: with its unique temp names, each interrupted rewrite of a growing session file leaves a full-size orphan, so its temp sweep at start matters more after this PR.

> [@hckrman101](https://github.com/hckrman101) reviewed on GitHub · 2026-11-12 16:34 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2625#pullrequestreview-5878229918) Thanks, this goes at the right thing: one file per session, rewritten through the same `.tmp` + rename, is a clear improvement over a new file per save, and the tests follow the new behavior well. **Crash safety looks fine.** Every write still goes through `.tmp` and a rename, so killing the game mid-session leaves the session file as of its last good save (only the generation since then is lost, as before). A kill at start between writing the combined file and removing the originals just leaves duplicates, which the next start merges again. Two instances at once: both take `play_seq + 1` for their session and share one `.tmp` name, so they overwrite each other's session file. That was already true of the per-save numbers before this PR, so I wouldn't block on it, but see the inline note for a cheap guard. **One real problem: the fold into `mkw_vk_pipelines.bin` can no longer happen** (inline at the start merge). After this change the folder holds at most two play files at any start, so `merged.size() >= kPlayFilesKept` is never reached again, except for folders that already have 16 or more. The combined play file then keeps every session's pipelines forever. It is read, merged, held in memory and rewritten whole at every start, and it grows without limit. That is the same pattern of rewriting a big file again and again that the play files were added to avoid, only moved to startup. It also puts all play data in one file, so a single bad write loses all of it. The 16-file trigger needs a replacement that still fires. I'd go by size: fold into the main cache when the play files merged at a start total more than some threshold, for example the 64 MB that `RX_VK_PLAY_SAVE_START_MB` already uses in play. A test that goes through several sessions and still ends with the main cache written would pin this down. Joey-Magnus's three points still stand and I agree with all of them: the session file is rewritten whole at each save (writes grow with the square of the session), the session cache stays in memory until exit, and a generation whose merge into the session cache fails is never written because the per-file fallback only runs without a session cache. The last one is a small fix: on a failed merge, write that generation to a file of its own as before. **Evidence:** so far there are only the model tests. For a fix, our rule is that it has to be shown gone in the game. Please add the log lines and a folder listing from three or more starts in a row, showing the play file count staying at one or two and the fold into the main cache happening. Because of the rewrite-per-save cost, please also add the session file's size and save times over a long first visit (from the existing `play file %u saved` lines), with the GPU named. **CI:** all five failures come from main, not this PR. The base commit's own push run failed the same way: the AppImage job fails on a `std::min` type mismatch in `vk_texture.cpp`, and the source checks fail on unrelated tests (audio, dlss, env_on, wayland, etc.). `test_pipeline_play_cache` and `test_pipeline_cache_save` pass on both OSes. A merge of current main should turn it green. It merges cleanly, and main hasn't touched these files since. **Overlaps:** #2649 (pipeline cache persistence via mappings) and #2456 (Intel saves: `play_file_write` signature, the drain wait in `play_cache_save`) both rewrite the same functions and `pipeline_play_cache_test.cpp`, so expect textual conflicts. #2649 is still a draft with open Linux issues, so this one should land first and #2649 rebase onto it. One thing to watch when #2649 rebases: with its unique temp names, each interrupted rewrite of a growing session file leaves a full-size orphan, so its temp sweep at start matters more after this PR.

github-sync referenced this pull request

2026-11-12 16:55:30 +00:00

perf(vk): persist pipeline caches without full-size heap copies #2649

github-sync referenced this pull request from a commit

2026-11-12 19:00:50 +00:00

perf(vk): persist pipeline caches without full-size heap copies (#2649)

Rows
Columns

fix(vk): one pipeline play file a session, and the next start puts them together #2625