perf(build): a renderer header edit compiles the files that read it, not the whole back end #2706
Open
github-sync wants to merge 2 commits from
github/pr/2706 into main
pull from: github/pr/2706
merge into: Hookshot:main
Hookshot:main
Hookshot:release/v2.2.0-rc.1-forgejo
Hookshot:release/v2.1.0-forgejo
Hookshot:github/pr/2893
Hookshot:github/pr/2890
Hookshot:github/pr/2887
Hookshot:github/pr/2878
Hookshot:github/pr/2875
Hookshot:github/pr/2874
Hookshot:github/pr/2873
Hookshot:github/pr/2871
Hookshot:github/pr/2756
Hookshot:github/pr/2897
Hookshot:github/pr/2896
Hookshot:github/pr/2718
Hookshot:github/pr/2842
Hookshot:github/pr/2691
Hookshot:github/pr/2895
Hookshot:github/pr/2743
Hookshot:github/pr/2765
Hookshot:github/pr/2708
Hookshot:github/pr/2732
Hookshot:github/pr/2755
Hookshot:github/pr/2817
Hookshot:github/pr/2876
Hookshot:release-please--branches--main
Hookshot:release-please--branches--main--release-notes
Hookshot:github/pr/2778
Hookshot:github/pr/2888
Hookshot:github/pr/2866
Hookshot:github/pr/2877
Hookshot:github/pr/2800
Hookshot:github/pr/2829
Hookshot:github/pr/2850
Hookshot:github/pr/2889
Hookshot:github/pr/2882
Hookshot:github/pr/2860
Hookshot:github/pr/2849
Hookshot:github/pr/2886
Hookshot:github/pr/2794
Hookshot:github/pr/2840
Hookshot:github/pr/2830
Hookshot:github/pr/2834
Hookshot:github/pr/2719
Hookshot:github/pr/2742
Hookshot:github/pr/2741
Hookshot:github/pr/2820
Hookshot:github/pr/2771
Hookshot:github/pr/2859
Hookshot:github/pr/2858
Hookshot:github/pr/2823
Hookshot:github/pr/2757
Hookshot:github/pr/2769
Hookshot:github/pr/2844
Hookshot:github/pr/2843
Hookshot:github/pr/2673
Hookshot:github/pr/2855
Hookshot:github/pr/2739
Hookshot:github/pr/2807
Hookshot:github/pr/2851
Hookshot:github/pr/2789
Hookshot:github/pr/2814
Hookshot:github/pr/2818
Hookshot:github/pr/2774
Hookshot:github/pr/2772
Hookshot:github/pr/2750
Hookshot:github/pr/2786
Hookshot:github/pr/2785
Hookshot:github/pr/2782
Hookshot:github/pr/2779
Hookshot:github/pr/2775
Hookshot:github/pr/2766
Hookshot:github/pr/2747
Hookshot:github/pr/2759
Hookshot:github/pr/2745
Hookshot:github/pr/2740
Hookshot:github/pr/2733
Hookshot:github/pr/2725
Hookshot:github/pr/2727
Hookshot:github/pr/2728
Hookshot:github/pr/2729
Hookshot:github/pr/2721
Hookshot:github/pr/2722
Hookshot:github/pr/2715
Hookshot:github/pr/2714
Hookshot:github/pr/2707
Hookshot:github/pr/2703
Hookshot:github/pr/2705
Hookshot:github/pr/2697
Hookshot:github/pr/2679
Hookshot:github/pr/2676
Hookshot:github/pr/2669
Hookshot:docs/what-hookshot-connects-to
Hookshot:fix/hdr-encode-test
Hookshot:fix/main-tests
Hookshot:fix/play-cache-files
Hookshot:perf/game-barrier-order
Hookshot:feat/heap-lazy
Hookshot:feat/texture-detail
Hookshot:perf/astc-opaque-bc1
Hookshot:fix/fs-dir-lock
Hookshot:fix/framegen-native-kept
Hookshot:fix/update-no-downgrade
Hookshot:port/k10-gcn-f16-mirrors
Hookshot:fix/framegen-mult-cap
Hookshot:chore/release-as-2.1.1
Hookshot:perf/pcache-load-switch
Hookshot:fix/prepare-progress-log
Hookshot:fix/update-closes-stale-game
Hookshot:fix/hdr-scaled-sampled
Hookshot:perf/first-start-preset
Hookshot:perf/zero-upload
Hookshot:fix/vcore-escape-2s
Hookshot:test/fake-gh-atomic
Hookshot:perf/batch-idle-flush
Hookshot:test/finish-stub-a
Hookshot:chore/record-2.1.0
Hookshot:fix/main-red-tests
Hookshot:fix/test-display-startup-driver-cache
Hookshot:feat/mesh-shader-mode
Hookshot:fix/menu-probes-at-startup
Hookshot:fix/fps-counter-classic
Hookshot:fix/test-log-batch-flake
Hookshot:fix/narrow-screen-hud
Hookshot:ci/prereleases
Hookshot:release/v2.0.15
Hookshot:hotfix/v2.0.15
Hookshot:fix/present-ahead-replay
Hookshot:release/v2.0.13
Hookshot:fix/bracket-paths-2013
Hookshot:fix/hdr-overlays-2013
Hookshot:fix/fps-counter-scale-2013
Hookshot:fix/stick-seen-at-rest
Hookshot:hotfix/menu-fits-screen
Hookshot:fix/f1-always-opens-menu
Hookshot:fix/ini-keep-lines
Hookshot:claude/cranky-pike-7cf247
Hookshot:perf/texture-answer-flat
Hookshot:fix/stamp-test-modules
Hookshot:fix/appimage-fonts
Hookshot:fix/display-awake
Hookshot:release/v2.0.12
Hookshot:fix/discord-windows
Hookshot:rc/v2.0.12-tests
Hookshot:fix/dlss-keep-feature
Hookshot:rc/v2.0.12
Hookshot:button-icons
No reviewers
Labels
Clear labels
accessibility
Barrier affecting people with disabilities
autorelease: pending
autorelease: tagged
bug
Something isn't working
dependencies
Pull requests that update a dependency file
documentation
Improvements or additions to documentation
duplicate
This issue or pull request already exists
enhancement
New feature or request
github_actions
Pull requests that update GitHub Actions code
good first issue
Good for newcomers
help wanted
Extra attention is needed
invalid
This doesn't seem right
question
Further information is requested
testing wanted
wontfix
This will not be worked on
No labels
accessibility
autorelease: pending
autorelease: tagged
bug
dependencies
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
question
testing wanted
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
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
Hookshot/Hookshot!2706
Loading…
Reference in a new issue
No description provided.
Delete branch "github/pr/2706"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Editing any renderer header recompiles the whole back end: on Windows a comment in
vk_present_rect.h, which two files include, compiled 35 files again (417 s of compiler time; a 44 s build against 26 s for a.cppedit, and minutes on a 4–8 thread PC). This is the dev-loop proposal's A6 (#2636), validated by hckrman101's Linux numbers there (16 objects for one header).Change
gpu/vk/*.cpp) are compiled with-MD, and clang writes a depfile beside each object (vk_x.d).gpu/vk,gpu/render,gpu/compat, suyu's texture decoders), by name and content (renderer_deps). A header the file doesn't read no longer changes its key; one it reads, or one that's gone, does.flag_textleaves-MDout, so no key changes and nothing compiles again for this change itself. Depfiles appear as files are compiled.rekey_depfiles), so the next build doesn't compile it once more. Only an object of this build, so made from this build's source and headers, is re-keyed.compile_onestamps itdepfile-pending, andrekey_depfilesstamps and caches it by its own depfile's key, also when that's the planned key. A build that stops before re-keying leaves it pending, and the next build compiles it again. (hckrman101's review: an include added, removed, the header edited, the include back, used to get the first object back from the cache.)Measured
On Windows (Ryzen 9 3900X, NVMe):
vk_present_rect.h, beforeThe four are the files that read it:
vk_framegen.cpp,vk_menu.cpp,vk_replay.cpp, andhost/hos_main.c, through the host's own blob. Getting depfiles took one compile of each back-end file, done here by commenting them all. The re-keying then kept the reverted files from compiling a third time.In the game
Windows 11, RTX 3080 (NVIDIA 621.14), from this branch's
build.py(with the review fix) onmainee29dd55: a build with depfiles, then one with a comment added tovk_present_rect.h(5 files compiled: the four above, andvk_hangdump.cpp, which reads no repo header, so its depfile names nothing under those folders and it keeps the blob; no object left pending; with the comment taken out again, 0 files). That game, launched headless through the control bridge from a save: in play in Kokiri Forest, 27–34 fps over five samples, nothing in the log about a lost device or a crash.Testing
tests/test_renderer_depfiles.py(new, 11 cases):/and\in paths with spaces, CRLF continuations), plus$$and\#;-MDis in no key;b.h.It fails against
main'sbuild.py.test_compiler_failure.pyandtest_mem_gate_leases.pytakeDEPFILE_DEPSandDEPFILE_PENDINGfrombuild.pywithcompile_one.test_dlss_sdk_cache.pyandtest_shader_stamp_cache.pyrun the job loop with its new helpers. Both still take an olderbuild.py(--build-script), andtest_shader_stamp_cachepasses againstmain's.Every check that reads
build.py(39), on Windows, passes apart fromtest_driver_cache_folder,test_fpx_stickyandtest_linux_update, which fail the same way onmainhere.Not tested: Linux. gcc and clang write the same make-style depfile.
🤖 Generated with Claude Code
This adds the correct key, but
compile_onehas already cached the object under the planned key, which for a depfile job came from the previous compile's depfile (the new source hashed with the old include list). That entry is what a later build can hit with a stale header (steps in the review body). Doing thecache_puthere even whenkey == old, and not under the planned key incompile_oneforDEPFILE_JOBSobjects, would keep only sound keys in the cache.Thanks, this is a nice win and the numbers in the description are exactly what I wanted to see (same edit before/after, the hardware named). The depfile parser, the
-MDexclusion from the key, the blob fallback and the "only re-key an object of this build" guard all read right to me, and CI is green.One correctness problem before I can approve: a back-end object can come back from the object cache built against an old header.
compile_onestill caches every compiled object under its planned key (cache_put(obj, key)at the end, not in this diff). For a depfile job that planned key comes from the previous compile's depfile, so it hashes the new source with the old include list. That key is lossy: it doesn't cover a header the new source reads for the first time.rekey_depfilesadds the correct key, but the lossy one stays in the cache, and a later build can land on it:vk_x.cppincludesa.h; build. Its depfile says{a.h}.#include "b.h"tovk_x.cpp; build. Planned key = (new source,{a.h}). Compiled withb.hv1 and cached under that key; re-keyed under (new source,{a.h, b.h}).{a.h}.b.h, then put the include back (undo/redo, orjj editback to the revision that has it); build. The key is (new source,{a.h}) again, so it's a cache hit on step 2's object, still built withb.hv1. A cache hit doesn't write a depfile, sorekey_depfilescomputes the same key and the stale object stays untila.hor the source changes.Switching between revisions where one adds an include is an everyday thing, and a stale struct layout here means a crash or corruption in the game, not a build error, so I'd rather close this hole. The simplest fix I see: for a
DEPFILE_JOBSobject, only ever cache it under the key its own depfile gives. Skip thecache_putunder the planned key incompile_onewhen that key came from a depfile (a blob key is fine, since it covers every header) and letrekey_depfilesdo the put, also when the key didn't change. A lookup can then only hit an object whose depfile lists exactly the headers being hashed, and that's sound. A case intest_renderer_depfiles.pythat walks through the steps above would pin it down.Smaller things:
-MD(and so the depfiles) only goes to the back end's own files, buthost/hos_main.cstill goes through the host's blob, as the description says. That's fine for this PR.Thanks: that's a real hole, and your steps reproduce it exactly. Fixed in
f0f345b6:compile_oneno longer caches, or stamps, an object under a key that came from the last compile's depfile. It stamps itdepfile-pendinginstead.rekey_depfilesdoes the keying. It stamps and caches the object by the key its own new depfile gives, also when that's the planned key. The cache now only ever holds blob keys (every header) or a compile's own depfile key.test_renderer_depfiles.pywalks your four steps with a stand-in compiler that writes its included headers into the "object" and a real depfile. On the previous commit it fails: step 4 got back step 2's object, built with the oldb.h.On running the game: with this branch's
build.py(the fix included) on Windows 11 and an RTX 3080, I built once to get the depfiles, then built again after adding a comment tovk_present_rect.h. That compiled the four files plusvk_hangdump.cpp(no repo headers, so it keeps the blob), and left no object pending. The game from that build, launched headless from a save, plays in Kokiri Forest at 27-34 fps with nothing in the log about a lost device or a crash. With the comment taken out, the next build compiled nothing. That's in the description now.test_compiler_failure.pyandtest_mem_gate_leases.pyalso cutcompile_oneout, so they now take the two new constants frombuild.pyas well.Done in
f0f345b6:compile_onestamps a depfile-keyed objectdepfile-pendingand caches nothing, andrekey_depfilesdoes thecache_putunder the key its own depfile gives, also whenkey == old. Details and the four-step test are in https://git.magicbean.dev/MagicBeanDev/Hookshot/pulls/2706.hckrman101's stale-object hole is closed in
f0f345b6.compile_onestamps an object it compiled under a depfile keydepfile-pendingand doesn't cache it (nx2/build.py:952).rekey_depfilesthen caches and stamps it under the key from its own new depfile, so the cache only holds blob keys or a compile's own depfile key. I traced the cache-hit path (compile_onerestoring fromOBJ_CACHE). A hit needs the planned key to equal a key computed from that object's own depfile, so the source and the set of hashed headers match, and leaving the old.don disk is sound. A build that fails beforerekey_depfilesleaves pending stamps, which never match, so those files compile again. I merged this into current main and ran the depfile, compile, mem_gate, dlss_sdk, shader_stamp and build checks: 31 passed.test_precompile_compute_guardandtest_portable_local_builderalso fail on main on this machine, so they're not from this PR. The in-game run after the header edit is what was asked for. The red appimage job timed out in the PPA apt step, which is infrastructure, so a re-run should clear it.Linux result on
f0f345b6, against its main base46780503: the identical header edit recompiles 5 files instead of 35, and both resulting games load Kokiri Forest and exit cleanly.Setup: Arch Linux, kernel 7.2.15, i9-13900K, RTX 4090, NVIDIA 615.78.08, Clang 23.1.3 release builds with the same PGO profile and compatible translated inputs. Separate checkouts and game folders. After the initial builds, I added the same harmless comment to
gpu/vk/vk_present_rect.hin each checkout and rebuilt throughnx2/build.py.46780503f0f345b6The five were
vk_framegen.cpp,vk_menu.cpp,vk_replay.cpp,host/hos_main.c, andvk_hangdump.cpp. This matches your explanation: the host still uses its header group, andvk_hangdump.cpphas no repository-header dependency and keeps the fallback. Counts come from the build's own@@file ... newrecords, asserted by script. Other work was running, so I make no wall-time speed claim.The stale-cache review fix also passes with real compilers. A separate check drives the actual cache/key/compile/rekey functions, adds an include, removes it, edits that header, and adds it back. It links and runs each resulting object. GCC 16.2 and Clang 23.1.3 both return the new value 50 at step 4 on
f0f345b6; both return the stale value 30 on632b55ac. This does not use the stand-in compiler from the PR's test.Game check after the header edit: windowless, fresh copied save, 2048×1152 game output / 1920×1080 offscreen presentation, 60 cap, DLSS/frame generation off. Base: 37.9 seconds, 1,505 game-present rows, exit 0. PR: 34.3 seconds, 1,454 rows, exit 0. Both show Link, the forest and HUD; neither logs a crash, lost device or kernel driver fault. Load averages were 36.37→34.31 and 13.85→13.41, respectively: these are not frame-rate comparisons. The experimental comments were removed afterward; the retained tested binaries are explicitly the post-comment builds.
All six changed checks pass in both compiler environments. Against base source, the new depfile check reports one flag-key failure and ten missing-helper errors; the compile-failure/memory-gate adaptations reject missing constants. The existing compile-retry, DLSS-cache and shader-stamp checks pass on base too, as compatibility coverage. Full suite on the head: 473 passed, 0 failed, 20 skipped.
The hosted AppImage job separately timed out in System packages, exit 124, before compiling this change: https://github.com/NotRexed/Hookshot/actions/runs/43211349705/job/131159467435.
Not tested: Windows runtime, all possible include-path shadowing changes, other game scenes, or quiet repeated build timings. No new defect found in the exercised head.
View command line instructions
Manual merge helper
Use this merge commit message when completing the merge manually.
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.