Vulkan: vk_draw.cpp's per-thread objects without the TLS init check on every use #1035

Merged

jordanella merged 8 commits from render-cpu-fixes into main

2026-11-09 15:52:07 +00:00

jordanella commented

2026-11-09 07:25:42 +00:00

(Migrated from github.com)

With llvm-mingw, every use of a thread_local whose type has a constructor or destructor calls the file's TLS init function. That call saves registers, finds the thread's TLS block and checks a guard byte, however long ago the thread set the object up. vk_draw.cpp has twelve such objects (dw, pass, handle_hits, pushed, bank_cache, ...), used dozens of times per draw. Found with the sampling profiler (HOS_SAMPLE=Presentation + scripts/prof.py): ~2.4% of the replay thread.

They are now TLS_OBJECT(name, type). name() returns the calling thread's object through a plain thread_local pointer, which needs no init function. Each object is made on the thread's first use and deleted when the thread ends. Only the list that owns them has a destructor, and only a first use touches it. The change at the use sites is mechanical (dw. becomes dw().). Because the names are now functions, a use left unchanged fails to compile instead of quietly paying the check again. tests/test_tls_objects.py fails if a namespace-scope thread_local in vk_draw.cpp has a container type, or a struct type of the file holding one.

This PR first also read RX_VK_TAAU_KEYS once instead of on every dispatch. Main has since fixed that in df1e26d (the lookup after the stand-in's early returns), so that part is dropped in favour of main's. tests/test_taau_env_lookup.py's fixture now has dw as a call, like vk_draw.cpp, and so do test_handle_lookup.py, test_pass_hazard.py and test_push_state.py, which read the source.

Measured on a Ryzen 9 3900X with an RTX 3080 on Windows 11, on main 8b2e1d3 (v2.0.8-rc.1) with this branch merged in. Kokiri Forest after loading a save, headless, input timed in game frames (HOS_INPUT_FRAME_CLOCK=1), live numbers over presents 3120-5520, two runs each, in two rounds (the second in reverse order):

build	standing fps	replay per frame	panning fps
main	36.8 / 36.3	22.5 ms	33.9 / 34.9
this branch	37.1 / 37.2 (+1.7%)	21.9 ms	35.0 / 34.9 (+1.7%)

On v2.0.5 the profile showed the init function gone (0.00% of the replay thread, from 2.38%).

python -I -B tests/run_all.py on main 8b2e1d3 with all three of my open PRs merged: the same 12 failures as main itself on this machine (missing tools, and an older g++ that rejects volatile compound assignment), and every other test passes.

🤖 Generated with Claude Code

With llvm-mingw, every use of a `thread_local` whose type has a constructor or destructor calls the file's TLS init function. That call saves registers, finds the thread's TLS block and checks a guard byte, however long ago the thread set the object up. `vk_draw.cpp` has twelve such objects (`dw`, `pass`, `handle_hits`, `pushed`, `bank_cache`, ...), used dozens of times per draw. Found with the sampling profiler (`HOS_SAMPLE=Presentation` + `scripts/prof.py`): ~2.4% of the replay thread. They are now `TLS_OBJECT(name, type)`. `name()` returns the calling thread's object through a plain `thread_local` pointer, which needs no init function. Each object is made on the thread's first use and deleted when the thread ends. Only the list that owns them has a destructor, and only a first use touches it. The change at the use sites is mechanical (`dw.` becomes `dw().`). Because the names are now functions, a use left unchanged fails to compile instead of quietly paying the check again. `tests/test_tls_objects.py` fails if a namespace-scope `thread_local` in `vk_draw.cpp` has a container type, or a struct type of the file holding one. This PR first also read `RX_VK_TAAU_KEYS` once instead of on every dispatch. Main has since fixed that in df1e26d (the lookup after the stand-in's early returns), so that part is dropped in favour of main's. `tests/test_taau_env_lookup.py`'s fixture now has `dw` as a call, like `vk_draw.cpp`, and so do `test_handle_lookup.py`, `test_pass_hazard.py` and `test_push_state.py`, which read the source. Measured on a Ryzen 9 3900X with an RTX 3080 on Windows 11, on main 8b2e1d3 (v2.0.8-rc.1) with this branch merged in. Kokiri Forest after loading a save, headless, input timed in game frames (`HOS_INPUT_FRAME_CLOCK=1`), live numbers over presents 3120-5520, two runs each, in two rounds (the second in reverse order): | build | standing fps | replay per frame | panning fps | |---|---|---|---| | main | 36.8 / 36.3 | 22.5 ms | 33.9 / 34.9 | | this branch | 37.1 / 37.2 (+1.7%) | 21.9 ms | 35.0 / 34.9 (+1.7%) | On v2.0.5 the profile showed the init function gone (0.00% of the replay thread, from 2.38%). `python -I -B tests/run_all.py` on main 8b2e1d3 with all three of my open PRs merged: the same 12 failures as main itself on this machine (missing tools, and an older g++ that rejects `volatile` compound assignment), and every other test passes. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

hckrman101 (Migrated from github.com) reviewed

2026-11-09 08:16:22 +00:00

hckrman101 (Migrated from github.com) left a comment

I don't see a correctness problem: the ::mkwvk:: names resolve to the unnamed-namespace thread-locals, each reference is bound per call on the calling thread (the recursive bind_descriptors call binds again), and the static keys is fine. My comments are about the approach and coverage rather than bugs.

I don't see a correctness problem: the `::mkwvk::` names resolve to the unnamed-namespace thread-locals, each reference is bound per call on the calling thread (the recursive `bind_descriptors` call binds again), and the `static` `keys` is fine. My comments are about the approach and coverage rather than bugs.

gpu/vk/vk_draw.cpp

Same in build_descriptors: dump_written.clear() / dump_inputs.clear() here run unconditionally through the wrapper even with dumping off, and bank_cache via uniform_range is the hottest remaining TLS access in this function. The "thread-locals once" comment above is only half true as it stands.

Same in `build_descriptors`: `dump_written.clear()` / `dump_inputs.clear()` here run unconditionally through the wrapper even with dumping off, and `bank_cache` via `uniform_range` is the hottest remaining TLS access in this function. The "thread-locals once" comment above is only half true as it stands.

nit: the locals shadow the thread-locals with the same names (dw, draw_sampled, and handle_hits at 1491), so if one of these bindings is ever removed the code still compiles against the thread-local and the slowdown comes back silently (and -Wshadow warns on each). A distinct name like DescWriter& w = dw;, or the struct above, makes it explicit. The three sites also spell it differently (auto& vs the full type).

nit: the locals shadow the thread-locals with the same names (`dw`, `draw_sampled`, and `handle_hits` at 1491), so if one of these bindings is ever removed the code still compiles against the thread-local and the slowdown comes back silently (and `-Wshadow` warns on each). A distinct name like `DescWriter& w = dw;`, or the struct above, makes it explicit. The three sites also spell it differently (`auto&` vs the full type).

gpu/vk/vk_draw.cpp

The underlying cost is that every namespace-scope thread_local with a non-trivial constructor/destructor goes through the TLS init wrapper on each use, and binding a reference at the top of three functions fixes just those three. Other hot users still pay it: uniform_range's bank_cache (once per uniform binding per draw, ~1584), gfx_sampled = draw_sampled in the draw path (~4809), draw_hazards, and so on. The next profile will likely want the same patch in more places. Putting the per-thread descriptor state in one struct reached through a trivially initialised thread_local Tls* tls = nullptr; (allocated on first use) would remove the wrapper for every user in one go. What do you think?

The underlying cost is that every namespace-scope `thread_local` with a non-trivial constructor/destructor goes through the TLS init wrapper on each use, and binding a reference at the top of three functions fixes just those three. Other hot users still pay it: `uniform_range`'s `bank_cache` (once per uniform binding per draw, ~1584), `gfx_sampled = draw_sampled` in the draw path (~4809), `draw_hazards`, and so on. The next profile will likely want the same patch in more places. Putting the per-thread descriptor state in one struct reached through a trivially initialised `thread_local Tls* tls = nullptr;` (allocated on first use) would remove the wrapper for every user in one go. What do you think?

pushed is a thread-local with vector members too, so this line still goes through the wrapper (twice on the fallback path that re-enters bind_descriptors), one line below the dw it just fixed.

`pushed` is a thread-local with vector members too, so this line still goes through the wrapper (twice on the fallback path that re-enters `bind_descriptors`), one line below the `dw` it just fixed.

gpu/vk/vk_draw.cpp

minor: this now reads the env once, but it still duplicates taau_key's RX_VK_TAAU_KEYS lookup with a different default (fd029a317a9a4b94 here vs - there), and the static guard is checked before the taau_key early return on every dispatch. One shared taau_keys() helper, read after the early return, would remove both.

minor: this now reads the env once, but it still duplicates `taau_key`'s `RX_VK_TAAU_KEYS` lookup with a different default (`fd029a317a9a4b94` here vs `-` there), and the static guard is checked before the `taau_key` early return on every dispatch. One shared `taau_keys()` helper, read after the early return, would remove both.