WIP: perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contents #30

Draft
dyst wants to merge 1 commit from dyst/Longshot:perf/link-skip into main
Member

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

  • build: run_link in nx2/build.py skips a link whose inputs are the last successful one's. link_key hashes the command, the compiler (its size and time) and the work folder's system_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>.linkkey with the output's size; a failed link, or an output of another size or gone, links again. The three links in main go through it (the executable's, the renderer's and the game DLL's). (A4)
  • build: write_compdb writes every compile as compile_one runs it to the build folder's compile_commands.json, for clangd and clang-tidy; a failure to write it does not fail the build. .gitignore ignores /compile_commands.json at the checkout's top, for a copy there. (A8)
  • build: flag_text leaves the path of -fprofile-instr-use= out of the hot pieces' object keys, since -DLONGSHOT_PROFILE_SHA1 beside it already says what the key needs of the profile. This changes their keys once.
  • build: nx2/build.py imports re (GitHub's main didn't; https://github.com/NotRexed/Hookshot/pull/2706 adds it too).
  • tests: tests/test_link_skip.py (new) runs a stand-in linker through the real run_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: 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.

GitHub main this PR (GitHub)
A build with nothing to do 16 s 6 s, both links skipped
A second checkout of the same tree, building in the same work folder the eight hot pieces compiled again the eight hot pieces from the cache

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:

  • Linux: a second build with nothing changed, expecting the links skipped and compile_commands.json written (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>.linkkey makes 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.py from 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's main of that day, Windows: pass, apart from four that fail the same way on main there (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

  • One topic.
  • The title is a Conventional Commit (type(scope): description).
  • Rebased on current main.
  • python -I -B tests/run_all.py run and passing (any failure that also fails on main is named above).
  • Tested in the game, or what wasn't is named under Testing.
  • Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests).
  • Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported.
  • No game files, dumps, build output or generated files.
  • third_party/ untouched (a new folder with a PROVENANCE.md instead).
  • The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled.

If applicable

  • AI assistance recorded with Assisted-by: / Co-Authored-By: trailers.
  • A game-free test added for the fix, failing on main and passing here.

🤖 Generated with Claude Code

## 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 - build: `run_link` in `nx2/build.py` skips a link whose inputs are the last successful one's. `link_key` hashes the command, the compiler (its size and time) and the work folder's `system_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>.linkkey` with the output's size; a failed link, or an output of another size or gone, links again. The three links in `main` go through it (the executable's, the renderer's and the game DLL's). (A4) - build: `write_compdb` writes every compile as `compile_one` runs it to the build folder's `compile_commands.json`, for clangd and clang-tidy; a failure to write it does not fail the build. `.gitignore` ignores `/compile_commands.json` at the checkout's top, for a copy there. (A8) - build: `flag_text` leaves the path of `-fprofile-instr-use=` out of the hot pieces' object keys, since `-DLONGSHOT_PROFILE_SHA1` beside it already says what the key needs of the profile. This changes their keys once. - build: `nx2/build.py` imports `re` (GitHub's `main` didn't; https://github.com/NotRexed/Hookshot/pull/2706 adds it too). - tests: `tests/test_link_skip.py` (new) runs a stand-in linker through the real `run_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: `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. | | GitHub main | this PR (GitHub) | |---|---|---| | A build with nothing to do | 16 s | 6 s, both links skipped | | A second checkout of the same tree, building in the same work folder | the eight hot pieces compiled again | the eight hot pieces from the cache | 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:** - Linux: a second build with nothing changed, expecting the links skipped and `compile_commands.json` written (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>.linkkey` makes 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.py` from 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's `main` of that day, Windows: pass, apart from four that fail the same way on `main` there (`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 - [x] One topic. - [x] The title is a Conventional Commit (`type(scope): description`). - [ ] Rebased on current `main`. - [ ] `python -I -B tests/run_all.py` run and passing (any failure that also fails on `main` is named above). - [x] Tested in the game, or what wasn't is named under Testing. - [x] Testing filled in: what was tested, what wasn't, and what needs other testers (with test requests). - [ ] Measured: main and this PR at named commits, on the same PC, scene and settings, at least 2 interleaved runs each, the median reported. - [x] No game files, dumps, build output or generated files. - [x] `third_party/` untouched (a new folder with a `PROVENANCE.md` instead). - [x] The license is OK: GPL-3.0-or-later, nothing incompatible and nothing decompiled. ### If applicable - [x] AI assistance recorded with `Assisted-by:` / `Co-Authored-By:` trailers. - [x] A game-free test added for the fix, failing on `main` and passing here. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contents
All checks were successful
PR description / Description follows the template (pull_request_target) Successful in 4s
Website / Website on Cloudflare (pull_request_target) Has been skipped
eb8ca08fce
Three changes to nx2/build.py:

- A4: a link whose inputs are the last successful one's is not run again
  (run_link). Its key, beside the output as <output>.linkkey: the command,
  the compiler and the work folder's system-headers stamp, each object's
  key (.o.sha1), and the contents of the other files it names, inside
  options too (the hot symbol order). A build with nothing to do went from
  16 s to 6 s on Windows. Players' builds let sa/build go, so it helps a
  work folder that keeps its files (the checkout's work/,
  LONGSHOT_KEEP_BUILD_FILES=1).
- A8: <build>/compile_commands.json, every compile as compile_one runs it,
  for clangd and clang-tidy (git-ignored at the checkout's top).
- The hot pieces' keys name the play profile by its contents
  (-DLONGSHOT_PROFILE_SHA1), not its path: a build from a checkout other
  than the work folder's compiled all eight again (~10-20 minutes). This
  changes their keys once.

tests/test_link_skip.py (a stand-in linker: skipped, relinked on a changed
object, library, symbol order or output, a failed link forgets the key;
the compile commands) and tests/test_profile_path_key.py.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
dyst changed title from perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contents to WIP: perf(build): skip unchanged links, write compile_commands.json, key the hot pieces by the profile's contents 2026-11-13 01:13:53 +00:00
Author
Member

Back 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.

Back 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.
Author
Member

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 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.
Author
Member

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).

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).
Author
Member

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.

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.
mooki left a comment

Read head eb8ca08f against main cf605c85: 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

  • The cost still exists on main: both standalone links run again even when every object is cached, and the hot-piece key still includes the profile's path. The compile database uses the same argument list as compile_one, including cached jobs.
  • The profile change retains the content digest already added by the hot-flag producer. A changed profile still changes the key. A failed link removes its old success record before starting, and a missing binary is linked again.
  • The description correctly keeps the old Windows timings separate from an unmeasured Longshot result.

Must change

  1. Include libraries resolved by the linker in the key. In nx2/build.py, lines 149-153, link_inputs only 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++, -lunwind and other -l options. 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.txt does not cover this: on Linux it hashes stdio.h and 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 / -l and checks that the next binary contains the new implementation. The actual linker selected by -fuse-ld=lld needs the same treatment; hashing the compiler driver alone does not notice a linker-only update.

  2. 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 .map output 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.py then cannot name that module's sampled frames, and nx2/samap.py cannot open longshot.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

  • This head is 117 commits behind main, with one commit of its own, from base 82de567a. It nevertheless merges cleanly with cf605c85; the WIP mergeability flag is not a conflict. Main's intervening build.py changes 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.
  • The displayed 16 s versus 6 s is a useful lead, not a measured Longshot gain. It has no named comparison commits, run count, alternating order, median or spread. Keep the WIP qualification until the same PC has at least two alternating runs per named commit, with every run, the median and the spread retained. Include an unchanged build, a renderer edit, and the same-profile second-checkout case. State whether build outputs were retained: normal cleanup of a separate work folder removes the build directory and therefore its link records and compile database.
  • The new link test honestly checks invocation counts, but its stand-in always writes the same bytes. It does not establish that a real linker selected the same libraries or produced the right binary. The existing comma-path fixture passed its Linux, MSVC-target and MinGW-target links with Clang; those are cross-link checks, not Windows execution.
  • Correct the description's game-DLL claim. --dll still calls subprocess.run at nx2/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 supplies dev.py compdb; this PR writes the database but does not add that command.

Overlaps

  • #24 changes the object-cache location and publication in the same build file. #35 changes the source-key cache beside flag_text. Both merge pairwise with this head; keep their object-key and failure behavior when combining them.
  • #28 consumes the compile database produced here, while #22 changes the lift key. Neither conflicts with this head in the pairwise merges checked.
  • #357 changes lift workers; #358 and #314 change prompt generation and reuse; #360 changes game-file reuse. They affect different stages and also merge pairwise with this head. No combined-stack build or timing was checked.

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_jobs and test_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.

Read head `eb8ca08f` against main `cf605c85`: 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 - The cost still exists on main: both standalone links run again even when every object is cached, and the hot-piece key still includes the profile's path. The compile database uses the same argument list as `compile_one`, including cached jobs. - The profile change retains the content digest already added by the hot-flag producer. A changed profile still changes the key. A failed link removes its old success record before starting, and a missing binary is linked again. - The description correctly keeps the old Windows timings separate from an unmeasured Longshot result. ### Must change 1. **Include libraries resolved by the linker in the key.** In `nx2/build.py`, lines 149-153, `link_inputs` only 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++`, `-lunwind` and other `-l` options. 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.txt` does not cover this: on Linux it hashes `stdio.h` and 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` / `-l` and checks that the next binary contains the new implementation. The actual linker selected by `-fuse-ld=lld` needs the same treatment; hashing the compiler driver alone does not notice a linker-only update. 2. **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 `.map` output 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.py` then cannot name that module's sampled frames, and `nx2/samap.py` cannot open `longshot.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 - This head is **117 commits behind main**, with one commit of its own, from base `82de567a`. It nevertheless merges cleanly with `cf605c85`; the WIP mergeability flag is not a conflict. Main's intervening `build.py` changes 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. - The displayed **16 s versus 6 s** is a useful lead, not a measured Longshot gain. It has no named comparison commits, run count, alternating order, median or spread. Keep the WIP qualification until the same PC has at least two alternating runs per named commit, with every run, the median and the spread retained. Include an unchanged build, a renderer edit, and the same-profile second-checkout case. State whether build outputs were retained: normal cleanup of a separate work folder removes the build directory and therefore its link records and compile database. - The new link test honestly checks invocation counts, but its stand-in always writes the same bytes. It does not establish that a real linker selected the same libraries or produced the right binary. The existing comma-path fixture passed its Linux, MSVC-target and MinGW-target links with Clang; those are cross-link checks, not Windows execution. - Correct the description's game-DLL claim. `--dll` still calls `subprocess.run` at `nx2/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 https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/28 as the companion that supplies `dev.py compdb`; this PR writes the database but does not add that command. ### Overlaps - https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/24 changes the object-cache location and publication in the same build file. https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/35 changes the source-key cache beside `flag_text`. Both merge pairwise with this head; keep their object-key and failure behavior when combining them. - https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/28 consumes the compile database produced here, while https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/22 changes the lift key. Neither conflicts with this head in the pairwise merges checked. - https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/357 changes lift workers; https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/358 and https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/314 change prompt generation and reuse; https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/360 changes game-file reuse. They affect different stages and also merge pairwise with this head. No combined-stack build or timing was checked. ### 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_jobs` and `test_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.
HumanBean left a comment

Code review of head eb8ca08f against main f804b7b8 (#385 merged). I read the diff and the code around it, and checked the merged tree with git 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++, the lld that -fuse-ld picks) 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).

  • Hot pieces keyed by the profile's contents (flag_text): main doesn't cover this, and it is worth more now than when it was written. make_game.py sets RX_PGO=<work>/longshot_pieces.prof, so the hot pieces' key names the work folder's path. Main's nx2/object_store.py docstring says "the object keys hold no folder path". For the eight hot pieces that is false today, so neither the object store from 371 nor dev_packs.link_objects can share them between work folders. This part makes it true. It doesn't interact with 385: split_fl goes only to the other lifted chunks, and the hot pieces keep hot_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.
  • Skipping unchanged links (run_link / link_key): main doesn't cover this; 371 shares objects, not links. It works with the store's hard links: link_key reads each object's .o.sha1 stamp, which compile_one writes the same way for a fetched object and a compiled one, and system_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 from main(), before the pool starts, so parallel compiles can't race on it. Since 385, compile_one compiles a temporary split_giants.compile_copy, not src, 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

  1. tests/test_link_skip.py fails once rebased. test_the_compile_commands_are_the_compiles looks for '[cc] + fl + ["-c", src, "-o", obj]' in compile_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 at src with the split flag is the right choice: the copy is deleted after the compile. Just say so.
  2. mooki's two points (the libraries and the .map).

Should change

  1. The output check is size only. A file of the same size written over the output keeps a stale binary. Recording st_mtime_ns (and the inode) beside the size costs nothing (inline).
  2. Describe the one-time cost accurately. Changing flag_text also changes kept_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, returns KEPT_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.
  3. nx2/dev.py compdb doesn't exist on main. The .gitignore comment and the write_compdb docstring point at it. It comes from #28, which is still open. Reword both, or say this lands after 28.
  4. clangd will background-index every entry, about 1.4 GB of lifted C under gen/. That is heavy for anyone who opens the work folder's database. Consider leaving the lifted chunks out, or document a .clangd Index: Background: Skip for them.

Description and checklist

  • The description is WIP-accurate: Rebased, run_all.py and Measured are unticked with reasons, and the numbers are labelled as the GitHub version's. Once rebased and measured on Longshot, it needs columns named main @ <hash> / this PR @ <hash>, at least two interleaved runs, and the median.
  • What changed still says the game DLL links through run_link. It doesn't: --dll still calls subprocess.run, as mooki noted.
  • After the rebase, add A check under tests/ that reads changed source updated in this PR if any existing check needs it. Docs updated applies too: docs/development.md should name compile_commands.json and <output>.linkkey, including how to force a link by deleting it.
  • Keep A game-free test added. Once it passes on the rebased tree it holds, since both new checks fail on main for lack of the functions.

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.

Code review of head `eb8ca08f` against main `f804b7b8` (https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/385 merged). I read the diff and the code around it, and checked the merged tree with `git merge-tree`. I ran no tests, no build and no game. mooki's request for changes (https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/30#issuecomment-17425) still stands, and I agree with both of its must-change points: the libraries the linker finds by search path (`-l…`, `-static-libstdc++`, the `lld` that `-fuse-ld` picks) 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 https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/371 and https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/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). - **Hot pieces keyed by the profile's contents (`flag_text`):** main doesn't cover this, and it is worth more now than when it was written. `make_game.py` sets `RX_PGO=<work>/longshot_pieces.prof`, so the hot pieces' key names the work folder's path. Main's `nx2/object_store.py` docstring says "the object keys hold no folder path". For the eight hot pieces that is false today, so neither the object store from 371 nor `dev_packs.link_objects` can share them between work folders. This part makes it true. It doesn't interact with 385: `split_fl` goes only to the other lifted chunks, and the hot pieces keep `hot_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. - **Skipping unchanged links (`run_link` / `link_key`):** main doesn't cover this; 371 shares objects, not links. It works with the store's hard links: `link_key` reads each object's `.o.sha1` stamp, which `compile_one` writes the same way for a fetched object and a compiled one, and `system_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 from `main()`, before the pool starts, so parallel compiles can't race on it. Since 385, `compile_one` compiles a temporary `split_giants.compile_copy`, not `src`, 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 1. **`tests/test_link_skip.py` fails once rebased.** `test_the_compile_commands_are_the_compiles` looks for `'[cc] + fl + ["-c", src, "-o", obj]'` in `compile_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 at `src` with the split flag is the right choice: the copy is deleted after the compile. Just say so. 2. mooki's two points (the libraries and the `.map`). ### Should change 3. **The output check is size only.** A file of the same size written over the output keeps a stale binary. Recording `st_mtime_ns` (and the inode) beside the size costs nothing (inline). 4. **Describe the one-time cost accurately.** Changing `flag_text` also changes `kept_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, returns `KEPT_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. 5. **`nx2/dev.py compdb` doesn't exist on main.** The `.gitignore` comment and the `write_compdb` docstring point at it. It comes from https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/28, which is still open. Reword both, or say this lands after 28. 6. **clangd will background-index every entry**, about 1.4 GB of lifted C under `gen/`. That is heavy for anyone who opens the work folder's database. Consider leaving the lifted chunks out, or document a `.clangd` `Index: Background: Skip` for them. ### Description and checklist - The description is WIP-accurate: *Rebased*, *run_all.py* and *Measured* are unticked with reasons, and the numbers are labelled as the GitHub version's. Once rebased and measured on Longshot, it needs columns named `main @ <hash>` / `this PR @ <hash>`, at least two interleaved runs, and the median. - *What changed* still says the game DLL links through `run_link`. It doesn't: `--dll` still calls `subprocess.run`, as mooki noted. - After the rebase, add *A check under tests/ that reads changed source updated in this PR* if any existing check needs it. *Docs updated* applies too: docs/development.md should name `compile_commands.json` and `<output>.linkkey`, including how to force a link by deleting it. - Keep *A game-free test added*. Once it passes on the rebased tree it holds, since both new checks fail on main for lack of the functions. **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 ----
Owner

nx2/dev.py compdb isn't on main. It comes from #28, still open. Reword this, or land after 28 (same for write_compdb's docstring).

`nx2/dev.py compdb` isn't on main. It comes from https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/28, still open. Reword this, or land after 28 (same for `write_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))]:
Owner

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. Record st_mtime_ns and st_ino with the size (line 200) and compare all three, and also require the .map here (mooki's point 2).

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. Record `st_mtime_ns` and `st_ino` with the size (line 200) and compare all three, and also require the `.map` here (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 clangd
and 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's
gen/ 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]}
Owner

Since 385, split chunks are compiled from a temporary copy (split_giants.compile_copy), not src. Naming src with 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 leaving gen/ chunks out, or document the .clangd setting that skips them.

Since 385, split chunks are compiled from a temporary copy (`split_giants.compile_copy`), not `src`. Naming `src` with 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 leaving `gen/` chunks out, or document the `.clangd` setting 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>"
Owner

Main still needs this: make_game.py sets RX_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) and dev_packs.link_objects can't share them. It's worth saying that in the description. One more cost to name under Risk: kept_part uses flag_text, so a player with a kept translation gets KEPT_STALE once and the game is translated again, not only the eight hot pieces compiled.

Main still needs this: `make_game.py` sets `RX_PGO=<work>/longshot_pieces.prof`, so without it the hot pieces' key names the work folder, and https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/371's store (whose docstring says the keys hold no folder path) and `dev_packs.link_objects` can't share them. It's worth saying that in the description. One more cost to name under Risk: `kept_part` uses `flag_text`, so a player with a kept translation gets `KEPT_STALE` once 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)
Owner

This fails on main as it is: since #385, compile_one runs [cc] + fl + ["-c", c_src, "-o", obj] (a split_giants.compile_copy when the split flag is set). Check what the database should say instead: the flags are compile_one's, and the file is the source, not the temporary copy. Update the comment too.

This fails on main as it is: since https://git.magicbean.dev/MagicBeanDev/Longshot/pulls/385, `compile_one` runs `[cc] + fl + ["-c", c_src, "-o", obj]` (a `split_giants.compile_copy` when the split flag is set). Check what the database should say instead: the flags are `compile_one`'s, and the file is the source, not the temporary copy. Update the comment too.
All checks were successful
PR description / Description follows the template (pull_request_target) Successful in 4s
Website / Website on Cloudflare (pull_request_target) Has been skipped
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u 'perf/link-skip:dyst-perf/link-skip'
git switch 'dyst-perf/link-skip'
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
3 participants
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
MagicBeanDev/Longshot!30
No description provided.