Repository navigation
fix(vk): refresh pipeline states during Builder preparation - #1980
Conversation
hckrman101
left a comment
There was a problem hiding this comment.
Thanks, the C++ half is the right fix for the real gap: on an already prepared seed prepare_run returned before any state list was read, so a new shipped list was never imported by --prepare-shaders. A few things to sort out before this can go in.
1. The PR is out of date with main (mergeable state: dirty), and the Python half no longer applies. Since the list was reworked, ship_state_list on main merges the sources in assets/pipeline_states/ (pipeline_states.sources_dir, merged, pack; see "The pipeline state list" in docs/development.md). pipeline_states.SHIPPED and assets/pipeline_states.bin.xz are gone, so the rewritten ship_state_list(out, work) and the new test steps (which move an .xz to tool.SHIPPED) will not rebase cleanly and would not work if they did. Nothing on main writes a pipeline_states.bin into --work either, so I could not find what the new override is for. If it is for a locally recorded list, say how it gets there. As written it replaces the shipped list rather than adding to it, so a stale file in the work folder would hide the list an update ships, which is the situation this PR is trying to fix. I would drop the make_game change, or redo it on the merged layout and merge the local list with the shipped one instead of replacing it.
2. #2032 (merged). The C++ part is compatible in intent. In started_up on main, prepare_run returning false already means bar.skip(0), bar.skip(1) and no predict_run, then precompile_run, so notes added by the refresh are built before the game. Two things to check after the rebase: the refresh calls state_list_prepare(nullptr), so it can translate programs for a while with no progress while the bar has the programs stage marked as skipped (a stalled bar rather than a wrong one), and the new prepare_run argument has to be carried through #2032's changes to that call site.
3. Not rebuilding on every start: verified. Only the --prepare-shaders call (started_up, the prepare_only() branch) passes refresh_states=true. The in-game start paths (prepare_with_screen, the console path) and prepare_background still call prepare_run with the default false, so ordinary launches are unchanged. That also means a normal start after an update, with no Builder run, still does not import a new shipped list; that is fine if intended, but the description could say so. Each Builder prepare now re-reads both lists and reopens the seed even when nothing changed. Cost is bounded because state_list_prepare only translates programs the cache lacks, but I did not see a number for the no-change case (the description measures 0 to 1829 pipelines, not a repeat run).
4. No shipped list: fine. state_list_prepare returns on recs.empty() before seed_open() is tried, so nothing is opened or left open, and RX_VK_NO_STATE_LIST still short-circuits. I found no early return between the seed_open() and the seed_close() that would leak the seed. The ordering in the already-prepared branch is also right: states are noted before the libraries warm.
5. Tests and CI. The test files are registered (test_prepare_cache_save.py gains RUN_ALL_ARGS; test_pipeline_states.py is existing), and both passed on the Windows job. The fixture in test_prepare_cache_save.py pastes the real prepare_run branch text, which fits the repo's "tests read the source" style. The single red check, Python source checks (windows-2022), is tests/test_log_batch.py (a 200 ms timing assertion) and a test_bake_skeleton.py timeout. Neither touches this change, so I read them as flakes, but a re-run is needed to get green, and the tests need re-running anyway after the rebase. Linux and the other Windows jobs pass.
Decision on merging stays with the maintainer. I am leaving this as a comment because of the conflict and point 1.
|
Thanks for the detailed review, @hckrman101. I merged main at I carried The 12 focused checks passed locally, and the Windows Vulkan and Linux AppImage builds passed. The new source CI run is red on |
hckrman101
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround. Re-checked at ba406a6.
Fixed from my last review:
-
- The make_game override and its test steps are gone. The PR is now only
vk_draw.cpp,vk_internal.h,vk_main.cppand the test, so the shipped list comes from main'sassets/pipeline_states/packaging, and a stale work file can't hide it anymore. It merges cleanly with main.
- The make_game override and its test steps are gone. The PR is now only
-
refresh_statesis carried through #2032's prepare-only call site. The bar no longer stalls:state_list_preparenow reports progress for the programs and then the missing variants. The total is recomputed once the variant count is known, andPrepareBar::fractionkeeps the bar from moving back when the total changes.report_doneserialises the callback under a mutex. That lock is needed, since this is the first caller to pass a non-nullprogressfrom the worker threads, and theprepare_onlylambda touches the static bar andlast.if (!bar.counted[0]) bar.skip(0)is right: part 0 stays in the total only when the import actually reported something.
-
- The description now says the refresh runs only for Builder preparation. Only the
--prepare-shaderscall passestrue, so in-game starts andprepare_backgroundare unchanged.
- The description now says the refresh runs only for Builder preparation. Only the
Also checked: folding seed_open()/seed_close() into state_list_prepare(..., open_seed) keeps state_list_run's old behaviour. RX_VK_NO_STATE_LIST is checked first, the seed is opened only when there are records, and it is left open under RX_VK_SEED_CHECK as before. There's no return between the open and the close. The extended test_prepare_cache_save.py pastes the real already-prepared branch and covers fresh, refresh and empty. It passes on Linux and both Windows jobs.
Nits, not blocking:
- If the seed is prepared but the driver part is stale (
prepared_for.warmed != warmed), the state import and thenwarm_librariesboth report on part 0. The bar sits still while the warm catches up to where the import left it. It's cosmetic, sincefractionclamps it. - The repeat-run cost with unchanged lists is still unmeasured. That's fine as stated in the description. A log line from a second
--prepare-shadersrun would settle it.
CI: the three red Python source jobs are test_make_game_ngx.py (stub missing switching()) and, on Windows, test_pipeline_state_portable.py (fopen deprecation under -Werror). Main's latest source run (21fa136) fails on exactly the same two, so they aren't from this PR. The builds and the AppImage are green.
Looks good to me. The merge is NotRexed's call.
Eight checks fail on any PC without clang or the zstandard module,
instead of using the compiler `run_all.py` found or being skipped. Five
C++ tests default to `clang++` and two C tests to `clang` without a
`RUN_ALL_ARGS` line (for example `tests/test_3d_view_create_flags.py`
and `tests/test_nvn_null_pool.py`), and `nx2/test_nup_read_name.py`
imports `zstandard` without `RUN_ALL_MODULES`. `run_all.py` only checks
the tools a test names, so it can neither hand these its compiler nor
skip them, and on a g++-only box they show as failures.
Each now declares what it needs: `RUN_ALL_ARGS = ["--cxx", "{cxx}"]` on
the five (the runner's compiler, which is still clang++ where it is
installed), `["--cc", "clang"]` on the two C ones, and `RUN_ALL_MODULES
= ["zstandard"]` on the last. Nothing changes where clang and zstandard
are installed, as on CI.
`tests/test_prepare_cache_save.py` was a sixth C++ one; main gave it the
same line in #1980, so it is no longer part of this.
**Testing (Linux, g++ 15, no clang, no zstandard):** after the rebase,
the eight through `run_all.py`: the five C++ tests pass with g++, and
the other three are skipped with their reason. Before the rebase,
`tests/test_run_all_declarations.py` passed, and the full `run_all.py`
gave 298 passed, 61 skipped, with the only failures the two that #2109
has since fixed on main.
The Builder can install a newer pipeline state list while the game's shader seed stays the same. With an already prepared seed,
--prepare-shadersused to return before importing the new states, leaving those graphics pipelines to compile during play.Explicit Builder preparation now imports the shipped and locally recorded state lists before precompiling and saving the cache. The progress bar advances while the state list checks programs and translates missing variants. The Builder uses main's merged list packaging. This refresh runs only when the Builder explicitly prepares shaders, so launching the game by itself after an update does not trigger it.
All 12 focused checks passed locally on Windows. The Windows Vulkan build and Linux AppImage build passed in CI. Source CI reported two fixture failures outside the diff:
test_make_game_ngx.pycalls a stub missingswitching(), and the Windows toolchain treatsfopendeprecation warnings as errors intest_pipeline_state_portable.py. The cost of repeating Builder preparation with unchanged lists has yet to be measured.