Repository navigation
fix(vk): bound precompile thread concurrency via half_cpus (#1593) - #1610
oscarlosu-lab wants to merge 1 commit into
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
Looks good. The change is small and correct: cpus/2 clamped to 1..16 by default, with an override env var, and the existing 64 cap still applies. CI is green.
Nits, none blocking:
- The description says this matches predict_run()/state_list_run(), but those use clamp(cpus / 2, 1, 32), not 16. Fine as a deliberately lower cap for the heavier compile pass, but the wording could say so.
- HOOKSHOT_JOBS is the build-time jobs variable (builder / make_game.py), so it will now also throttle runtime precompile for anyone who exports it globally. Probably what you want here, but worth a line in the docs next to RX_VK_PRECOMPILE_JOBS.
- atoi on a garbage value falls back to the default, which is fine.
|
Actually maybe we should do CPU*0.75? |
|
Maybe a slider in builder too like RAM would be good |
|
Personally, I prefer that certain things be hardcoded and that the end user doesn't have so many controls that could generate instability, something more typical of a product that goes into production, I think the best approach would be to make a game that is compatible with all systems without each user having to adjust it. |
…on found nothing on fresh builds; no 90 s budget; one progress bar) (#2032) The pipelines the prediction makes are meant to be built before the game. On a fresh build, though, none were predicted at all. Where they were, a 90 s budget left most of them to be compiled in play, each one a stall on its first draw. This PR builds every predicted pipeline in the preparation, on a progress bar that counts them from the start. ## What changes 1. **fix(vk): the prediction sees the programs the preparation read in.** - `program_cache_load` and `program_cache_load_some` take a key's records out of `disk_records` when they read a program in. - The preparation reads in every program the state list names (`state_list_prepare`) just before `predict_run`. - `program_cache_each` listed `disk_records` alone, so no note found its program. Every new build logged `predicted 0 pipelines from 1261 notes for 0 program variants`. - The records taken out are now kept in `disk_taken`, and `program_cache_each` lists both. - A played folder whose start did not run `state_list_prepare` (for example, a start after a driver update) predicted fine, which is why this went unnoticed. 2. **fix(vk): build every predicted pipeline in the preparation, on one progress bar.** - **No budget by default.** `RX_VK_PREDICT_S` no longer defaults to 90 s; setting it still sets a budget. - **Every CPU, normal priority.** The workers now use every CPU at normal priority, as `precompile_run` already does; nothing else runs before the game. They used half the CPUs below normal priority. (#1610's job limit would apply here the same way.) - **Closing the window stops them**, through `prepare_stopped()`. What the driver has built is kept. - **One bar.** The start-up screen and the builder's `--prepare-shaders` bar now cover the programs, the predicted pipelines and the noted pipelines together (`PrepareBar`). The total holds all three from the start. - Until a part counts itself, it stands at a guess from the files on disk (`prepare_guess`: the seed's program count, the notes and the lists). - A pipeline weighs as two programs, the measured ratio. - The bar never goes back and never reads 100% while pipelines are still being built. - Before, the screen showed a second bar after a full one, and the builder gave the predicted pipelines a fixed 20% share. - **Log.** It now says how long deciding the predictions took, separately from building them. ## Numbers Setup: RTX 3090, Ryzen 9 7900X (24 threads), Linux, OoT 1.1.1, an empty NVIDIA shader cache, and the shipped 1,261-pipeline list. | Case | Predicted | Built in the preparation | Time | |---|---|---|---| | main, fresh build | 0 (the bug) | 0 | prepare 116 s with the program cache present | | main, played folder after a driver update | 43,406-45,882 | 8,634-10,858 (90 s budget, 12 threads) | the rest in play, up to 205 draws a frame left out | | this PR with `RX_VK_PREDICT_S=90` | 43,414 | 13,941 | 93 s | | **this PR** | **43,414** | **43,414** | **7-11 s deciding + 306-320 s building** | - **A whole cold preparation takes 457-581 s.** - 457 s from nothing: translation 131 s, prediction 313 s, noted pipelines 10 s. - 581 s with the programs already translated but no compute list, so every compute pipeline was built. - **A second preparation took 96 s**, with the driver cache kept from the first. - **Peak memory was 4.3 GB** for the process. ## Tests - `tests/test_predict_sees_loaded.py` (new): checks that both take-out sites keep what they take, and that `program_cache_each` lists those records too. - `tests/test_startup_progress.py` covers `PrepareBar`: - The finished programs leave room on the bar for the pipelines. - The bar holds while the prediction decides. - A part's count replaces its guess without the bar going back. - The bar stays under 1 until the last part is done. - A start with nothing to prepare fills the bar with its pipelines alone. - The screen shows the whole bar's share. - It also checks the source: `predict_run` has no default budget, and does not use half the CPUs or lower its priority. - `tests/test_prepare_cache_save.py` and `tests/test_program_cache_cleanup.py`: stubs updated. Not tested: the Windows start-up screen and Windows builder bar (the logic is shared and tested here), and AMD or Intel GPUs. --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
Tested on an Intel UHD 770 (Raptor Lake-S, no mesh shaders), Mesa 26.2.5 ANV, Arch Linux, i9-13900K (32 threads). I merged this into main 872cb68 and ran it headless with the game capped at 32 GB. On this machine it doesn't get the start-up precompile through. Same save-load scene and the same caches for both runs, start-up precompile on:
With this PR the game's memory sat at 9.6-15 GB for two minutes. Then it went to 20 GB and 30 GB in the last 30 s. The worker count isn't what decides it here. A few compute programs are about 250,000 SPIR-V words each ( So this cap may well be right for RADV (#1593). On Intel it only changes how soon the OOM kill happens. What would help there is keeping the very large compiles from running at the same time, or leaving them out of the precompile. I haven't tested this on RADV. |
|
Thanks a lot for testing this thoroughly on Intel ANV and sharing the exact numbers and shader keys ( Your 4-worker preparation run confirms that thread-count clamping alone cannot prevent ANV OOM on oversized compute shaders (~250k SPIR-V words): because they appear back-to-back in I'd like to keep this PR ( |
|
Sounds good to me: #1610 for the worker cap, and the very large compiles in their own PR. I have a local branch on a similar idea, but it only changes builds in play, and I'm not offering it as it is. On Intel, main already builds every compute pipeline in play one at a time (#496). The branch records each stage's SPIR-V word count and keeps only programs with a stage of 100,000 words or more on that thread. The others go to the shared workers (4 on this machine). It doesn't touch On the UHD 770 it didn't work out. Both runs started with no caches, with the shader preparation and the start-up precompile switched off (
More compute work ran with the branch. But the smaller builds on the shared workers, next to the one big build, still took the game to the cap. A second run of the branch, with #2110 merged as well, was killed after 306 s. The picture was wrong too, blown out to white with coloured bands. Main's picture on this GPU is also wrong, but in a different way. I haven't found the reason, so the branch stays local. What our runs say about a threshold:
So your idea and our branch do different things. Your lock covers the precompile and the shader preparation, which our branch leaves alone. Our branch only changes builds in play on Intel, where main already builds every compute pipeline one at a time, and lets the small ones run side by side. Both use a word count. I can't say yet how far one of these compiles grows by the time it finishes, because it was still growing when my run ended. When your follow-up is up, I'll run it on the UHD 770 with the same scene and post the numbers. Added 2026-11-11 16:46 UTC: I have since found why the branch's picture was wrong; a frame dump shows it. The four shared workers also translate the graphics shaders, and with the branch they were busy with compute builds. Eight of those took 25 to 61 s each (their largest stages are about 28,000 to 66,000 words; the machine was busy, load average 100 to 120). From the save load to the end of the run the queue stayed at 449-516 jobs and shader translation hardly moved: 2,846 ms in all when the save loaded, 2,921 ms at the end. Near the end, 2,350-2,390 draws a frame were still left out because their shaders weren't ready. The dumped frame has 230 draws with the branch and 1,170 without it, so the world wasn't drawn and only the sky reached the upscale. The coloured bands are main's own false colours on this GPU, in both runs. For a threshold this adds: in play, compute compiles of that size already block everything that waits behind them on shared workers here, well below 100,000 words. |
7ffe45b to
6531a7c
Compare
Joey-Magnus
left a comment
There was a problem hiding this comment.
Main still sizes the precompile pool as min(usable_cpus(), 64) (vk_draw.cpp:8084), so the cap isn't on main yet. But the conflict is more than text: #2323 replaced hardware_concurrency() with usable_cpus()/half_cpus(), which honour process affinity on Windows. A rebase should build on half_cpus() rather than going back to hardware_concurrency().
Two things decide whether this goes in at all:
- #2414 (open) caps the same pools at physical cores,
precompile_runincluded, so taking both would stack two policies. One of them should go. - mooki000's UHD 770 numbers show the 16-worker cap only delays the OOM kill (124 s to 199 s). The trigger there is the ~250k-word compute programs compiling side by side, which the follow-up you mentioned would address.
Smaller point: HOOKSHOT_JOBS is the build's jobs variable, so reusing it throttles play for anyone who exports it globally. I'd keep only RX_VK_PRECOMPILE_JOBS.
6531a7c to
e3a158b
Compare
|
Thanks for the review, @Joey-Magnus! Updated in
As noted, #2414 vs this |
hckrman101
left a comment
There was a problem hiding this comment.
Re-review at e3a158b. Both of my earlier nits are handled: HOOKSHOT_JOBS is gone, so only RX_VK_PRECOMPILE_JOBS throttles the runtime pass, and the description now says the 16 cap is deliberately lower than the 32 used by the other pools. Joey-Magnus's rebase point is done too. The default is now clamp(half_cpus(), 1, 16), so it follows process affinity like the other pools since #2323, and test_prepare_cpus.py checks for half_cpus() in precompile_run. The code looks correct to me: a garbage or zero override falls back to the default, and the override is still capped at 64. Mergeable, and CI is green except the builder package job, which is still pending.
The one open question is the one Joey raised: #2414 (still open) caps the same precompile_run pool with a different policy (physical cores through compile_worker_count()) and edits the same lines. Its test_compile_threads.py also expects precompile_run to use that helper. So only one of the two can go in as written, and the other would need reworking. Choosing between them is a policy decision for @NotRexed, so I'm leaving this as a comment rather than an approval.
For the record, mooki000's UHD 770 runs show this cap only delays the OOM on ANV (124 s to 199 s). The cause there is the very large compute programs compiling side by side. The author has scoped that to a follow-up, and mooki000 agreed, so I don't count it against this PR. The description should just not suggest this fixes Intel.
Nit (inline): the new variable isn't documented anywhere.
| const unsigned cpus = usable_cpus(); | ||
| for (unsigned i = 0; i < std::min(cpus, 64u); i++) threads.emplace_back(worker); | ||
| const char* env_jobs = getenv("RX_VK_PRECOMPILE_JOBS"); | ||
| const unsigned workers = env_jobs && atoi(env_jobs) > 0 ? (unsigned)atoi(env_jobs) : std::clamp(half_cpus(), 1u, 16u); |
There was a problem hiding this comment.
Nit: RX_VK_PRECOMPILE_JOBS is the only knob for this pool now, but nothing mentions it. A word in the comment above Pre, next to RX_VK_PRECOMPILE_STALL_S, would help. A one-line logf of the worker count would also make tester logs easier to compare (mooki000 had to infer it). Calling atoi once into a local would also read better.
e3a158b to
9c17e2b
Compare
|
Rebased onto eaef146 in 9c17e2b. hckrman101's inline nit is in: The description now says what this does and does not cover: it bounds concurrent compiles (the #1593 RADV case) and does not fix the Intel ANV OOM from the ~250k-word compute programs, which needs those compiles serialized on their own (follow-up). On #2414: it caps the pools by physical cores and keeps every smaller stage ceiling, so this PR's 16-worker clamp would sit below its cap. They compose; which policy you want as the default is your call. |
|
Note on the current CI failures on
Happy to rebase cleanly once those are fixed on |
Joey-Magnus
left a comment
There was a problem hiding this comment.
The rebase onto half_cpus() is done and HOOKSHOT_JOBS is gone, so my earlier points are handled. I merged 9c17e2b into current main (clean) and ran test_prepare_cpus and test_env_on, and both pass. The three red Python jobs on this head come from main's breakage yesterday (test_env_on on HOS_FPS_LOG, and test_psychov35 and test_pcache_header_read on Windows). Current main has fixed those, so a re-run should clear them.
One small thing: hookshot_env.h has env_int(name, def) for numeric switches. env_int("RX_VK_PRECOMPILE_JOBS", 0) could replace the getenv and atoi pair at vk_draw.cpp:8142.
The open question is policy, not code. #2414 is still open and caps the same precompile_run pool by physical cores, and its test_compile_threads expects its own helper there, so only one of the two can go in as written. As the description says, this doesn't fix mooki000's UHD 770 out-of-memory kill. That needs the very large compiles serialized.
…1593) precompile_run previously spawned up to std::min(usable_cpus(), 64u) compiler threads without clamping. On multi-core systems, especially under Linux with Mesa RADV (ACO compiler), compiling dozens of pipelines concurrently exhausted system RAM and triggered the Linux OOM-killer. Clamp default precompile concurrency to std::clamp(half_cpus(), 1u, 16u), honoring process CPU affinity and keeping the compiler pool deliberately lower than predict_run's 32 cap, with an optional RX_VK_PRECOMPILE_JOBS override (up to 64).
9c17e2b to
5d1351f
Compare
|
Rebased cleanly on latest |
This PR bounds worker thread concurrency in
precompile_run()using #2323's affinity-awarehalf_cpus()helper and honorsRX_VK_PRECOMPILE_JOBS(#1593). Without a cap, systems with high thread counts (especially on Linux using Mesa RADV / ACO) can spawn dozens of concurrent pipeline compiles simultaneously and exhaust system RAM.Why
In
gpu/vk/vk_draw.cpp(precompile_run()), the thread pool previously spawnedstd::min(usable_cpus(), 64u)compiler threads. On high-core-count systems, compiling many Vulkan graphics/compute pipelines concurrently can build excessive per-thread resident memory and trigger the Linux OOM-killer (#1593).Changes
std::clamp(half_cpus(), 1u, 16u), building on perf(prepare): build fewer predicted pipelines, size the pools to the usable CPUs, and keep translated programs across cache-code changes #2323'shalf_cpus()budget (already honoring process affinity viausable_cpus()) while keeping the compiler cap deliberately lower thanpredict_run()/state_list_run()'s 32 cap.RX_VK_PRECOMPILE_JOBS(capped at64u), leavingHOOKSHOT_JOBSstrictly for build-time concurrency so global build settings do not throttle runtime startup.tests/test_prepare_cpus.pysoprecompile_runis checked forhalf_cpus().Scope
Related to #1593 (closed, tracked in #1072). This bounds how many pipeline compiles run side by side, which is what #1593's RADV report showed. It does not fix the Intel ANV out-of-memory that mooki000 measured on a UHD 770: there a few ~250,000-word compute programs compiled together exhaust RAM even with 4 workers. That needs the large compiles serialized on their own, which is a separate follow-up.
#2414 caps the same pools by physical cores and keeps every smaller stage ceiling, so its cap would sit above this one: the two compose rather than conflict.