WIP: perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contents #30
Labels
No labels
area/accessibility
area/audio
area/input
area/install
area/menu
area/renderer
area/saves
area/upscaling
good first issue
gpu/amd
gpu/igpu
gpu/intel
gpu/nvidia
help wanted
kind
bug
kind
chore
kind
docs
kind
feature
kind
performance
kind
question
needs measurements
platform/linux
platform/steam-deck
platform/windows
preview
priority
critical
priority
high
priority
low
status
blocked
status
confirmed
status
duplicate
status
invalid
status
needs-info
status
wontfix
testing wanted
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
MagicBeanDev/Longshot!30
Loading…
Reference in a new issue
No description provided.
Delete branch "dyst/Longshot:perf/link-skip"
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?
Why
A build with nothing to compile still relinked the 440 MB executable and the renderer, a build from a second checkout of the same tree recompiled all eight hot pieces (~10-20 minutes) because their keys named the play profile by its path, and clangd had no compile database.
What changed
run_linkinnx2/build.pyskips a link whose inputs are the last successful one's.link_keyhashes the command, the compiler (its size and time) and the work folder'ssystem_headers.txt; each object's key (its.o.sha1, or its size and time without one); and the contents of the other files the command names, inside options too (link_inputs: the@response files, the hot symbol order, the libraries, the resources; not the output or a.map). The key is kept beside the output as<output>.linkkeywith the output's size; a failed link, or an output of another size or gone, links again. The three links inmaingo through it (the executable's, the renderer's and the game DLL's). (A4)write_compdbwrites every compile ascompile_oneruns it to the build folder'scompile_commands.json, for clangd and clang-tidy; a failure to write it does not fail the build..gitignoreignores/compile_commands.jsonat the checkout's top, for a copy there. (A8)flag_textleaves the path of-fprofile-instr-use=out of the hot pieces' object keys, since-DLONGSHOT_PROFILE_SHA1beside it already says what the key needs of the profile. This changes their keys once.nx2/build.pyimportsre(GitHub'smaindidn't; https://github.com/NotRexed/Hookshot/pull/2706 adds it too).tests/test_link_skip.py(new) runs a stand-in linker through the realrun_link: the same inputs aren't linked again; a changed object, library or symbol order is; so is an output of another size or gone; a failed link forgets its key; the files inside options are inputs; the compile commands are the compiles; the build's links go through it.tests/test_profile_path_key.py(new): two paths, one profile give one key; another profile, another key; every use of the profile's path names its contents too.Flags and settings
None.
Measurements
Measured on the GitHub version (https://github.com/NotRexed/Hookshot/pull/2893), not yet on Longshot: this PR stays WIP until it is.
Hardware: Ryzen 9 3900X, Windows 11 (as the GitHub description states it), a dev work folder.
Method: the GitHub description gives one figure for each side and does not say how many runs.
One renderer edit, on this PR: only the renderer's link runs (no figure for
main).Testing
Tested: the runs under Measurements (on the GitHub version) and the game-free checks this description names. Checked but not measured: every build the author ran in the game that day used this
build.py. The latest: Windows 11, RTX 3080 (NVIDIA 621.14), a dev-profile build, from a save, headless through the control bridge: in play in Kokiri Forest, 28-31 fps, nothing in the log about a lost device or a crash. A skipped link leaves the same binary in place.Not tested: Linux.
Needs other testers:
compile_commands.jsonwritten (request to open)Screenshots
Nothing on screen.
Risk and rollback
A link that should have run but was skipped would leave an old binary in place: the key covers the command, the compiler, each object's key and the files the command names, and deleting
<output>.linkkeymakes the next build link again. The hot pieces' new keys compile the eight hot pieces once after this lands. Reverting the commit is enough.Reviewer notes
Carried over from https://github.com/NotRexed/Hookshot/pull/2893 with the move to Longshot: the same change on Longshot's
main, with its rename applied. Its review and discussion are there.Three small changes to
nx2/build.pyfrom the dev-loop proposal (https://github.com/NotRexed/Hookshot/pull/2636, A4 and A8), in one PR because they touch the same lines; they can be split if preferred.Checks run on the GitHub version: every check that reads the changed files (37), through
run_all.py, rebased on GitHub'smainof that day, Windows: pass, apart from four that fail the same way onmainthere (test_driver_cache_folder,test_fpx_sticky,test_linux_update,nx2/test_dll.py). Only those checks were run, not the whole suite. The checks and the game runs were done on the GitHub version; this branch is the same change with the rename applied (the same files and line counts). Forgejo runs no checks; they were not run again here.Issues
None on Longshot. See https://github.com/NotRexed/Hookshot/pull/2636 (A4, A8).
Checklist
For everyone
type(scope): description).main.python -I -B tests/run_all.pyrun and passing (any failure that also fails onmainis named above).third_party/untouched (a new folder with aPROVENANCE.mdinstead).If applicable
Assisted-by:/Co-Authored-By:trailers.mainand passing here.🤖 Generated with Claude Code
perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contentsto WIP: perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contentsBack to WIP under #48's process: this was carried over before the new template. Its description will be brought to the template and measured on Longshot (main against this PR, at named commits) when it's wanted for review; ask here if you'd like it sooner.
Description brought to Longshot's PR template (#48). Still WIP: its numbers are from the GitHub version, and it isn't rebased on current main; it gets measured on Longshot (main against this PR, at named commits) before it is marked ready.
Description moved to the template's Testing section and whole checklist (#72): what was tested, what wasn't, and what needs other testers (their test requests to open while this is WIP).
The description follows the template after #76 and #106: If applicable keeps only the boxes that apply (one that applies and isn't done yet stays unticked while this is WIP), the Flags table has the template's seven columns (Goal is now Recommendation), and GitHub references are full URLs. Nothing else changed.
Read head
eb8ca08fagainst maincf605c85: all four changed files, the description, all four comments, the empty reviews, the link callers and their consumers; ran both new checks and five existing build checks on Linux, 37 tests passed. No game or full build was run.What's good
compile_one, including cached jobs.Must change
Include libraries resolved by the linker in the key. In
nx2/build.py, lines 149-153,link_inputsonly accepts arguments that name existing files directly. The real Linux renderer link uses-static-libstdc++and-static-libgcc; the Windows MinGW links use-static,-lc++,-lunwindand other-loptions. None of those library files enters the key. Replacing a selected archive while keeping the compiler, object stamps and command unchanged therefore keeps the old executable or renderer.system_headers.txtdoes not cover this: on Linux it hashesstdio.hand the compiler version, and on Windows the compiler version and selected header paths. Resolve and record the actual libraries, including implicit runtime archives and the search settings that select them, or do not skip a link whose inputs are not completely known. Add a small real-link test that replaces an archive reached through-L/-land checks that the next binary contains the new implementation. The actual linker selected by-fuse-ld=lldneeds the same treatment; hashing the compiler driver alone does not notice a linker-only update.Require the requested link maps before skipping. In
nx2/build.py, lines 192-194, a matching key and binary size are sufficient to return success, even if the link's.mapoutput is missing. All three converted call sites request a map. After a map is removed from an otherwise retained build, rebuilding no longer restores it, unlike main. On Windows,scripts/prof.pythen cannot name that module's sampled frames, andnx2/samap.pycannot openlongshot.map. Keep maps out of the input hash, but include them in the required output checks. A regression should link once, remove only the map, and verify that the next build recreates it.Measurements and smaller points
82de567a. It nevertheless merges cleanly withcf605c85; the WIP mergeability flag is not a conflict. Main's interveningbuild.pychanges concern FSR's prebuilt-renderer selection, not these optimizations. A rebase is needed before the eventual comparison, but is not the reason for requesting changes.--dllstill callssubprocess.runatnx2/build.py, lines 1519-1521. The three converted sites are the Linux executable, Windows executable and Vulkan renderer, exactly as the new test's comment says. Also identify #28 as the companion that suppliesdev.py compdb; this PR writes the database but does not add that command.Overlaps
flag_text. Both merge pairwise with this head; keep their object-key and failure behavior when combining them.Not checked
No Windows execution, GCC run, full suite, game, full build, real-library replacement reproduction, missing-map reproduction or independent timing here. The seven passing files were
test_link_skip,test_profile_path_key,test_build_hash_cache,test_kept_translation,test_compiler_failure,test_compile_retry_jobsandtest_link_path_comma; the latter is specifically a Clang-driver check. The two must-change points above are traced from the code, not claimed as executed regressions. Real archive-replacement and missing-map tests would settle them directly, followed by clean and incremental Windows and Linux builds.The current head's reported statuses are a successful description check and a skipped website check, not source-test evidence: https://git.magicbean.dev/MagicBeanDev/Longshot/actions/runs/1174 and https://git.magicbean.dev/MagicBeanDev/Longshot/actions/runs/3076. The linked original discussion at https://github.com/NotRexed/Hookshot/pull/2893 and its comment/review endpoints return 404, so I could not compare these points with that older review.
Code review of head
eb8ca08fagainst mainf804b7b8(#385 merged). I read the diff and the code around it, and checked the merged tree withgit merge-tree. I ran no tests, no build and no game.mooki's request for changes (#30 (comment)) still stands, and I agree with both of its must-change points: the libraries the linker finds by search path (
-l…,-static-libstdc++, thelldthat-fuse-ldpicks) are not in the key, and a skipped link does not bring back a missing.map. I don't repeat them here. What follows is new, and is mostly about main as it is since #371 and #385.The three parts against main now
The branch is 134 commits behind main. It still merges with no textual conflict, but the rebase changes code: one of its own checks fails on the merged tree (point 1).
flag_text): main doesn't cover this, and it is worth more now than when it was written.make_game.pysetsRX_PGO=<work>/longshot_pieces.prof, so the hot pieces' key names the work folder's path. Main'snx2/object_store.pydocstring says "the object keys hold no folder path". For the eight hot pieces that is false today, so neither the object store from 371 nordev_packs.link_objectscan share them between work folders. This part makes it true. It doesn't interact with 385:split_flgoes only to the other lifted chunks, and the hot pieces keephot_fl. The description's case (a second checkout building in the same work folder) is now the smaller benefit. The main one is the store and dev_packs packs. See the inline comment for the one-time cost.run_link/link_key): main doesn't cover this; 371 shares objects, not links. It works with the store's hard links:link_keyreads each object's.o.sha1stamp, whichcompile_onewrites the same way for a fetched object and a compiled one, andsystem_headers.txt(the store's note) is in the key. The gaps mooki named still apply, plus a weak output check (inline).compile_commands.json(write_compdb): main doesn't cover this. It is written once frommain(), before the pool starts, so parallel compiles can't race on it. Since 385,compile_onecompiles a temporarysplit_giants.compile_copy, notsrc, so the database is no longer "as compile_one runs it", and the new check that says it is fails on the merged tree (point 1).Must change
tests/test_link_skip.pyfails once rebased.test_the_compile_commands_are_the_compileslooks for'[cc] + fl + ["-c", src, "-o", obj]'incompile_one. Since 385 that line reads["-c", c_src, "-o", obj]. Update the check, and the docstring's "as compile_one runs it", to match main. Pointing clangd atsrcwith the split flag is the right choice: the copy is deleted after the compile. Just say so..map).Should change
st_mtime_ns(and the inode) beside the size costs nothing (inline).flag_textalso changeskept_part. For a player whose last build kept its translation (--kept-lift), the first build after this lands finds the hot files' kept part stale, returnsKEPT_STALE, and translates the game again, on top of compiling the eight hot pieces. Name that under Risk. It would cost less to land it in a release that recompiles everything anyway.nx2/dev.py compdbdoesn't exist on main. The.gitignorecomment and thewrite_compdbdocstring point at it. It comes from #28, which is still open. Reword both, or say this lands after 28.gen/. That is heavy for anyone who opens the work folder's database. Consider leaving the lifted chunks out, or document a.clangdIndex: Background: Skipfor them.Description and checklist
main @ <hash>/this PR @ <hash>, at least two interleaved runs, and the median.run_link. It doesn't:--dllstill callssubprocess.run, as mooki noted.compile_commands.jsonand<output>.linkkey, including how to force a link by deleting it.Verdict: request changes. The idea is sound, and the profile-key part is now a fix for the object store from 371. The link skip needs mooki's two fixes, and the branch needs a rebase that updates its own check. Splitting the profile-key part into its own small PR would let it land quickly.
@ -38,2 +38,4 @@# ---- NVIDIA's DLSS SDK (scripts/ngx_sdk.py): NVIDIA's files, never committed ----ngx-sdk/# ---- nx2/dev.py compdb: the last build's compile commands, for clangd and clang-tidy ----nx2/dev.py compdbisn't on main. It comes from #28, still open. Reword this, or land after 28 (same forwrite_compdb's docstring).@ -134,0 +189,4 @@old = f.read().split()except OSError:old = []if key and os.path.exists(output) and old == [key, str(os.path.getsize(output))]:A skip trusts the output if its size matches. A file of the same size written over it (a prebuilt renderer copied over
longshot_vk.*, a restored backup, an old build copied in) keeps a stale binary that never relinks. Recordst_mtime_nsandst_inowith the size (line 200) and compare all three, and also require the.maphere (mooki's point 2).@ -134,0 +208,4 @@"""(the dev-loop proposal's A8) <build>/compile_commands.json: every compile this build runs, as compile_one runs it, for clangdand clang-tidy (nx2/dev.py compdb puts it at the checkout's top). The lifted code's and the host's commands name the lift'sgen/ and rt64/ folders, which are this build's; a failure to write it is no failed build"""entries = [{"directory": os.getcwd(), "file": src, "output": obj, "arguments": [cc] + list(fl) + ["-c", src, "-o", obj]}Since 385, split chunks are compiled from a temporary copy (
split_giants.compile_copy), notsrc. Namingsrcwith the same flags is the right thing for clangd, but say so here and in the docstring, which still says "as compile_one runs it". Also, this lists every lifted chunk (about 1.4 GB of C), and clangd's background index will try to index all of it. Consider leavinggen/chunks out, or document the.clangdsetting that skips them.@ -667,0 +756,4 @@elif f.startswith("-fprofile-instr-use="):# (the play profile's path: its contents are in -DLONGSHOT_PROFILE_SHA1 beside it. Its path kept two checkouts, or a# build from a checkout other than the work folder's, from sharing the eight hot pieces, ~10-20 minutes to compile)f = "-fprofile-instr-use=<profile>"Main still needs this:
make_game.pysetsRX_PGO=<work>/longshot_pieces.prof, so without it the hot pieces' key names the work folder, and #371's store (whose docstring says the keys hold no folder path) anddev_packs.link_objectscan't share them. It's worth saying that in the description. One more cost to name under Risk:kept_partusesflag_text, so a player with a kept translation getsKEPT_STALEonce and the game is translated again, not only the eight hot pieces compiled.@ -0,0 +109,4 @@main = src[src.index("\ndef main("):]self.assertLess(main.index("write_compdb(out, jobs)"), main.index("plan_jobs(jobs)"))one = src[src.index("\ndef compile_one("):src.index("\ndef ", src.index("\ndef compile_one(") + 1)]self.assertIn('[cc] + fl + ["-c", src, "-o", obj]', one) # (the command the database says, as compile_one runs it)This fails on main as it is: since #385,
compile_oneruns[cc] + fl + ["-c", c_src, "-o", obj](asplit_giants.compile_copywhen the split flag is set). Check what the database should say instead: the flags arecompile_one's, and the file is the source, not the temporary copy. Update the comment too.View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.