WIP: perf(vk): flat per-draw caches, inline op payload copies and a flat c1 map in the replay #231

dukenheimer · 2026-11-13T15:53:28Z

dukenheimer commented

2026-11-13 15:53:28 +00:00

First-time contributor

Why

The Vulkan replay on the game's render thread sets the frame rate on CPU-bound machines (a Steam Deck above all), and each draw spent a good part of its time in hash-map lookups, a C library memmove per replayed op, and a heap allocation per kept c1 upload. This makes those per-draw costs cheaper, with the same results.

What changed

vk: gpu/vk/flat_map.h (new): FlatMap<K, V>, an integer-keyed table in one array (open addressing, linear probing, at most 3/4 full, slot from a multiplicative hash; find/end, operator[], size, clear; a clear gives back an array over 4096 slots). Ported from Corentin Bonjour's https://github.com/NotRexed/Hookshot/pull/2774 (its two commits, with authorship kept).
vk: three per-draw thread caches use it: handle_hits (texture_view_of, gpu/vk/vk_draw.cpp), graphics_pipeline's mine (vk_draw.cpp) and picks (program_for, gpu/vk/vk_shader.cpp). HandleHit is reordered and its tex_epoch is 32 bits, as texture_epoch() returns. The clear thresholds are the same, so entries are kept and dropped as before.
vk: payload<T>() (gpu/vk/vk_replay.cpp) copies sizeof(T) bytes, inline, when the op is at least that long (nearly always), instead of a variable-length memcpy, which was a call of libc's memmove for every op replayed. A shorter op is copied for its length as before.
vk: RecordingRes::c1_at (vk_draw.cpp) is a FlatMap keyed by the stage's address: no node allocation for each c1 upload a recording keeps, and no frees at the recording's clear. It is emptied at each recording's start with FlatMap::reset, which keeps its array (a clear would give an array over 4096 slots back and grow it again from 16 every recording); the recording streams' log line (every 3000 recordings) now says the most c1 stages a recording uploaded.
vk: program_for (vk_shader.cpp) holds Pick& pk across program_pick, the one place a reference into a FlatMap is held across a possible insert; a comment there says why that is safe (nothing in between inserts into the thread's picks: program_pick takes the non-recursive shader_mu and can't reach program_for again).
tests: tests/test_flat_map.py (from #2774: FlatMap against std::unordered_map, and texture_view_of over either; here also reset against clear, and c1_at emptied with reset); tests/test_replay_copies.py (payload<T> cut out and run for every length, and the flat c1_at); test_fsr_uniform_ownership.py, test_recording_admission.py and test_draw_stream_visibility.py include flat_map.h, since they cut RecordingRes out of the source.

vk_shader.cpp changes (the picks' map), so the translator's fingerprint changes: the first start after updating prepares the shaders again (the program cache of the old build is not read).

Flags and settings

None. The change is internal and gives the same results; reverting the commits removes it.

Measurements

Two machines. On the Steam Deck the same changes were measured as part of a combined build, not alone (below). The evidence is thin, and a measurement of the head is still owed before this leaves WIP: MP165 rests on two pairs (after round 1 was dropped and rerun), M1152 is one pair inside the noise, the PC is an RTX 5090 rather than a CPU-bound machine, and the Deck saw these changes only inside a combined build. Owed: the head against main, several interleaved runs, on a CPU-bound machine (the Steam Deck), with the replay's "descriptors" and "submit" ms as well as fps; then the test request and the docs/tested-configurations.md row.

PC (PC runner, one prepared game folder per arm under D:\Githubproject\Longshot-bench, warm-up per folder; experiments.md, "flat caches redone with separate bench folders")

main: e28236b4 (2026-11-12)
this PR: opus/replay-flat-caches-measured @ 53a83cca (pushed to the fork so it can be seen: the same five changes on that main). The head has been rebased since (onto fbd4202, after #198 and #107, without conflict in these files) and has the review's c1_at reset and comment on top, not measured.
Hardware: RTX 5090, Windows 11 (the runner's PC)
Scene: Kokiri Forest, the runner's moving route; M1152 = 2048x1152 window; MP165 = the 165 Hz present config
Method: 2 interleaved rounds each (MP165 round 1 excluded and rerun as round 2: main's run had outside CPU load, 22 fps for ~20 s); fps and p99 from the runner's frame log, replay times from vk: a frame's replay time (ms) lines
How to reproduce: the runner's pcrun.ps1 labels lsflat (two folders: main and this PR, each built with make_game and shaders prepared)

	main @ `e28236b4`	this PR @ 53a83cca
MP165 fps (2 pairs)	67.5, 70.0	72.2, 72.0 (+4.7, +2.0; mean +3.4)
MP165 p99	22.1 ms	20.4-21.3 ms
M1152 fps	75.0	75.4 (+0.4, sd 1.2: noise)
M1152 p99	19.7 ms	19.3 ms
replay "descriptors", ms a frame	1.61-1.81	1.54-1.57
replay "submit", ms a frame		−0.3 to −1.1 against main

An earlier pair that swapped only the renderer DLL into one folder is withdrawn: this branch changes the translator's fingerprint, so the swapped DLL dropped the shader cache and prepared in play.

Steam Deck (Deck agent, Hookshot-based user build, the same five changes among others; render-thread perf, Kokiri moving route)

Measured in a combined build, not alone, so no fps is claimed for this PR there. The profile of the render thread before and after showed what these changes target: texture_view_of 3.1%, program_for 2.5% and graphics_pipeline 1.5% of the thread were these three maps' lookups; libc memmove 6.7% (in part the per-op payload copies); malloc/free about 3.5%.

Testing

Tested:

python -I -B tests/run_all.py, Linux (Ubuntu 22.04 container, clang 23, the CPM Vulkan headers, --jobs 12): this head @ 559f480 and main @ fbd4202 fail the same 4 and nothing else: test_cpp_preflight.py, test_dev_packs.py, test_prof_clock.py, and the byte-compile of every tracked .py (no .git in the container).
python -I -B tests/run_all.py, Windows (llvm-mingw): this head @ 559f480: 454 passed, 5 failed, 72 skipped; main @ fbd4202 fails the same 5 on this PC: test_fpx_sticky, test_bake_portability, test_check_third_party, test_spawn_emit, test_release.
tests/test_flat_map.py and tests/test_replay_copies.py pass on both.
In the game: the PC runner's runs above (Windows, RTX 5090, at 53a83cca); the Steam Deck's runs of a build with these changes (Linux, RADV). The head's c1_at reset and comment have not run in the game.

Not tested:

This branch alone on a Steam Deck or another CPU-bound machine: only as part of a combined build.
AMD and Intel GPUs on Windows.

Needs other testers:

A CPU-bound PC (4-6 cores, a laptop): Kokiri Forest's replay "descriptors" and "submit" ms a frame against main, expecting them lower and fps not lower (request to open)

Screenshots

Nothing on screen.

Risk and rollback

A lookup that answered differently would show as a wrong texture or pipeline; the tests compare FlatMap with std::unordered_map over random keys and texture_view_of over both. Reverting the commits is enough. The first start after the change prepares the shaders again (the translator's fingerprint changes with vk_shader.cpp).

Memory: FlatMap holds its entries in one array. At handle_hits' clear threshold (65536 entries) that is 131072 × 56 B ≈ 7.3 MB per recording thread, against about 5 MB of nodes before, with a moment at about 11 MB while it grows (the old array and the new). With several recording threads (RX_VK_RECORDER=thread, the parallel workers) that adds up to a few MB more on the 8 GB / Steam Deck tier. c1_at keeps its array between recordings (its largest size so far, logged).

Reviewer notes

Read gpu/vk/flat_map.h first, then the three map declarations in vk_draw.cpp and vk_shader.cpp, then payload<T> in vk_replay.cpp.

Issues

See https://github.com/NotRexed/Hookshot/pull/2774 (the flat caches, by Corentin Bonjour).

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). (The test request is still to open.)
Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (2 interleaved rounds each at 53a83cca, now on the fork; the head against main on a CPU-bound machine is owed, above.)
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 row in docs/tested-configurations.md for each measured run. (Not added yet.)
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.

## Why The Vulkan replay on the game's render thread sets the frame rate on CPU-bound machines (a Steam Deck above all), and each draw spent a good part of its time in hash-map lookups, a C library `memmove` per replayed op, and a heap allocation per kept c1 upload. This makes those per-draw costs cheaper, with the same results. ## What changed - vk: `gpu/vk/flat_map.h` (new): `FlatMap<K, V>`, an integer-keyed table in one array (open addressing, linear probing, at most 3/4 full, slot from a multiplicative hash; `find`/`end`, `operator[]`, `size`, `clear`; a clear gives back an array over 4096 slots). Ported from Corentin Bonjour's https://github.com/NotRexed/Hookshot/pull/2774 (its two commits, with authorship kept). - vk: three per-draw thread caches use it: `handle_hits` (`texture_view_of`, `gpu/vk/vk_draw.cpp`), `graphics_pipeline`'s `mine` (`vk_draw.cpp`) and `picks` (`program_for`, `gpu/vk/vk_shader.cpp`). `HandleHit` is reordered and its `tex_epoch` is 32 bits, as `texture_epoch()` returns. The clear thresholds are the same, so entries are kept and dropped as before. - vk: `payload<T>()` (`gpu/vk/vk_replay.cpp`) copies `sizeof(T)` bytes, inline, when the op is at least that long (nearly always), instead of a variable-length `memcpy`, which was a call of libc's `memmove` for every op replayed. A shorter op is copied for its length as before. - vk: `RecordingRes::c1_at` (`vk_draw.cpp`) is a `FlatMap` keyed by the stage's address: no node allocation for each c1 upload a recording keeps, and no frees at the recording's clear. It is emptied at each recording's start with `FlatMap::reset`, which keeps its array (a `clear` would give an array over 4096 slots back and grow it again from 16 every recording); the recording streams' log line (every 3000 recordings) now says the most c1 stages a recording uploaded. - vk: `program_for` (`vk_shader.cpp`) holds `Pick& pk` across `program_pick`, the one place a reference into a `FlatMap` is held across a possible insert; a comment there says why that is safe (nothing in between inserts into the thread's `picks`: `program_pick` takes the non-recursive `shader_mu` and can't reach `program_for` again). - tests: `tests/test_flat_map.py` (from #2774: `FlatMap` against `std::unordered_map`, and `texture_view_of` over either; here also `reset` against `clear`, and `c1_at` emptied with `reset`); `tests/test_replay_copies.py` (`payload<T>` cut out and run for every length, and the flat `c1_at`); `test_fsr_uniform_ownership.py`, `test_recording_admission.py` and `test_draw_stream_visibility.py` include `flat_map.h`, since they cut `RecordingRes` out of the source. `vk_shader.cpp` changes (the picks' map), so the translator's fingerprint changes: the first start after updating prepares the shaders again (the program cache of the old build is not read). ## Flags and settings None. The change is internal and gives the same results; reverting the commits removes it. ## Measurements Two machines. On the Steam Deck the same changes were measured as part of a combined build, not alone (below). **The evidence is thin, and a measurement of the head is still owed** before this leaves WIP: MP165 rests on two pairs (after round 1 was dropped and rerun), M1152 is one pair inside the noise, the PC is an RTX 5090 rather than a CPU-bound machine, and the Deck saw these changes only inside a combined build. Owed: the head against main, several interleaved runs, on a CPU-bound machine (the Steam Deck), with the replay's "descriptors" and "submit" ms as well as fps; then the test request and the `docs/tested-configurations.md` row. **PC (PC runner, one prepared game folder per arm under `D:\Githubproject\Longshot-bench`, warm-up per folder; experiments.md, "flat caches redone with separate bench folders")** main: e28236b4 (2026-11-12) this PR: opus/replay-flat-caches-measured @ 53a83cca (pushed to the fork so it can be seen: the same five changes on that main). The head has been rebased since (onto fbd4202, after #198 and #107, without conflict in these files) and has the review's `c1_at` reset and comment on top, not measured. Hardware: RTX 5090, Windows 11 (the runner's PC) Scene: Kokiri Forest, the runner's moving route; M1152 = 2048x1152 window; MP165 = the 165 Hz present config Method: 2 interleaved rounds each (MP165 round 1 excluded and rerun as round 2: main's run had outside CPU load, 22 fps for ~20 s); fps and p99 from the runner's frame log, replay times from `vk: a frame's replay time (ms)` lines How to reproduce: the runner's `pcrun.ps1` labels `lsflat` (two folders: main and this PR, each built with `make_game` and shaders prepared) | | main @ e28236b4 | this PR @ 53a83cca | |---|---|---| | MP165 fps (2 pairs) | 67.5, 70.0 | 72.2, 72.0 (**+4.7, +2.0; mean +3.4**) | | MP165 p99 | 22.1 ms | 20.4-21.3 ms | | M1152 fps | 75.0 | 75.4 (+0.4, sd 1.2: noise) | | M1152 p99 | 19.7 ms | 19.3 ms | | replay "descriptors", ms a frame | 1.61-1.81 | 1.54-1.57 | | replay "submit", ms a frame | | −0.3 to −1.1 against main | An earlier pair that swapped only the renderer DLL into one folder is withdrawn: this branch changes the translator's fingerprint, so the swapped DLL dropped the shader cache and prepared in play. **Steam Deck (Deck agent, Hookshot-based user build, the same five changes among others; render-thread `perf`, Kokiri moving route)** Measured in a combined build, not alone, so no fps is claimed for this PR there. The profile of the render thread before and after showed what these changes target: `texture_view_of` 3.1%, `program_for` 2.5% and `graphics_pipeline` 1.5% of the thread were these three maps' lookups; libc `memmove` 6.7% (in part the per-op payload copies); malloc/free about 3.5%. ## Testing **Tested:** - `python -I -B tests/run_all.py`, Linux (Ubuntu 22.04 container, clang 23, the CPM Vulkan headers, `--jobs 12`): this head @ 559f480 and main @ fbd4202 fail the same 4 and nothing else: `test_cpp_preflight.py`, `test_dev_packs.py`, `test_prof_clock.py`, and the byte-compile of every tracked `.py` (no `.git` in the container). - `python -I -B tests/run_all.py`, Windows (llvm-mingw): this head @ 559f480: 454 passed, 5 failed, 72 skipped; main @ fbd4202 fails the same 5 on this PC: `test_fpx_sticky`, `test_bake_portability`, `test_check_third_party`, `test_spawn_emit`, `test_release`. - `tests/test_flat_map.py` and `tests/test_replay_copies.py` pass on both. - In the game: the PC runner's runs above (Windows, RTX 5090, at 53a83cca); the Steam Deck's runs of a build with these changes (Linux, RADV). The head's `c1_at` reset and comment have not run in the game. **Not tested:** - This branch alone on a Steam Deck or another CPU-bound machine: only as part of a combined build. - AMD and Intel GPUs on Windows. **Needs other testers:** - A CPU-bound PC (4-6 cores, a laptop): Kokiri Forest's replay "descriptors" and "submit" ms a frame against main, expecting them lower and fps not lower (request to open) ## Screenshots Nothing on screen. ## Risk and rollback A lookup that answered differently would show as a wrong texture or pipeline; the tests compare `FlatMap` with `std::unordered_map` over random keys and `texture_view_of` over both. Reverting the commits is enough. The first start after the change prepares the shaders again (the translator's fingerprint changes with `vk_shader.cpp`). Memory: `FlatMap` holds its entries in one array. At `handle_hits`' clear threshold (65536 entries) that is 131072 × 56 B ≈ 7.3 MB per recording thread, against about 5 MB of nodes before, with a moment at about 11 MB while it grows (the old array and the new). With several recording threads (`RX_VK_RECORDER=thread`, the parallel workers) that adds up to a few MB more on the 8 GB / Steam Deck tier. `c1_at` keeps its array between recordings (its largest size so far, logged). ## Reviewer notes Read `gpu/vk/flat_map.h` first, then the three map declarations in `vk_draw.cpp` and `vk_shader.cpp`, then `payload<T>` in `vk_replay.cpp`. ## Issues See https://github.com/NotRexed/Hookshot/pull/2774 (the flat caches, by Corentin Bonjour). ## 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. - [ ] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). (The test request is still to open.) - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. (2 interleaved rounds each at 53a83cca, now on the fork; the head against main on a CPU-bound machine is owed, above.) - [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. - [ ] A row in `docs/tested-configurations.md` for each measured run. (Not added yet.) - [x] A check under `tests/` that reads changed source updated in this PR, not weakened. - [x] Host code builds on both Windows (llvm-mingw) and Linux, with UTF-8 paths.

dukenheimer added 6 commits

2026-11-13 15:53:31 +00:00

perf(vk): keep the per-draw thread caches in a flat table bba39fd22a

handle_hits, picks and graphics_pipeline's mine are searched on every draw
of the replay's render thread, the frame's bottleneck. std::unordered_map
paid a modulo by a prime and a node pointer chase per lookup; FlatMap
(open addressing, linear probing, at most half full) keeps key and value
in one array. HandleHit's tex_epoch takes texture_epoch()'s 32 bits, so a
slot is 56 bytes. tests/test_flat_map.py checks it against
std::unordered_map and texture_view_of over both.

Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 <[email protected]>
(cherry picked from commit 0b8ad723bc42abbdf2c5be959089c1b412019222)

perf(vk): fill the flat caches to 3/4 and give a large array back on clear 60562c1264

At handle_hits' 65536-entry clear threshold the half-full table took
14 MB per recording thread against about 5 MB for the node map, and
clear kept it. Up to 3/4 full it is 7 MB there, and a clear gives back
an array over 4096 slots, so a cache cleared at its limit does not keep
its peak.

Assisted-by: Claude Opus 5.5
Co-Authored-By: Claude Opus 5.5 <[email protected]>
(cherry picked from commit 46f612e2da37d58a3def0f40e4a6d17db2dac1ea)

perf(vk): an op's payload is copied at its type's size, inline, when the op is long enough 52174e1aa0

Co-authored-by: Copilot App <[email protected]>
(cherry picked from commit 788dc480c412e11b679aa5a7e58993c4018079e7)

perf(vk): the recording's c1 map in the flat table: no allocation for each c1 upload kept 1b3eb36266

(cherry picked from commit fc9a88e8 in Hookshot's Deck branch; Longshot has no uniform snapshot map, so only c1_at)

Co-authored-by: Copilot App <[email protected]>

fix(tests): test_flat_map finds Longshot's comment marker f3bca42a9f

Co-authored-by: Copilot App <[email protected]>

fix(tests): the checks that cut RecordingRes include flat_map.h

PR description / Description follows the template (pull_request_target) Successful in 16s

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled

Details

CI / Workflow lint (actionlint) (pull_request) Has been cancelled

Details

Third-party parts / As their notes say (pull_request) Has been cancelled

Details

Third-party parts / The same as upstream (pull_request) Has been cancelled

Details

8ca9aa0423

Assisted-by: Claude Opus 5.5

Co-Authored-By: Claude Opus 5.5 <[email protected]>

HumanBean requested changes

2026-11-13 16:18:43 +00:00

HumanBean left a comment

Read-only review of 8ca9aa04 against main@origin (1ef93e79). Nothing built or run.

What's good

FlatMap is correct as far as I can trace it. The load factor stays at or below 3/4, so a probe always reaches an empty slot. shift_ is right for every power-of-two size. find checks count_ before it hashes, so the shift of 64 on an empty table is never evaluated. A failed allocation in grow() throws before the swap and leaves the map as it was. The hash is multiplicative and takes the top bits, so the zero low bits of c1_at's pointer keys and the slot/want/use/format fields of handle_hits' keys spread well.
The behaviour matches main. Nothing iterates these maps (there is no begin(), so it wouldn't compile), and the clear thresholds haven't moved. tex_epoch going to 32 bits matches uint32_t texture_epoch(), and the reordered HandleHit initializer at its only use matches the new field order.
- Stale entries: pipelines is never erased, so mine's PipeEntry* can't dangle. handle_hits still checks version and destroy_gen before it trusts tex.
- Threads: all four maps belong to one thread (thread_local, TLS_OBJECT, or reached through the thread-local cur), as on main, so RX_VK_RECORDER=thread and the parallel workers don't share them.
References across an insert: FlatMap moves its entries when it grows, where unordered_map didn't. Only one place holds a reference across a possible insert: Pick& pk = picks[key] is held across program_pick. That is safe, because program_pick takes the non-recursive shader_mu and can't reach program_for again. In texture_view_of and uniform_range, the found slot isn't used after the operator[] write.
payload<T> does the same thing as before: it never reads past bytes, and memcpy needs no alignment.
Conflicts: none with #198 (its vk_draw.cpp hunks are all at lines 3814 and later, away from these; its one-line change to recording_admission_test.cpp builds with the -include flat_map.h added here). None with #216 (it changes only vk_memory.cpp).
The checks that cut RecordingRes get the -include. test_handle_lookup, test_same_draw, test_pick_tex_gen and test_mem_trim still find their markers. test_flat_map.py is a good differential check.

Must change (before it leaves WIP)

"Rebased on current main" is ticked but isn't true. The fork point is fdc2840b, and main has 08771847 and 1ef93e79 on top. Neither touches these files, so a rebase is trivial. Rebase, or untick the box.
The measurement is of a commit nobody else can see. The PC numbers are for 53a83cca, which isn't on the forge, and the head has been rebased since. The evidence is also thin:
- MP165 rests on two pairs, after round 1 was dropped and rerun.
- M1152 is one pair, inside the noise (+0.4 fps, sd 1.2).
- The PC is an RTX 5090, not a CPU-bound machine.
- On the Deck, the change was only measured as part of a combined build.
The Measured box is rightly unticked. Before pr ready, measure the head (or a pushed commit) against main with several interleaved runs, ideally on a CPU-bound machine, and report the replay "descriptors"/"submit" ms as well as fps. Then open the test request and add the docs/tested-configurations.md row (both boxes are still open).

Should change

Say in the code why pk in program_for is safe to hold across program_pick. The header says no reference may be held across an insert, and this is the only place that does it. A future callback from program_pick into the draw path would turn it into a use-after-free that the old map would have survived.
c1_at is cleared at every recording start, and clear() gives back any array over 4096 slots. A recording with more than 3072 distinct c1 stages would therefore free its table and grow it again from 16 every recording: about 9 reallocations and rehashes, a cost the old map didn't have in that form. Below that size, a clear walks up to 4096 slots. Both are probably small for OoT. Logging c1_at.size()'s peak once would settle it, or c1_at could take a clear that keeps its capacity.
Memory: at the 65536-entry clear threshold, handle_hits holds 131072 × 56 B ≈ 7.3 MB per recording thread, against about 5 MB before (as the commit message says), with a moment at about 11 MB while it grows. That's fine, but worth a line under Risk for the 8 GB/Deck tier when several recording threads are running.

Read-only review of 8ca9aa04 against main@origin (1ef93e79). Nothing built or run. **What's good** - `FlatMap` is correct as far as I can trace it. The load factor stays at or below 3/4, so a probe always reaches an empty slot. `shift_` is right for every power-of-two size. `find` checks `count_` before it hashes, so the shift of 64 on an empty table is never evaluated. A failed allocation in `grow()` throws before the swap and leaves the map as it was. The hash is multiplicative and takes the top bits, so the zero low bits of `c1_at`'s pointer keys and the slot/want/use/format fields of `handle_hits`' keys spread well. - The behaviour matches main. Nothing iterates these maps (there is no `begin()`, so it wouldn't compile), and the clear thresholds haven't moved. `tex_epoch` going to 32 bits matches `uint32_t texture_epoch()`, and the reordered `HandleHit` initializer at its only use matches the new field order. - **Stale entries:** `pipelines` is never erased, so `mine`'s `PipeEntry*` can't dangle. `handle_hits` still checks `version` and `destroy_gen` before it trusts `tex`. - **Threads:** all four maps belong to one thread (`thread_local`, `TLS_OBJECT`, or reached through the thread-local `cur`), as on main, so `RX_VK_RECORDER=thread` and the parallel workers don't share them. - **References across an insert:** `FlatMap` moves its entries when it grows, where `unordered_map` didn't. Only one place holds a reference across a possible insert: `Pick& pk = picks[key]` is held across `program_pick`. That is safe, because `program_pick` takes the non-recursive `shader_mu` and can't reach `program_for` again. In `texture_view_of` and `uniform_range`, the found slot isn't used after the `operator[]` write. - `payload<T>` does the same thing as before: it never reads past `bytes`, and `memcpy` needs no alignment. - **Conflicts:** none with https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/198 (its `vk_draw.cpp` hunks are all at lines 3814 and later, away from these; its one-line change to `recording_admission_test.cpp` builds with the `-include flat_map.h` added here). None with https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/216 (it changes only `vk_memory.cpp`). - The checks that cut `RecordingRes` get the `-include`. `test_handle_lookup`, `test_same_draw`, `test_pick_tex_gen` and `test_mem_trim` still find their markers. `test_flat_map.py` is a good differential check. **Must change (before it leaves WIP)** - **"Rebased on current `main`" is ticked but isn't true.** The fork point is fdc2840b, and main has 08771847 and 1ef93e79 on top. Neither touches these files, so a rebase is trivial. Rebase, or untick the box. - **The measurement is of a commit nobody else can see.** The PC numbers are for `53a83cca`, which isn't on the forge, and the head has been rebased since. The evidence is also thin: - MP165 rests on two pairs, after round 1 was dropped and rerun. - M1152 is one pair, inside the noise (+0.4 fps, sd 1.2). - The PC is an RTX 5090, not a CPU-bound machine. - On the Deck, the change was only measured as part of a combined build. The *Measured* box is rightly unticked. Before `pr ready`, measure the head (or a pushed commit) against main with several interleaved runs, ideally on a CPU-bound machine, and report the replay "descriptors"/"submit" ms as well as fps. Then open the test request and add the `docs/tested-configurations.md` row (both boxes are still open). **Should change** - Say in the code why `pk` in `program_for` is safe to hold across `program_pick`. The header says no reference may be held across an insert, and this is the only place that does it. A future callback from `program_pick` into the draw path would turn it into a use-after-free that the old map would have survived. - **`c1_at` is cleared at every recording start, and `clear()` gives back any array over 4096 slots.** A recording with more than 3072 distinct c1 stages would therefore free its table and grow it again from 16 every recording: about 9 reallocations and rehashes, a cost the old map didn't have in that form. Below that size, a clear walks up to 4096 slots. Both are probably small for OoT. Logging `c1_at.size()`'s peak once would settle it, or `c1_at` could take a clear that keeps its capacity. - **Memory:** at the 65536-entry clear threshold, `handle_hits` holds 131072 × 56 B ≈ 7.3 MB per recording thread, against about 5 MB before (as the commit message says), with a moment at about 11 MB while it grows. That's fine, but worth a line under Risk for the 8 GB/Deck tier when several recording threads are running.

gpu/vk/flat_map.h Outdated

					
				@ -0,0 +38,4 @@

				    void clear() {

				        if (!count_) return;

				        count_ = 0;

				        if (slots_.size() > kKeptSlots) {   // (a cache cleared at its size limit: its memory goes back too)

This suits the caches cleared at their size limit. RecordingRes::c1_at, though, is cleared at every recording start: a recording with more than 3072 c1 stages would free its table and grow it again from 16 (about 9 reallocations and rehashes) every recording. Below that size the clear walks up to 4096 slots. Probably small for OoT. Do you have c1_at's peak size? If it can get near this, c1_at could take a clear that keeps its capacity.

This suits the caches cleared at their size limit. `RecordingRes::c1_at`, though, is cleared at every recording start: a recording with more than 3072 c1 stages would free its table and grow it again from 16 (about 9 reallocations and rehashes) every recording. Below that size the clear walks up to 4096 slots. Probably small for OoT. Do you have `c1_at`'s peak size? If it can get near this, `c1_at` could take a clear that keeps its capacity.

gpu/vk/vk_shader.cpp Outdated

					
				@ -1731,3 +1732,3 @@

				    std::vector<uint32_t> handles;   // (at its answers' descriptors: their slots and the slots' versions)

				};

				thread_local std::unordered_map<uint64_t, Pick> picks;   // (each recording thread's: program_pick has its own lock)

				thread_local FlatMap<uint64_t, Pick> picks;   // (each recording thread's: program_pick has its own lock)

program_for holds Pick& pk = picks[key] across program_pick(...) and writes through it afterwards. With FlatMap, an insert into picks during that call would move the entry and leave pk dangling, where unordered_map kept references stable. It's safe today, because program_pick takes the non-recursive shader_mu and never gets back to program_for. Please add a line here or at pk saying so, since flat_map.h's own rule is that no reference is held across an insert.

`program_for` holds `Pick& pk = picks[key]` across `program_pick(...)` and writes through it afterwards. With `FlatMap`, an insert into `picks` during that call would move the entry and leave `pk` dangling, where `unordered_map` kept references stable. It's safe today, because `program_pick` takes the non-recursive `shader_mu` and never gets back to `program_for`. Please add a line here or at `pk` saying so, since flat_map.h's own rule is that no reference is held across an insert.

dukenheimer force-pushed opus/replay-flat-caches from 8ca9aa0423

PR description / Description follows the template (pull_request_target) Successful in 16s

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled

Details

CI / Workflow lint (actionlint) (pull_request) Has been cancelled

Details

Third-party parts / As their notes say (pull_request) Has been cancelled

Details

Third-party parts / The same as upstream (pull_request) Has been cancelled

Details

to 4727116511

Website / Website on Cloudflare (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 52s

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled

Details

CI / Workflow lint (actionlint) (pull_request) Has been cancelled

Details

Third-party parts / As their notes say (pull_request) Has been cancelled

Details

Third-party parts / The same as upstream (pull_request) Has been cancelled

Details

2026-11-13 17:17:31 +00:00

Compare

dukenheimer force-pushed opus/replay-flat-caches from 4727116511

Website / Website on Cloudflare (pull_request) Blocked by required conditions

Details

PR description / Description follows the template (pull_request_target) Successful in 52s

Details

CI / Conventional Commit title (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 1/2) (pull_request) Has been cancelled

Details

CI / Python source checks (ubuntu-24.04, part 2/2) (pull_request) Has been cancelled

Details

CI / SPIR-V fixtures (global narrow load/store, direct surface atomics, F2I immediate) (pull_request) Has been cancelled

Details

CI / Workflow lint (actionlint) (pull_request) Has been cancelled

Details

Third-party parts / As their notes say (pull_request) Has been cancelled

Details

Third-party parts / The same as upstream (pull_request) Has been cancelled

Details

to 559f48074b

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) Successful in 3s

Details

2026-11-13 17:58:28 +00:00

Compare

dukenheimer commented

2026-11-13 17:58:36 +00:00

First-time contributor

Thanks for the review. Point by point, at 559f480 (rebased on main fbd4202):

Must change

Rebased: the branch is now on current main fbd4202 (after #198, #107 and the rest), with no conflicts, so the box is true. Suites below.
Measurement of a commit nobody can see: 53a83cca is pushed to the fork as opus/replay-flat-caches-measured, and the description names it. The description now says plainly how thin the evidence is: two MP165 pairs after a rerun, one M1152 pair inside the noise, an RTX 5090, and the Deck only inside a combined build. Measured stays unticked. Still owed before pr ready: this head against main with several interleaved runs on a CPU-bound machine (the Steam Deck), with the replay's "descriptors" and "submit" ms as well as fps, then the test request and the docs/tested-configurations.md row. I don't run the game myself, so I've asked for those runs.

Should change

pk across program_pick: there is now a comment at Pick& pk = picks[key] (gpu/vk/vk_shader.cpp, in 559f480). It says nothing between the lookup and the write inserts into this thread's picks: program_pick takes the non-recursive shader_mu and can't reach program_for again. A future callback from it into the draw path would have to look pk up again.
c1_at cleared at every recording: FlatMap::reset() empties the map and keeps its array whatever its size, and c1_at uses it at each recording's start (clear() still gives a big array back for the caches cleared at their size limit). I don't have a peak size yet, so the recording streams' log line (every 3000 recordings) now ends with c1 stages: at most N a recording, and the next run will show it. tests/test_flat_map.py checks reset against clear (capacity kept or given back, then refilled) and alternates the two in the differential against std::unordered_map; its source check requires r.c1_at.reset().
Memory: there is a paragraph under Risk: ~7.3 MB per recording thread at handle_hits' threshold against ~5 MB before, ~11 MB for a moment while it grows, a few MB more on the 8 GB / Deck tier with several recording threads.

Tests (python -I -B tests/run_all.py): on Linux, 559f480 and main @ fbd4202 fail the same 4 (test_cpp_preflight, test_dev_packs, test_prof_clock, the byte-compile without .git); on Windows, 454 passed and the same 5 failed as on main (test_fpx_sticky, test_bake_portability, test_check_third_party, test_spawn_emit, test_release).

Thanks for the review. Point by point, at 559f480 (rebased on main fbd4202): **Must change** - **Rebased:** the branch is now on current main fbd4202 (after #198, #107 and the rest), with no conflicts, so the box is true. Suites below. - **Measurement of a commit nobody can see:** 53a83cca is pushed to the fork as `opus/replay-flat-caches-measured`, and the description names it. The description now says plainly how thin the evidence is: two MP165 pairs after a rerun, one M1152 pair inside the noise, an RTX 5090, and the Deck only inside a combined build. *Measured* stays unticked. Still owed before `pr ready`: this head against main with several interleaved runs on a CPU-bound machine (the Steam Deck), with the replay's "descriptors" and "submit" ms as well as fps, then the test request and the `docs/tested-configurations.md` row. I don't run the game myself, so I've asked for those runs. **Should change** - **`pk` across `program_pick`:** there is now a comment at `Pick& pk = picks[key]` (`gpu/vk/vk_shader.cpp`, in 559f480). It says nothing between the lookup and the write inserts into this thread's `picks`: `program_pick` takes the non-recursive `shader_mu` and can't reach `program_for` again. A future callback from it into the draw path would have to look `pk` up again. - **`c1_at` cleared at every recording:** `FlatMap::reset()` empties the map and keeps its array whatever its size, and `c1_at` uses it at each recording's start (`clear()` still gives a big array back for the caches cleared at their size limit). I don't have a peak size yet, so the recording streams' log line (every 3000 recordings) now ends with `c1 stages: at most N a recording`, and the next run will show it. `tests/test_flat_map.py` checks `reset` against `clear` (capacity kept or given back, then refilled) and alternates the two in the differential against `std::unordered_map`; its source check requires `r.c1_at.reset()`. - **Memory:** there is a paragraph under Risk: ~7.3 MB per recording thread at `handle_hits`' threshold against ~5 MB before, ~11 MB for a moment while it grows, a few MB more on the 8 GB / Deck tier with several recording threads. **Tests** (`python -I -B tests/run_all.py`): on Linux, 559f480 and main @ fbd4202 fail the same 4 (`test_cpp_preflight`, `test_dev_packs`, `test_prof_clock`, the byte-compile without `.git`); on Windows, 454 passed and the same 5 failed as on main (`test_fpx_sticky`, `test_bake_portability`, `test_check_third_party`, `test_spawn_emit`, `test_release`).

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) Successful in 3s

Details

This pull request is marked as a work in progress.

Some workflows are waiting to be reviewed.

This branch is out-of-date with the base branch

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u 'opus/replay-flat-caches:dukenheimer-opus/replay-flat-caches'

git switch 'dukenheimer-opus/replay-flat-caches'

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

MagicBeanDev/Longshot!231

Rows
Columns