Fix quadratic slowdown - #304
Open
samestep wants to merge 2 commits into
Open
Conversation
Assisted-by: Claude:opus-5
Assisted-by: Claude:opus-5
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes #303 by dropping the invariant that a
NOMState'sforestRootsis sorted, and instead just sorting in the printer, allowing us to change it from aSeq DerivationIdwith linear removal time to aDerivationSetwith logarithmic removal time.As a result, nom's time-to-first-build in that issue's original
npbexample drops from 57m17s to 11m53s on my machine, and the synthetic example goes from quadratic to roughly linear:It's worth noting that this PR does introduce a minor performance regression in specific situations, since it causes the roots to be sorted every time we print a new state, which can cause more CPU usage if there are a lot of roots but the graph isn't changing much as we repeatedly print over a long period of time.
To test this, we can use the same
example.nixfrom #303. First, generate JSON to feed to nom:n=32000 { printf '@nix {"action":"msg","level":3,"msg":"these %d derivations will be built:"}\n' "$n" nix-instantiate example.nix --arg n "$n" 2>/dev/null | sed 's|.*|@nix {"action":"msg","level":3,"msg":" &"}|' printf '@nix {"action":"start","id":1,"level":6,"parent":0,"text":"building","type":105,"fields":["%s","localhost",1,1]}\n' "$(nix-instantiate example.nix --arg n 1 2>/dev/null)" } > plan.jsonlThen feed it to nom followed by an endless stream of "evaluating file" messages (triggering
withChange), so that nom keeps redrawing at its 60ms ceiling instead of slowing to 1000ms per frame:{ cat plan.jsonl q=\' i=0 while :; do printf '@nix {"action":"msg","level":5,"msg":"evaluating file %s/some/path/file-%d.nix%s"}\n' "$q" "$i" "$q" i=$((i + 1)) sleep 0.02 done } | nom --jsonAnd finally, in a separate terminal, measure nom's CPU usage for a minute:
Note that you should wait until nom has finished ingesting the entire build graph before running the measurement; the wait is only about 4 seconds using this PR, but roughly 40 seconds with version 2.2.0 of nom.
When I ran this on my machine using nom 2.2.0, I saw 17.05 seconds of CPU time:
And using this PR, I saw 23.28 seconds of CPU time:
So, roughly 40% more CPU usage in this example.
Finally, here's a script to print nom's last frame that contains a tree:
n=200 { printf '@nix {"action":"msg","level":3,"msg":"these %d derivations will be built:"}\n' "$n" nix-instantiate example.nix --arg n "$n" 2>/dev/null | sed 's|.*|@nix {"action":"msg","level":3,"msg":" &"}|' sleep 3 } | nom --json 2>&1 | awk -v RS='\033\\[\\?2026h' '/Dependency Graph/ {last = $0} END {printf "%s", last}' | sed 's/\x1b\[[0-9;?]*[a-zA-Z]//g; /Finished at/d'I got the same output with nom v2.2.0 and with this PR, showing that the sort order is preserved.
The regression check scripts in this PR description were written by Claude Opus 5.