WIP: perf(build): lifted chunks of ~4 MB, by guest address, besides chunks.txt's boundaries #31
No reviewers
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!31
Loading…
Reference in a new issue
No description provided.
Delete branch "dyst/Longshot:perf/smaller-chunks"
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 small update that changes a few of the game's functions still recompiles every lifted chunk that holds one, and a chunk is ~24 MB of C: a typical one takes ~227 s of compiler time. Splitting the lifted C into ~4 MB pieces by guest address makes that ~41 s, with no cost in play or in a full build.
What changed
sub_startsinnx2/chunks.pyadds a chunk boundary everySUB_STRIDE(0x40000) of guest address, at the first function at or after it, besidesimage/chunks.txt's stable boundaries;writebuilds itsBoundariesfrom both. They are guest addresses, so they stay put when a function grows: a change moves no boundary but its own chunk's.chunks.txtkeeps only the boundaries it had, and they are worked out again each lift from the same functions, so dropping this change later is clean.write: more than 1000 chunks (acode_NNNN.c) stops the lift with a message, sincesplit_big,split_hotandbuild.pymatch three-digit chunk names.tests/test_sub_chunks.py(new): the first function past each stride; a long function spans strides without a boundary inside it; a function found since moves one boundary at most; chunks end at them as well as at the saved boundaries;writeuses them and saves onlychunks.txt's; the stride makes chunks of a few MB.The first build after this compiles every lifted chunk once (all boundaries are new): for a player, that update takes about as long as a full build, once. Updates after it recompile only the ~4 MB pieces that changed.
Flags and settings
None.
Measurements
Measured on the GitHub version (https://github.com/NotRexed/Hookshot/pull/2890), not yet on Longshot: this PR stays WIP until it is.
Compiles: Ryzen 9 3900X, Windows 11 (as the GitHub description states it).
In the game: RTX 3080 (NVIDIA 621.14), Ryzen 9 3900X, 64 GB, Windows 11; Kokiri Forest from a save (the game logic bounds its frame on the CPU there); two interleaved pairs of 30 samples, the median of each.
The heavy-function cluster (old chunks 021-023) still takes 300-900 s per piece on this PR. No errors in either game run.
These two were measured with the author's other local build changes on the same side (this, the lift key's environment, link skips and others), each in a fresh player-like work folder, so they are not this PR alone:
The smaller pieces spread better over the cores.
Testing
Tested: the runs under Measurements (on the GitHub version) and the game-free checks this description names.
Not tested: Linux. The 1000-chunk guard (the game's code makes ~330 pieces).
Needs other testers:
mainwith the ~4 MB chunks (request to open)Screenshots
Nothing on screen.
Risk and rollback
The first build after this lands compiles every lifted chunk once, for players too. A stride that made more than 1000 chunks would stop the lift with a message rather than leave a chunk out. Nothing is saved to
chunks.txt, so reverting the commit is enough.Reviewer notes
Carried over from https://github.com/NotRexed/Hookshot/pull/2890 with the move to Longshot: the same change on Longshot's
main, with its rename applied. Its review and discussion are there.Checks run on the GitHub version: every check that reads the changed files (9), through
run_all.py, rebased on GitHub'smainof that day (with https://github.com/NotRexed/Hookshot/pull/2748's spawned emit), Windows: 8 pass;test_spawn_emitfails the same way onmainthere. 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. Part of the dev-loop proposal's work on small updates (https://github.com/NotRexed/Hookshot/pull/2636).Issues
None on Longshot. See https://github.com/NotRexed/Hookshot/pull/2636.
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): lifted chunks of ~4 MB, by guest address, besides chunks.txt's boundariesto WIP: perf(build): lifted chunks of ~4 MB, by guest address, besides chunks.txt's boundariesBack 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
a16d681aagainst maincf605c85: the complete diff, description, all four comments, the empty review list, issue #362, and the chunk writer, splitters, lift cache and compiler consumers; ran the new check and eight existing lift checks on Linux, including the spawned emitter with GCC and Clang. All passed. No game, full build or timing run was made.What's good
code_*.cfile.split_big, and stale numbered and hot pieces still have their cleanup paths. The new guard stops the lift before a completed lift key can be recorded.Must change (traced through the source, not run)
nx2/chunks.py, line 318, puttingsub_starts(L.funcs)intoBoundaries.startsalso disables its size-based recording when there is no existing boundary list.Boundaries.__le__, lines 54-60, returns through the nonemptystartsbranch, sob.recordedstays empty and line 373 writes an empty list. On the next directlift.py --writeinto the same folder, lines 316-317 callseed_from_gen, which reads the new small chunks and then saves their starts permanently. This violates the promised clean rollback: reverting the PR still loads those small boundaries. A newly discovered function just before one of them also adds a new boundary without removing the old one, rather than moving the temporary boundary. Keep the persistent size boundaries separate from the temporary address cuts, and distinguish an existing intentionally empty boundary file from an old lift that needs seeding. Add an actual two-lift test starting withoutchunks.txt, followed by a rollback or changed-function-list case. The current source-string check intests/test_sub_chunks.py, lines 49-54, passes without exercising this path. This is source-traced, not a reproduced full-build failure.Measurements and smaller points
The old timings do not yet establish this PR's gain or absence of runtime cost. The individual-chunk numbers have no named commits, repeated samples or spread. The full-build and update numbers include other changes, as the description says. The game table has two interleaved pairs on one PC, but no commit IDs, matching build settings or sample spread. Keep those as historical context; for readiness, publish current main / PR / main / PR results with only this patch differing, the median and range, and the number of compiler workers. Include both a clean build and a small update with the same retained build inputs.
Issue 362 supports a separate question, not an eight-compiler result for this patch alone. Its 63 s versus 127 s comparison changes both chunk size and compiler count, from four to eight, and covers a subset of the C. Main's
mem_needstill estimates each cold file as400 + 50 × MB of C + 250 × MB of its largest function; this PR does not change that admission policy. The issue's proposed replacement needs its own evidence. Likewise, this PR cuts only between functions. It cannot split the giant functions discussed in section 3, which explains why the description still reports very slow individual pieces. Link the issue and keep those claims separate: #362.Exercise the writer rather than only its inputs. The new tests check the address set and source strings, but not two successive writes, exact function coverage after regrouping, a hot function immediately before a boundary, or the 1000-chunk guard. The passing existing spawn test compares two worker modes, not old versus new chunking. A small made-up lifter can check these without the game.
Age and overlaps
This head is 117 main commits behind
cf605c85, with one commit of its own, based on82de567a. It nevertheless merges cleanly. Main has not changednx2/chunks.pysince that base, so the boundary issue above still applies; the old game/build measurements are not measurements of current main.chunks.txt. #35 changes the saved source-hash records. These concern reuse around the changed layout, not a competing chunk algorithm.Each of those eight heads merges cleanly with this head in isolation. This is not a check of all eight combined.
Not checked
No full suite, real lift, Windows execution, Linux game build, output-equivalence comparison on the game, or timing measurement here. The old GitHub PR, including its comments and reviews, now returns HTTP 404, so that earlier discussion could not be checked. The forge status lists a passing description check and a skipped website check, not a passing source suite. A behavioral regression for the saved-boundary issue and the isolated current-head builds and measurements above would settle the remaining questions.
Measured: what #31's pieces gain, and what holds them back (
mem_need)I measured this PR's pieces on Windows (Ryzen 5 5600, 6 cores, 16 GB; llvm-mingw clang 23.1.5, the build's exact flags for lifted chunks). The input was a full lift of the game, 75 chunks. Section 2 of #362 summarises it. Here are the details for this PR.
Pieces against whole chunks, on the same 204 MB of lifted C (3 reps each):
A piece peaks at ~280 MB, against ~1.45 GB for a whole chunk. Eight at once suits a 6-core, 16 GB PC; twelve gains little more and took free memory down to 3.3 GB.
What holds that back is the memory gate.
nx2/build.py mem_needcounts400 + 50 × MB of C + 250 × MB of the largest function. Measured against peak working sets:A formula that fits all 70 measured compiles (52 pieces, whole chunks, the giant):
Two notes:
mem_needcounts the split pieces of a giant. The two would need a small merge inmem_need, whichever lands second.The raw numbers (every compile, 1-second memory samples, and the fitting script) are on my PC; I can attach them if useful.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.