feat(vk): observe completed GPU write-back differences #2131

Merged

Arkex-ai merged 4 commits from feat/wb-diff-observations into main

2026-11-11 21:30:45 +00:00

Arkex-ai commented

2026-11-11 04:29:33 +00:00

(Migrated from github.com)

Summary

There is no measurement of how much copied GPU data actually changed, so it is hard to judge whether skipping copies would be safe. This adds optional observations of completed write-backs. It changes no copy decisions and claims no performance improvement.

Coordinated with blessedplummer in #2099; their dirty-origin, urgent/bulk and sparse-mirror work remains separate. Dev-work claim.

Technical changes

Export one actual-difference flag per chunk from the existing shader with RX_VK_WB_OBSERVE=1; regenerate SPIR-V with the pinned glslang 16.6.0.
Tag mapped results with their submission and read them only after completion, before reuse. Invalid metadata, flush/invalidation or optional allocation failure leaves observations unknown and preserves ordinary write-back. No extra queue wait, submission or thread.
Report bounded aggregate diagnostics. Changed-chunk volume is not exact changed bytes, and a clean observation cannot justify skipping a later write.

Validation

Current tested head da2747e1, rebased onto main 940eb9ef. The tested-configurations conflict preserves the new main evidence rows and this PR’s earlier benchmark. Production/test blobs are unchanged from fb8d0c55; focused checks pass again on both systems. Review follow-up uses the shared env_on rule, so on and true enable observations and off spellings disable them. Production lifetime checks pass for ten unset/off/on forms; all five causal mutations are rejected. Shared C/C++/Python switch and menu-switch checks pass on Windows and Linux. The worklist entry is in Fixed, waiting for a release. Shader source and generated SPIR-V are identical to the GPU-tested 7ee371f8; no new game/runtime or overhead result is claimed. Fresh hosted checks are separate exact-head evidence.

Earlier validation evidence and limits

At 7ee371f8 on 6c8c1bfb, four CPU/page checks, lifetime/four mutations and all five actual RTX shader scenarios passed; the main shader passed off parity and rejected missing feedback. Both tested-configuration rows were retained during rebase.

Head 6d53c2a7 on main 4a53c897 passed the same focused Windows CPU/lifetime and actual RTX shader checks before this documentation-only rebase.

Earlier tested head bb9944f2, rebased onto main 6df80a8d. Both production/test patches are unchanged by range-diff; the documentation conflict preserves both evidence rows.

Current-head Windows production-helper checks pass for CPU writes, chunk splitting, stale shadows and page tracking. Observation lifetime passes with the option off/on; all four causal mutations are rejected (premature completion, repeated consumption, missing flags and ignored invalidation error).
Actual current-head shader execution passes on RTX 5070 Ti: off mode, mixed chunks, reused clean results, zero count and the 65535-wide 2D grid boundary, checking guest/shadow parity and output guards. The main shader passes off-mode parity then fails exactly the missing-feedback assertion.
Fresh hosted checks run on this head. No current-head Linux execution, complete game build, gameplay or overhead comparison is claimed.

Earlier authored shader execution passed on Linux llvmpipe; lifetime tests and four mutations passed on both platforms. At earlier head 2fe9e4c0, full Linux was 352 / 0 failed / 23 skipped, Windows 341 / 4 failed / 30 skipped: three failures reproduced on the exact base, and one unchanged compiler fixture timed out then passed isolated. Fourteen release-script cases subsequently passed with jq; the aggregate's earlier skipped internal cases are not counted as executed.

Six earlier native processes used the fab8cb37 host with candidate edits, clean/off/on/on/off/clean, Kokiri balcony at 4K, FG off and cap 144. Mean source FPS: clean 75.25, off 75.52, on 74.24. On was 1.70% below off; two windows per mode do not establish equivalence or precise overhead. Reports found differences in about 15% of chunks, covering about 13% of compared volume, with zero unavailable chunks. All six exited normally and preserved the checkpoint. These are earlier-host results, not validation of the current rebase.

No AMD hardware, physical noncoherent-memory or campaign coverage is claimed. No game-derived files are included.

Current-head workflow	Result
Game-free source regressions	Queued
Linux AppImage	Queued
PR title	Queued
Windows builder	Queued

## Summary There is no measurement of how much copied GPU data actually changed, so it is hard to judge whether skipping copies would be safe. This adds optional observations of completed write-backs. It changes no copy decisions and claims no performance improvement. Coordinated with blessedplummer in #2099; their dirty-origin, urgent/bulk and sparse-mirror work remains separate. [Dev-work claim](https://discord.com/channels/1568810484818378804/1569019786766188665/1569914220039114883). ## Technical changes - Export one actual-difference flag per chunk from the existing shader with `RX_VK_WB_OBSERVE=1`; regenerate SPIR-V with the pinned glslang 16.6.0. - Tag mapped results with their submission and read them only after completion, before reuse. Invalid metadata, flush/invalidation or optional allocation failure leaves observations unknown and preserves ordinary write-back. No extra queue wait, submission or thread. - Report bounded aggregate diagnostics. Changed-chunk volume is not exact changed bytes, and a clean observation cannot justify skipping a later write. ## Validation Current tested head **`da2747e1`**, rebased onto main **`940eb9ef`**. The tested-configurations conflict preserves the new main evidence rows and this PR’s earlier benchmark. Production/test blobs are unchanged from `fb8d0c55`; focused checks pass again on both systems. Review follow-up uses the shared `env_on` rule, so `on` and `true` enable observations and off spellings disable them. Production lifetime checks pass for ten unset/off/on forms; all five causal mutations are rejected. Shared C/C++/Python switch and menu-switch checks pass on Windows and Linux. The worklist entry is in Fixed, waiting for a release. Shader source and generated SPIR-V are identical to the GPU-tested `7ee371f8`; no new game/runtime or overhead result is claimed. Fresh [hosted checks](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2131/checks) are separate exact-head evidence. <details> <summary>Earlier validation evidence and limits</summary> At `7ee371f8` on `6c8c1bfb`, four CPU/page checks, lifetime/four mutations and all five actual RTX shader scenarios passed; the main shader passed off parity and rejected missing feedback. Both tested-configuration rows were retained during rebase. Head `6d53c2a7` on main `4a53c897` passed the same focused Windows CPU/lifetime and actual RTX shader checks before this documentation-only rebase. Earlier tested head **`bb9944f2`**, rebased onto main **`6df80a8d`**. Both production/test patches are unchanged by range-diff; the documentation conflict preserves both evidence rows. - Current-head Windows production-helper checks pass for CPU writes, chunk splitting, stale shadows and page tracking. Observation lifetime passes with the option off/on; all four causal mutations are rejected (premature completion, repeated consumption, missing flags and ignored invalidation error). - Actual current-head shader execution passes on **RTX 5070 Ti**: off mode, mixed chunks, reused clean results, zero count and the 65535-wide 2D grid boundary, checking guest/shadow parity and output guards. The main shader passes off-mode parity then fails exactly the missing-feedback assertion. - Fresh [hosted checks](https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2131/checks) run on this head. No current-head Linux execution, complete game build, gameplay or overhead comparison is claimed. Earlier authored shader execution passed on Linux llvmpipe; lifetime tests and four mutations passed on both platforms. At earlier head `2fe9e4c0`, full Linux was **352 / 0 failed / 23 skipped**, Windows **341 / 4 failed / 30 skipped**: three failures reproduced on the exact base, and one unchanged compiler fixture timed out then passed isolated. Fourteen release-script cases subsequently passed with jq; the aggregate's earlier skipped internal cases are not counted as executed. Six earlier native processes used the `fab8cb37` host with candidate edits, clean/off/on/on/off/clean, Kokiri balcony at 4K, FG off and cap 144. Mean source FPS: clean 75.25, off 75.52, on 74.24. On was 1.70% below off; two windows per mode do not establish equivalence or precise overhead. Reports found differences in about 15% of chunks, covering about 13% of compared volume, with zero unavailable chunks. All six exited normally and preserved the checkpoint. These are earlier-host results, not validation of the current rebase. No AMD hardware, physical noncoherent-memory or campaign coverage is claimed. No game-derived files are included. </details>  | Current-head workflow | Result | |---|---| | [Game-free source regressions](https://github.com/NotRexed/Hookshot/actions/runs/43098217881) | Queued | | [Linux AppImage](https://github.com/NotRexed/Hookshot/actions/runs/43098217917) | Queued | | [PR title](https://github.com/NotRexed/Hookshot/actions/runs/43098217772) | Queued | | [Windows builder](https://github.com/NotRexed/Hookshot/actions/runs/43098217993) | Queued |

Arkex-ai commented

2026-11-11 04:49:37 +00:00

(Migrated from github.com)

All current-head checks are green at d67505a4. The follow-up fixes the Windows fixture's CRT declarations, and the rebase includes main's shared release-fixture race fix (#2130). The GPU/host production patch is unchanged; full previous-head local results and the earlier-host six-run comparison remain labeled separately in the description.

All current-head checks are green at `d67505a4`. The follow-up fixes the Windows fixture's CRT declarations, and the rebase includes main's shared release-fixture race fix (#2130). The GPU/host production patch is unchanged; full previous-head local results and the earlier-host six-run comparison remain labeled separately in the description.

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 20:06:43 +00:00

Joey-Magnus (Migrated from github.com) left a comment

This is careful work, and it stays inside what it claims: with RX_VK_WB_OBSERVE unset, the only changes on the hot path are a uniform branch in the shader, an 8-byte-larger push constant and an early return in wbd_table_get. I checked the parts that could hurt players:

Shader (gpu/vk/shaders/wb_diff.comp): observe comes from a push constant, and the early return depends only on the workgroup ID. Both barrier() calls are therefore in uniform control flow. Every invocation reaches the final barrier, because both loops run to their bounds. glslang emits them as OpControlBarrier Workgroup Workgroup AcquireRelease|WorkgroupMemory, so the shared atomicOr results are visible to invocation 0. On the vector path, the flag follows differs per vector, which matches what the vote writes.
Generated header: I regenerated wb_diff_comp_spv.h with glslang 16.6.0 using the command in the shader, and it is identical to the committed one.
Lifetime (gpu/vk/vk_memory.cpp): results are read only in wbd_table_get once t.value <= mem_completed, which is the same gate the ring already uses for reuse. They are read before the slot can grow, be freed or be overwritten. The compute→host barrier plus invalidate cover non-coherent memory. The 0xff sentinel makes a missing store invalidate the whole batch instead of reading it as clean. The growth-failure fallback keeps the old behaviour: retry at the ordinary size, or return nullptr and do whole copies.
Tests: on the PR's head, test_wb_*, test_env_on and test_menu_env_on pass. CI is green.

minor gpu/vk/vk_memory.cpp:3422: RX_VK_WB_OBSERVE is read as atoi(s) != 0. Since #2042, on/off switches go through env_on (this file already includes hookshot_env.h and uses it for RX_VK_LOG_ADDR, RX_VK_TABLE_NOW and others), so as written RX_VK_WB_OBSERVE=on or =true would quietly do nothing. static const bool on = env_on("RX_VK_WB_OBSERVE"); fixes it.

minor docs/worklist.md:36: the row is added to Open with you as owner, which fits "put your name in Owner in the same PR that starts the work". But this PR also finishes that work, and the worklist says a merged PR's row moves to Fixed, waiting for a release. Put it there with this PR's link, or nobody will clear it.

minor: the branch conflicts with main in docs/tested-configurations.md (rows added on top). The description could also open with a sentence a player could follow, per CONTRIBUTING (for example, "Nobody knows how much of each write-back actually changed, so we can't tell whether skipping some would be safe"). The current summary gets there, but in its second sentence and in renderer terms.

This is careful work, and it stays inside what it claims: with `RX_VK_WB_OBSERVE` unset, the only changes on the hot path are a uniform branch in the shader, an 8-byte-larger push constant and an early return in `wbd_table_get`. I checked the parts that could hurt players: - **Shader** (`gpu/vk/shaders/wb_diff.comp`): `observe` comes from a push constant, and the early `return` depends only on the workgroup ID. Both `barrier()` calls are therefore in uniform control flow. Every invocation reaches the final barrier, because both loops run to their bounds. glslang emits them as `OpControlBarrier Workgroup Workgroup AcquireRelease|WorkgroupMemory`, so the shared `atomicOr` results are visible to invocation 0. On the vector path, the flag follows `differs` per vector, which matches what the vote writes. - **Generated header**: I regenerated `wb_diff_comp_spv.h` with glslang 16.6.0 using the command in the shader, and it is identical to the committed one. - **Lifetime** (`gpu/vk/vk_memory.cpp`): results are read only in `wbd_table_get` once `t.value <= mem_completed`, which is the same gate the ring already uses for reuse. They are read before the slot can grow, be freed or be overwritten. The compute→host barrier plus `invalidate` cover non-coherent memory. The `0xff` sentinel makes a missing store invalidate the whole batch instead of reading it as clean. The growth-failure fallback keeps the old behaviour: retry at the ordinary size, or return `nullptr` and do whole copies. - **Tests**: on the PR's head, `test_wb_*`, `test_env_on` and `test_menu_env_on` pass. CI is green. **minor** `gpu/vk/vk_memory.cpp:3422`: `RX_VK_WB_OBSERVE` is read as `atoi(s) != 0`. Since #2042, on/off switches go through `env_on` (this file already includes `hookshot_env.h` and uses it for `RX_VK_LOG_ADDR`, `RX_VK_TABLE_NOW` and others), so as written `RX_VK_WB_OBSERVE=on` or `=true` would quietly do nothing. `static const bool on = env_on("RX_VK_WB_OBSERVE");` fixes it. **minor** `docs/worklist.md:36`: the row is added to **Open** with you as owner, which fits "put your name in Owner in the same PR that starts the work". But this PR also finishes that work, and the worklist says a merged PR's row moves to **Fixed, waiting for a release**. Put it there with this PR's link, or nobody will clear it. **minor**: the branch conflicts with main in `docs/tested-configurations.md` (rows added on top). The description could also open with a sentence a player could follow, per CONTRIBUTING (for example, "Nobody knows how much of each write-back actually changed, so we can't tell whether skipping some would be safe"). The current summary gets there, but in its second sentence and in renderer terms.

Arkex-ai commented

2026-11-11 20:34:27 +00:00

(Migrated from github.com)

Thanks for the detailed review. Addressed in fb8d0c55 on main bebb0c64: RX_VK_WB_OBSERVE uses env_on, the worklist row is under Fixed, and the summary opens with the measurement problem in plain language. The tested-configuration conflict is resolved with both rows kept.

The lifetime fixture covers ten off/on forms and five rejected mutations, including restoring the numeric-only parser; shared switch and menu-switch checks pass on Windows and Linux. Shader source and SPIR-V are unchanged from the RTX-tested 7ee371f8. Fresh CI is queued; no new gameplay or overhead result is claimed.

Thanks for the detailed review. Addressed in `fb8d0c55` on main `bebb0c64`: `RX_VK_WB_OBSERVE` uses `env_on`, the worklist row is under Fixed, and the summary opens with the measurement problem in plain language. The tested-configuration conflict is resolved with both rows kept. The lifetime fixture covers ten off/on forms and five rejected mutations, including restoring the numeric-only parser; shared switch and menu-switch checks pass on Windows and Linux. Shader source and SPIR-V are unchanged from the RTX-tested `7ee371f8`. Fresh CI is queued; no new gameplay or overhead result is claimed.

Joey-Magnus (Migrated from github.com) approved these changes

2026-11-11 21:22:53 +00:00

Joey-Magnus (Migrated from github.com) left a comment

All three earlier points are fixed in the head: env_on("RX_VK_WB_OBSERVE") at vk_memory.cpp:3428, the worklist row moved to Fixed, and the description opens with why. With the switch off, the hot path only gains a uniform branch, 8 bytes of push constant and an early return. Good once the queued CI is green.

All three earlier points are fixed in the head: `env_on("RX_VK_WB_OBSERVE")` at `vk_memory.cpp:3428`, the worklist row moved to Fixed, and the description opens with why. With the switch off, the hot path only gains a uniform branch, 8 bytes of push constant and an early return. Good once the queued CI is green.