MagicBeanDev/Hookshot

Archived

Watch

Fork 0

perf(vk): a command buffer too short to split skips the dry pass #2781

Closed

github-sync wants to merge 1 commit from github/pr/2781 into main

github-sync commented

2026-11-12 13:38:09 +00:00

Owner

@Revens2 opened this pull request on GitHub · 2026-11-12 12:47 UTC · original
Merged on GitHub in 685133b3c0

Why

With parallel replay on (RX_VK_PARALLEL), every command buffer first paid a dry pass - the whole stream applied for state only - and then its one segment was recorded on the replay thread anyway: the same stream walked twice for nothing. A submission of a few hundred draws is one segment, and on a title that submits six a frame that was most of them. This removes that second walk for those short command buffers, a recording cost on the CPU side of the replay path.

Issue: #2278 - FPS Drops to 25-30 In-Game but Reaches 60 FPS in Menu (#2278). The report is collected in #995; this change addresses the replay path's share of it, with the measurement still to come.

The default path is untouched: with RX_VK_PARALLEL off (the default), every command buffer is recorded exactly as before. This change only affects players who turned parallel replay on.

What changed

gpu/vk/vk_replay.cpp:
- op_is_work is the draw/dispatch test the dry pass already counted dry_work with, now shared.
- stream_work counts a stream's draws and dispatches from its op headers without applying anything, stopping at the count asked.
- replay_handle counts the stream before the dry pass. When it holds fewer than two segments (2 * RX_VK_PARALLEL_SEG, default 320 draws and dispatches), it records the command buffer once on the replay thread, as parallel replay off does, and skips the dry pass. RX_VK_NO_DRY_SHORT=1 brings the old behavior back.
- split_off (thread-local) keeps maybe_split from chunking that recording: a segment is not split.
- The 240-buffer parallel log line now also reports how many short command buffers were recorded this round.
tests/test_parallel_small_cb.py (new): checks the source wiring (the op test, the count before the dry pass, the serial recording, the switch, maybe_split) and compiles stream_work against a byte stream: counts draws and dispatches, stops at the count asked, ignores ops past NOP_END, and returns 0 for a state-only stream.

Testing

Game-free, Windows, llvm-mingw, in the worktree:

python -I -B tests/run_all.py: 462 passed, 1 failed, 33 skipped.
Failed: tests/test_input_background.py, a known environment failure declared in this repo's config on this PC (no MSVC SDK); it fails the same way on an untouched upstream main, proven with clang 23.1.5 (-Wunused-but-set-global).
The two other declared environment failures, tests/test_precompile_compute_guard.py and tests/test_recorder_sync.py, passed in this run.
Focused: test_parallel_small_cb passes. The verification run also showed the new check fails on upstream ee29dd55 for the missing stream_work/op_is_work wiring and passes on the branch head.
Not tested: a real game run (see below), and a non-Windows build; the new check is source-based, so CI runs it on both.

GPU/driver/area: NVIDIA GeForce RTX 5070 12 GB, driver 32.0.16.2114 (Vulkan 1.4), Windows 11 build 26200, i5-14600KF. Zone: Vulkan command-buffer replay (gpu/vk/vk_replay.cpp), the RX_VK_PARALLEL=1 path only.

in-game A/B: pending, results will be added in a comment

AI assistance

Assisted-by: oc-impl-2 (opencode/deepseek-v4.1-flash), oc-upstream-1 (opencode/deepseek-v4.1-flash-free)

> [@Revens2](https://github.com/Revens2) opened this pull request on GitHub · 2026-11-12 12:47 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2781) > **Merged on GitHub** in https://git.magicbean.dev/MagicBeanDev/Hookshot/commit/685133b3c03424f527ef1ce07bbeb2cde1fdcb98 ## Why With parallel replay on (`RX_VK_PARALLEL`), every command buffer first paid a dry pass - the whole stream applied for state only - and then its one segment was recorded on the replay thread anyway: the same stream walked twice for nothing. A submission of a few hundred draws is one segment, and on a title that submits six a frame that was most of them. This removes that second walk for those short command buffers, a recording cost on the CPU side of the replay path. Issue: #2278 - FPS Drops to 25-30 In-Game but Reaches 60 FPS in Menu (https://git.magicbean.dev/MagicBeanDev/Hookshot/issues/2278). The report is collected in #995; this change addresses the replay path's share of it, with the measurement still to come. The default path is untouched: with `RX_VK_PARALLEL` off (the default), every command buffer is recorded exactly as before. This change only affects players who turned parallel replay on. ## What changed - `gpu/vk/vk_replay.cpp`: - `op_is_work` is the draw/dispatch test the dry pass already counted `dry_work` with, now shared. - `stream_work` counts a stream's draws and dispatches from its op headers without applying anything, stopping at the count asked. - `replay_handle` counts the stream before the dry pass. When it holds fewer than two segments (`2 * RX_VK_PARALLEL_SEG`, default 320 draws and dispatches), it records the command buffer once on the replay thread, as parallel replay off does, and skips the dry pass. `RX_VK_NO_DRY_SHORT=1` brings the old behavior back. - `split_off` (thread-local) keeps `maybe_split` from chunking that recording: a segment is not split. - The 240-buffer parallel log line now also reports how many short command buffers were recorded this round. - `tests/test_parallel_small_cb.py` (new): checks the source wiring (the op test, the count before the dry pass, the serial recording, the switch, `maybe_split`) and compiles `stream_work` against a byte stream: counts draws and dispatches, stops at the count asked, ignores ops past `NOP_END`, and returns 0 for a state-only stream. ## Testing Game-free, Windows, llvm-mingw, in the worktree: - `python -I -B tests/run_all.py`: 462 passed, 1 failed, 33 skipped. - Failed: `tests/test_input_background.py`, a known environment failure declared in this repo's config on this PC (no MSVC SDK); it fails the same way on an untouched upstream main, proven with clang 23.1.5 (`-Wunused-but-set-global`). - The two other declared environment failures, `tests/test_precompile_compute_guard.py` and `tests/test_recorder_sync.py`, passed in this run. - Focused: `test_parallel_small_cb` passes. The verification run also showed the new check fails on upstream `ee29dd55` for the missing `stream_work`/`op_is_work` wiring and passes on the branch head. - Not tested: a real game run (see below), and a non-Windows build; the new check is source-based, so CI runs it on both. GPU/driver/area: NVIDIA GeForce RTX 5070 12 GB, driver 32.0.16.2114 (Vulkan 1.4), Windows 11 build 26200, i5-14600KF. Zone: Vulkan command-buffer replay (`gpu/vk/vk_replay.cpp`), the `RX_VK_PARALLEL=1` path only. in-game A/B: pending, results will be added in a comment ## AI assistance Assisted-by: oc-impl-2 (opencode/deepseek-v4.1-flash), oc-upstream-1 (opencode/deepseek-v4.1-flash-free)

github-sync added 1 commit

2026-11-12 13:38:09 +00:00

perf(vk): a command buffer too short to split skips the dry pass 3d9b858e31

Assisted-by: oc-impl-2 (opencode/deepseek-v4.1-flash)
Trace-Issue: #2278

github-sync commented

2026-11-12 13:40:45 +00:00

Author

Owner

@Revens2 commented on GitHub · 2026-11-12 13:25 UTC · original

In-game A/B (short command buffers skip the parallel replay dry pass)

build	avg FPS	1% low	p99 ms	frames >33 ms	replay submit ms	replay draw ms
control	97.31	79.31	12.61	0	4.84	3.69
patch	98.77	83.02	12.05	0	4.73	3.64
patch + RX_VK_NO_DRY_SHORT=1	97.78	81.38	12.29	1	4.76	3.68

Path: "N short ones recorded here" 2329 per report with the patch (0 with the fallback), dry pass 0.22 -> 0.17 ms; segments per big command buffer unchanged (2.6-2.7). Replay CPU submit 4.84 -> 4.73 ms/frame (-2.3%), draw 3.69 -> 3.64, memory sync 1.29 -> 1.25. Costs: avg +1.5%, 1% low +4.7%, p99 -0.56 ms; the NO_DRY_SHORT=1 fallback lands within noise of control. No black-frame or exposure anomalies.

Setup: RTX 5070 12 GB, i5-14600KF, Windows 11 26200, 1440p, frame generation on, 240 cap, RX_VK_PARALLEL=4 (owner env), same save/route, 2 clean runs per build, CPU idle checks. DLL-only on the installed v2.1.0 host (MKWVK API 14): the vk_replay change was ported onto the v2.1.0 tree, because an upstream-main build (51da0464, API 19) cannot load into that host.

> [@Revens2](https://github.com/Revens2) commented on GitHub · 2026-11-12 13:25 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2781#issuecomment-6824860637) In-game A/B (short command buffers skip the parallel replay dry pass) | build | avg FPS | 1% low | p99 ms | frames >33 ms | replay submit ms | replay draw ms | |---|---|---|---|---|---|---| | control | 97.31 | 79.31 | 12.61 | 0 | 4.84 | 3.69 | | patch | 98.77 | 83.02 | 12.05 | 0 | 4.73 | 3.64 | | patch + RX_VK_NO_DRY_SHORT=1 | 97.78 | 81.38 | 12.29 | 1 | 4.76 | 3.68 | Path: "N short ones recorded here" 2329 per report with the patch (0 with the fallback), dry pass 0.22 -> 0.17 ms; segments per big command buffer unchanged (2.6-2.7). Replay CPU submit 4.84 -> 4.73 ms/frame (-2.3%), draw 3.69 -> 3.64, memory sync 1.29 -> 1.25. Costs: avg +1.5%, 1% low +4.7%, p99 -0.56 ms; the NO_DRY_SHORT=1 fallback lands within noise of control. No black-frame or exposure anomalies. Setup: RTX 5070 12 GB, i5-14600KF, Windows 11 26200, 1440p, frame generation on, 240 cap, RX_VK_PARALLEL=4 (owner env), same save/route, 2 clean runs per build, CPU idle checks. DLL-only on the installed v2.1.0 host (MKWVK API 14): the vk_replay change was ported onto the v2.1.0 tree, because an upstream-main build (51da0464, API 19) cannot load into that host.

github-sync referenced this pull request

2026-11-12 14:48:09 +00:00

fix(tests): the input-background fixture reads oc_consumed_pads, so clang 23 builds it #2777

github-sync commented

2026-11-12 14:48:14 +00:00

Author

Owner

@Joey-Magnus approved on GitHub · 2026-11-12 14:35 UTC · original

CI never ran on this head, so I merged it into current main and ran the whole tests/run_all.py with the Vulkan headers. 459 passed. The only failure is tests/test_lift_cache.py, which fails on main on this machine too. I traced the logic. The stream_work(h, 2 * per) < 2 * per test at vk_replay.cpp:1531 matches main's one-segment case: nseg is clamped from total / per, and stream_work counts the same ops as dry_work. NOP_JUMP and NOP_CALL do nothing in replay, so walking to NOP_END is linear and safe. The short path is the serial path players already run with parallel off. It fully replaces the old one-segment record_segment, and after a multi-segment buffer the next recording still starts with full_barrier. Turning off maybe_split matches main, where the one segment ran in Worker mode and wasn't split. The in-game A/B is on a v2.1.0 port, not this head, but the path is off by default, so that's acceptable. Heads-up: #2786 edits the same 240-buffer logf block, and the two conflict there.

> [@Joey-Magnus](https://github.com/Joey-Magnus) approved on GitHub · 2026-11-12 14:35 UTC · [original](https://github.com/NotRexed/Hookshot/pull/2781#pullrequestreview-5876254464) CI never ran on this head, so I merged it into current main and ran the whole tests/run_all.py with the Vulkan headers. 459 passed. The only failure is tests/test_lift_cache.py, which fails on main on this machine too. I traced the logic. The `stream_work(h, 2 * per) < 2 * per` test at vk_replay.cpp:1531 matches main's one-segment case: nseg is clamped from total / per, and stream_work counts the same ops as dry_work. NOP_JUMP and NOP_CALL do nothing in replay, so walking to NOP_END is linear and safe. The short path is the serial path players already run with parallel off. It fully replaces the old one-segment record_segment, and after a multi-segment buffer the next recording still starts with full_barrier. Turning off maybe_split matches main, where the one segment ran in Worker mode and wasn't split. The in-game A/B is on a v2.1.0 port, not this head, but the path is off by default, so that's acceptable. Heads-up: #2786 edits the same 240-buffer logf block, and the two conflict there.

github-sync referenced this pull request

2026-11-12 14:48:19 +00:00

feat(vk): ParLock counted by site, and why its two busiest sites take it #2786

github-sync referenced this pull request from a commit

2026-11-12 17:57:17 +00:00

perf(vk): a command buffer too short to split skips the dry pass (#2781)

github-sync closed this pull request

2026-11-12 17:59:16 +00:00

Commenting is not possible because the repository is archived.

No reviewers