MagicBeanDev/Longshot

Watch

Fork 99

fix(vk): order overlapping inline buffer updates #329

Merged

HumanBean merged 1 commit from klimovich008/Longshot:fix/inline-write-order into main

2026-11-14 00:59:56 +00:00

klimovich008 commented

2026-11-14 00:46:41 +00:00

Contributor

Why

Overlapping inline uniform updates can race on Vulkan, reporting SYNC-HAZARD-WRITE-AFTER-WRITE.

What changed

gpu/vk/vk_replay.cpp: One-line change. full_barrier(cmd); orders writes; CPU fallbacks/chunking stay unchanged.
tests/test_inline_write_order.py: real helper/barrier/deduplication, with two failing controls.

Flags and settings

None.

Measurements

Native record-time WAW: main@514591a3 1, this PR@7b2c70f 0 (one case; no submits).
AMD RX 9070 XT, Windows driver 32.0.32015.12012. No performance claim.

Testing

Tested: python -I -B tests/run_all.py --jobs 4 --timeout 480: 494 passed, 6 failed, 34 skipped. Failures reproduce on unchanged main: test_release, test_check_third_party, test_spawn_emit, test_fpx_sticky, test_nup_paths, test_menu_i18n.
Fails on the old code, passes now.
Rebased onto main 4bab3915 (d99befb3; main changed none of the files the fix or its test reads since 514591a3): tests/test_inline_write_order.py rerun there: current passes, both failing controls fail. The full run above is from 514591a3.
Backport: 96s Hyrule, zero WAW; BC02251 and timing failures remain.

Not tested: main-game integration, repeated play, other hardware/OS.

Needs other testers: NVIDIA/Intel/Linux: check update ordering (request to open).

Screenshots

Validation-only claim.

Risk and rollback

Extra barriers may cost GPU time. Revert this commit.

Issues

Related: #189 (flush timing).

Checklist

For everyone

One topic.
The title is a Conventional Commit (type(scope): description).
Rebased on current main.
python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
Tested in the game, or what wasn't is named under Testing.
Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
No game files, dumps, build output or generated files.
third_party/ untouched (a new folder with a PROVENANCE.md instead).
The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
A game-free test added for the fix, failing on main and passing here.
A check under tests/ that reads changed source updated in this PR, not weakened.
Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. Windows only.

## Why Overlapping inline uniform updates can race on Vulkan, reporting SYNC-HAZARD-WRITE-AFTER-WRITE. ## What changed - `gpu/vk/vk_replay.cpp`: One-line change. `full_barrier(cmd);` orders writes; CPU fallbacks/chunking stay unchanged. - `tests/test_inline_write_order.py`: real helper/barrier/deduplication, with two failing controls. ## Flags and settings None. ## Measurements Native record-time WAW: main@514591a3 **1**, this PR@7b2c70f **0** (one case; no submits). AMD RX 9070 XT, Windows driver 32.0.32015.12012. No performance claim. ## Testing **Tested:** `python -I -B tests/run_all.py --jobs 4 --timeout 480`: 494 passed, 6 failed, 34 skipped. Failures reproduce on unchanged main: `test_release`, `test_check_third_party`, `test_spawn_emit`, `test_fpx_sticky`, `test_nup_paths`, `test_menu_i18n`. Fails on the old code, passes now. Rebased onto main 4bab3915 (d99befb3; main changed none of the files the fix or its test reads since 514591a3): `tests/test_inline_write_order.py` rerun there: current passes, both failing controls fail. The full run above is from 514591a3. Backport: 96s Hyrule, zero WAW; BC02251 and timing failures remain. **Not tested:** main-game integration, repeated play, other hardware/OS. **Needs other testers:** NVIDIA/Intel/Linux: check update ordering (request to open). ## Screenshots Validation-only claim. ## Risk and rollback Extra barriers may cost GPU time. Revert this commit. ## Issues Related: [#189](https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/189) (flush timing). ## Checklist ### For everyone - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [x] Rebased on current `main`. - [x] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [ ] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths. Windows only.

klimovich008 added 1 commit

2026-11-14 00:46:41 +00:00

fix(vk): order overlapping inline buffer updates

CI / Conventional Commit title (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Blocked by required conditions

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Blocked by required conditions

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Blocked by required conditions

Details

CI / Workflow lint (actionlint) (pull_request) Blocked by required conditions

Details

Third-party parts / As their notes say (pull_request) Blocked by required conditions

Details

Third-party parts / The same as upstream (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Failing after 4s

Details

Website / Website on Cloudflare (pull_request_target) Has been skipped

Details

d99befb31d

Uniform updates are flushed in stream order, but the barriers around a
whole batch do not order overlapping vkCmdUpdateBuffer writes inside it.
Use the existing barrier in the shared write helper before each GPU write.

Exercise the real helper, barrier and work wrappers with overlapping
writes and the barrier dedupe enabled. Keep failing controls for removing
the barrier and the update's work count.

Assisted-by: OpenAI Codex
Co-Authored-By: Codex <[email protected]>

HumanBean added this to the v3.0.0 milestone

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

HumanBean changed title from ~~WIP: fix(vk): order overlapping inline buffer updates~~ to fix(vk): order overlapping inline buffer updates

2026-11-14 00:59:52 +00:00

HumanBean merged commit ed088cb6eb into main

2026-11-14 00:59:56 +00:00

HumanBean deleted branch fix/inline-write-order

2026-11-14 00:59:56 +00:00

HumanBean referenced this pull request from a commit

2026-11-14 00:59:58 +00:00

fix(vk): order overlapping inline buffer updates (#329)

HumanBean commented

2026-11-14 01:01:42 +00:00

Owner

Post-merge code review of d99befb3, merged as ed088cb6. It merged while I was reviewing it, so this is a comment, not a review. Read only, nothing built or run here.

The hazard is real, but the fix costs more than it needs to, and it went in unmeasured (the Measured box was unticked and there was no table). flush_ub_updates (gpu/vk/vk_replay.cpp) already puts one full_barrier before and one after a recording's uniform updates. Line 889 now adds an ALL_COMMANDS to ALL_COMMANDS memory barrier before every vkCmdUpdateBuffer, so N updates in a recording cost N+1 barriers. Each update counts as work (note_work), so the dedupe never skips one. NVN games update the same uniform block many times a recording (why the overlay copies exist), so N can be large, and this is the replay's hot path.

For v3.0.0: it's on main, so it ships unless someone acts. I'd want one of these before the release:

Measure it now: frame time and GPU time in a busy scene (Castle Town market, Hyrule Field), ed088cb6 against its parent 9ef17269, several interleaved runs, plus the ubupd span from RX_VK_PASS_TIMING. If the cost is noise, keep it.
Or replace it with a cheaper form:
- Barrier only on overlap: keep the ranges written since the last barrier, and call full_barrier only when a write meets one of them.
- Or merge on the CPU: in flush_ub_updates, apply the recording's ub_updates in stream order into merged ranges (later bytes win) and write each once. No WAW is left to order, fewer bytes go out, and no barrier is added.
- Either way, tests/test_inline_write_order.py should also assert that two disjoint writes add no barrier.
Or revert it for v3.0.0. The PR showed a sync-validation warning, but no symptom in the game.

Also left open:

"Backport: 96s Hyrule, zero WAW; BC02251 and timing failures remain" doesn't say which build, commit or GPU.
"main-game integration" is listed as not tested.
No test request was opened (test_requests.py).
The Linux box was unticked.
CI's runs for d99befb3 were still pending (run 2842) when it merged.
Nit: the comment on line 889 isn't in the repo's parenthesised "why" style.

Post-merge code review of d99befb3, merged as ed088cb6. It merged while I was reviewing it, so this is a comment, not a review. Read only, nothing built or run here. **The hazard is real, but the fix costs more than it needs to, and it went in unmeasured** (the *Measured* box was unticked and there was no table). `flush_ub_updates` (`gpu/vk/vk_replay.cpp`) already puts one `full_barrier` before and one after a recording's uniform updates. Line 889 now adds an ALL_COMMANDS to ALL_COMMANDS memory barrier before *every* `vkCmdUpdateBuffer`, so N updates in a recording cost N+1 barriers. Each update counts as work (`note_work`), so the dedupe never skips one. NVN games update the same uniform block many times a recording (why the overlay copies exist), so N can be large, and this is the replay's hot path. **For v3.0.0: it's on main, so it ships unless someone acts.** I'd want one of these before the release: 1. **Measure it now:** frame time and GPU time in a busy scene (Castle Town market, Hyrule Field), ed088cb6 against its parent 9ef17269, several interleaved runs, plus the `ubupd` span from `RX_VK_PASS_TIMING`. If the cost is noise, keep it. 2. **Or replace it with a cheaper form:** - Barrier only on overlap: keep the ranges written since the last barrier, and call `full_barrier` only when a write meets one of them. - Or merge on the CPU: in `flush_ub_updates`, apply the recording's `ub_updates` in stream order into merged ranges (later bytes win) and write each once. No WAW is left to order, fewer bytes go out, and no barrier is added. - Either way, `tests/test_inline_write_order.py` should also assert that two disjoint writes add no barrier. 3. **Or revert it for v3.0.0.** The PR showed a sync-validation warning, but no symptom in the game. Also left open: - "Backport: 96s Hyrule, zero WAW; BC02251 and timing failures remain" doesn't say which build, commit or GPU. - "main-game integration" is listed as not tested. - No test request was opened (`test_requests.py`). - The Linux box was unticked. - CI's runs for d99befb3 were still pending (run 2842) when it merged. - Nit: the comment on line 889 isn't in the repo's parenthesised "why" style.

HumanBean referenced this pull request

2026-11-14 01:36:43 +00:00

fix(vk): on Intel iGPUs a split submission's uniform updates are written by its last part #189

HumanBean referenced this pull request

2026-11-14 01:36:43 +00:00

fix(vk): on Intel iGPUs a split submission's uniform updates are written by its last part #189

HumanBean referenced this pull request from a commit

2026-11-14 01:39:47 +00:00

fix(vk): on Intel iGPUs a split submission's uniform updates are written by its last part (#189)

klimovich008 commented

2026-11-14 01:45:29 +00:00

Author

Contributor

Thanks, HumanBean. Agreed on the cost: it was opened as WIP with Measured unticked, and it merged before that was done. Each point:

Measuring now (your option 1). The standard test: Hyrule Field, 16:9, the fixed Custom profile, HOS_FPS_MAX=240, 3 alternating pairs. The merged line as a renderer-only pack on main 839e85ff (patch-identical to ed088cb6's change, which is the only difference between ed088cb6 and its parent in the replay) against the same base without it, on an RX 9070 XT, AMD 26.10.2, Ryzen 7 5700X3D, Windows 11. Reported: fps, CPU ms a frame, p99, worst frame, plus the renderer's own GPU time and barriers-recorded lines a frame from each run's log. Results go here when the runs are done.

A cheaper form (your option 2). A follow-up from Codex, who wrote the fix: the CPU merge in flush_ub_updates (the recording's updates applied in stream order into merged ranges, later bytes winning, each written once: no barrier added, fewer bytes) or, if that does not fit the overlay copies, the barrier only when a write meets a range written since the last barrier. Either way tests/test_inline_write_order.py gains the case you name: two disjoint writes add no barrier.

Revert (your option 3). If the A/B shows a cost and the follow-up is not ready before v3.0.0, we will open the revert ourselves.

The rest:

"Backport: 96s Hyrule, zero WAW": Codex's diagnostic build on main 839e85ff, the same line plus clock and texture-creation diagnostics, with validation on; RX 9070 XT, AMD 26.10.2, Windows 11; one 96 s run. It showed zero write-after-write reports against the control's ten; it is not a performance or picture pass (that run also had BC5/BC6 02251 reports and a rejected timing window).
Main-game integration: still not tested beyond that run.
Test request: not opened; it will be opened with the follow-up, so testers check the form that stays.
Linux: not built.
CI: noted that run 2842 was still pending at the merge; we'll check its result and say here if it failed.
Nit: the comment on line 889 goes to the parenthesised "why" style in the follow-up.

Thanks, HumanBean. Agreed on the cost: it was opened as WIP with *Measured* unticked, and it merged before that was done. Each point: **Measuring now (your option 1).** The standard test: Hyrule Field, 16:9, the fixed Custom profile, `HOS_FPS_MAX=240`, 3 alternating pairs. The merged line as a renderer-only pack on main 839e85ff (patch-identical to ed088cb6's change, which is the only difference between ed088cb6 and its parent in the replay) against the same base without it, on an RX 9070 XT, AMD 26.10.2, Ryzen 7 5700X3D, Windows 11. Reported: fps, CPU ms a frame, p99, worst frame, plus the renderer's own GPU time and barriers-recorded lines a frame from each run's log. Results go here when the runs are done. **A cheaper form (your option 2).** A follow-up from Codex, who wrote the fix: the CPU merge in `flush_ub_updates` (the recording's updates applied in stream order into merged ranges, later bytes winning, each written once: no barrier added, fewer bytes) or, if that does not fit the overlay copies, the barrier only when a write meets a range written since the last barrier. Either way `tests/test_inline_write_order.py` gains the case you name: two disjoint writes add no barrier. **Revert (your option 3).** If the A/B shows a cost and the follow-up is not ready before v3.0.0, we will open the revert ourselves. The rest: - *"Backport: 96s Hyrule, zero WAW"*: Codex's diagnostic build on main 839e85ff, the same line plus clock and texture-creation diagnostics, with validation on; RX 9070 XT, AMD 26.10.2, Windows 11; one 96 s run. It showed zero write-after-write reports against the control's ten; it is not a performance or picture pass (that run also had BC5/BC6 `02251` reports and a rejected timing window). - *Main-game integration:* still not tested beyond that run. - *Test request:* not opened; it will be opened with the follow-up, so testers check the form that stays. - *Linux:* not built. - *CI:* noted that run 2842 was still pending at the merge; we'll check its result and say here if it failed. - *Nit:* the comment on line 889 goes to the parenthesised "why" style in the follow-up.

mooki added the

area/renderer

kind

bug

labels

2026-11-14 02:05:23 +00:00

klimovich008 commented

2026-11-14 02:26:13 +00:00

Author

Contributor

Measured (HumanBean's option 1): on this PC the added barriers cost about 0.06 ms of GPU time a frame and no measurable frame rate.

main: 839e85ff (2026-11-13)
this change: the merged line (d99befb3, merged as ed088cb6) as a renderer-only pack on 839e85ff (diag/waw839 @ 96b445d4, longshot_vk.dll only)
Hardware: RX 9070 XT, AMD 26.10.2, Ryzen 7 5700X3D, 64 GB, Windows 11
Scene: the standard test: Hyrule Field from its save, camera turning at full speed, 16:9, the fixed Custom profile, HOS_FPS_MAX=240, 20 s window
Method: 3 alternating pairs (warm-ups first), the renderer's own lines from each run's log (the last four 10 s summaries)

	main @ `839e85ff`	+ the line	Δ
fps, median of 3	62.35	65.29	see below
fps, the runs without a long stall	65.49 (n=1)	65.89, 65.29 (n=2)	none measurable
CPU ms a frame, median	63.7	59.7	see below
GPU time a frame (run means)	7.42 / 7.45 / 7.45 ms	7.53 / 7.53 / 7.45 ms	+0.06 ms (+0.8 %)
full barriers recorded a frame	434 / 433 / 434	513 / 512 / 509	+78 (+18 %)
inline uniform updates a frame	80	80	—
frames over 100 / 250 ms (all runs)	8 / 4	4 / 2	—

The medians are not a gain: three of the six runs (two controls, one with the line) had stalls of 800-890 ms that pulled them to 61-62 fps, and two of those three were controls. The stall-free runs agree within 0.6 fps. Labelled diagnostic (3 pairs, a difference under 5 %).

So in Hyrule on this card the barrier count rises by about one per inline update (+78 a frame, 80 updates), and the GPU pays about 0.06 ms for them; the frame is limited by the CPU's replay here, so fps does not move. Not measured: other GPUs, where a full barrier can cost more (NVIDIA, Intel, the Steam Deck), and busier scenes such as the Castle Town market. That is the case for the cheaper form (option 2), which Codex is preparing: with it, this table should show the barrier count back at main's.

Measured (HumanBean's option 1): on this PC the added barriers cost about 0.06 ms of GPU time a frame and no measurable frame rate. main: 839e85ff (2026-11-13) this change: the merged line (d99befb3, merged as ed088cb6) as a renderer-only pack on 839e85ff (diag/waw839 @ 96b445d4, `longshot_vk.dll` only) Hardware: RX 9070 XT, AMD 26.10.2, Ryzen 7 5700X3D, 64 GB, Windows 11 Scene: the standard test: Hyrule Field from its save, camera turning at full speed, 16:9, the fixed Custom profile, `HOS_FPS_MAX=240`, 20 s window Method: 3 alternating pairs (warm-ups first), the renderer's own lines from each run's log (the last four 10 s summaries) | | main @ 839e85ff | + the line | Δ | |---|---|---|---| | fps, median of 3 | 62.35 | 65.29 | see below | | fps, the runs without a long stall | 65.49 (n=1) | 65.89, 65.29 (n=2) | none measurable | | CPU ms a frame, median | 63.7 | 59.7 | see below | | GPU time a frame (run means) | 7.42 / 7.45 / 7.45 ms | 7.53 / 7.53 / 7.45 ms | +0.06 ms (+0.8 %) | | full barriers recorded a frame | 434 / 433 / 434 | 513 / 512 / 509 | +78 (+18 %) | | inline uniform updates a frame | 80 | 80 | — | | frames over 100 / 250 ms (all runs) | 8 / 4 | 4 / 2 | — | The medians are not a gain: three of the six runs (two controls, one with the line) had stalls of 800-890 ms that pulled them to 61-62 fps, and two of those three were controls. The stall-free runs agree within 0.6 fps. Labelled diagnostic (3 pairs, a difference under 5 %). So in Hyrule on this card the barrier count rises by about one per inline update (+78 a frame, 80 updates), and the GPU pays about 0.06 ms for them; the frame is limited by the CPU's replay here, so fps does not move. Not measured: other GPUs, where a full barrier can cost more (NVIDIA, Intel, the Steam Deck), and busier scenes such as the Castle Town market. That is the case for the cheaper form (option 2), which Codex is preparing: with it, this table should show the barrier count back at main's.

mooki added the

priority

high

label

2026-11-14 04:18:38 +00:00

mooki referenced this pull request

2026-11-14 22:11:54 +00:00

Game: rough frame times in the Castle Town market by day as child Link on nightly 153 at 4K with DLAA #597

mooki referenced this pull request

2026-11-15 02:50:22 +00:00

feat(vk): bind segment-straddling buffers and texel views as piece arrays, and split transfers at segment cuts, on macOS #663

mooki referenced this pull request