Repository navigation
Joining an element kind into a boxed parameter does not end the boxed-source walk's generation - #7384
Merged
matz merged 4 commits intoOct 5, 2026
Conversation
…ady found unchanged widen_boxed_array_sources (with widen_boxed_elem_sources) follows every write of a local to depth 6 with nothing to remember what it already walked, so a function writing one local W times walked W^6 paths, from every boxed push or index write and every call binding a boxed argument whose parameter takes stores. A large machine-generated program (6.9k lines) that writes the same few locals thousands of times, and shifts them as `l0 << 1`, which counts as a push, spent 45 of its 48 seconds of C generation here. The walk writes the analysis state as it goes, so only a visit that changed nothing is remembered: under the current generation, with the depth it ran at. A later visit of the same node, at that depth or deeper and in the same generation, makes a subset of the same calls in the same state, which change nothing either, and is skipped. Each walk from outside opens and closes a generation, and so does every call in it that may write the state, whether or not it reports a change (the pin a local re-derived from its writes takes again writes without one). The widenings, and the generated C, are unchanged.
…found unchanged With each walk remembering only what it found itself, every call site binding a boxed argument walked the caller's locals again, and a read of a local walked the local's writes again from every read: a function of W writes and C calls still cost C * W^2 per pass. A large machine-generated program (53k lines), where one function calls the same callee thousands of times with the same locals, stayed over two minutes in this walk alone. A boxed local now has its own record, since every read of it walks the same, and the records carry the element kind they were made for. Within infer_param_types the walks share one generation, which ends wherever the pass reports a change: after each node it binds, and before each walk in bind_args_params once the binding has changed something, as well as at every write in the walk itself. The work is linear in the nodes and locals reached per generation. A write between two walks that the pass does not report would make a shared record stale. With SPINEL_WBAS_SHADOW=1, every visit skipped on another walk's record is made anyway, in a fresh generation, and the compiler stops if it widens or writes anything. The test corpus (5,746 tests, both overflow modes) and large machine-generated programs run clean under it, and the generated C is unchanged.
…rce generation Within infer_param_types the walks of widen_boxed_array_sources share their records until the pass reports a change, and the generation moved only at the end of each node and, inside bind_args_params, before a walk once that binding had changed something. A node that binds more than once (a method and its overrides, each initialize a dynamic `new` may reach, the candidates of a poly call) could change something after the last walk of one binding, or between two bindings, and the next binding then walked on records made before that change. bind_args_params now ends the generation as it returns if it changed anything, and as it starts if the pass changed anything since the last step (the pass hands it its own `changed`), and the direct ivar write in bind_dynamic_new_initializers ends it too. This only ends generations sooner, so it can only make the walks skip less.
…-source walk's generation Every write in widen_boxed_array_sources started a new memo generation, including the join of the store's element kind into a boxed parameter's boxed_push_elem and boxed_known_elem. In the first round after the optimistic ones nearly every walk reaches a parameter that has not taken the kind yet, so the memo was dropped again and again: a 289k-line machine-generated program was still in that round 150 seconds into its compilation. The walk reads those two fields nowhere but at that join, and the join only grows (UNKNOWN, one kind, then the boxed kind). A visit recorded as changing nothing found the kind already there, and finds it there after any later join too; the records also carry the element kind, so a parameter a different kind reaches later is walked as a different visit. The join therefore starts no generation. It still reports the change, so SPINEL_WBAS_SHADOW=1 still catches a skipped visit that would have joined. The widenings and the generated C are unchanged.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe analysis pass adds generation-aware memoization for boxed-source walks. Parameter inference tracks changes across bindings and advances shared generations when analysis state changes. ChangesBoxed-source memoization
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to No actionable issue was established in the changed analysis paths; the PR is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to This internal optimization does not introduce an identified public interface, privilege expansion, or trust-boundary bypass. The inspected lifecycle rules support sequential execution, but concurrent or reentrant execution remains unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Oct 5, 2026
Following a call into its callee's value scans every ReturnNode and every scope of the program
#7403
Closed
kieranklaassen
pushed a commit
to kieranklaassen/spinel
that referenced
this pull request
Oct 5, 2026
… compiler `h[:a1] = h[:a0]; h[:a2] = h[:a1]; ...` with a String of h changed in place cost the compiler n! walks for n such stores: on f8abc3e eight take 12 seconds, and nine do not finish in 90. The same in an Array's slots, in a Hash an instance variable holds, in one a parameter holds, and in two Hashes that store each other's elements. The walk that demands a container's Strings as handles follows a boxed stored value to the element read it is, and strbuf_demand_elem_arg starts the walk of that read's container. Since matz#7369 a read whose walk is under way answers 0, which ended the recursion. A second read of the same container was still another read, so the stores were walked once more for it with one read fewer left to meet, and again inside that. The walk is of the stores into the container a local or an instance variable holds, whichever element is read. A read of that container met inside the walk now answers 0 as the same read does: the walk on the stack reaches every store the inner one would. Each of the n stores walks the others once. An element of another element (`r[0][:b] = r[0][:a]`) is keyed by its read as before; that walk did not grow this way. The nearest changes upstream are matz#7369, which this completes, and the walk-speed changes merged since (matz#7365's memo and the per-method one beside it, matz#7384, matz#7386, matz#7388, matz#7390, matz#7392, matz#7396 to matz#7402). Those make one walk cheaper or remember a walk that changed nothing; none touches this guard, and here it is the number of walks that grows. A walk that reaches a container moves the memo's generation whether it changed anything or not, so the memo never holds one of these. The generated C is unchanged for every other test. Co-Authored-By: Claude Code <noreply@anthropic.com>
kieranklaassen
pushed a commit
to kieranklaassen/spinel
that referenced
this pull request
Oct 5, 2026
… compiler `h[:a1] = h[:a0]; h[:a2] = h[:a1]; ...` with a String of h changed in place cost the compiler n! walks for n such stores: on c6bbdfb eight take 13 seconds, and nine do not finish in 90. The same in an Array's slots, in a Hash an instance variable holds, in one a parameter holds, and in two Hashes that store each other's elements. The walk that demands a container's Strings as handles follows a boxed stored value to the element read it is, and strbuf_demand_elem_arg starts the walk of that read's container. Since matz#7369 a read whose walk is under way answers 0, which ended the recursion. A second read of the same container was still another read, so the stores were walked once more for it with one read fewer left to meet, and again inside that. The walk is of the stores into the container a local or an instance variable holds, whichever element is read. A read of that container met inside the walk now answers 0 as the same read does: the walk on the stack reaches every store the inner one would. Each of the n stores walks the others once. An element of another element (`r[0][:b] = r[0][:a]`) is keyed by its read as before; that walk did not grow this way. The nearest changes upstream are matz#7369, which this completes, and the walk-speed changes merged since (matz#7365's memo and the per-method one beside it, matz#7384, matz#7386, matz#7388, matz#7390, matz#7392, matz#7396 to matz#7402). Those make one walk cheaper or remember a walk that changed nothing; none touches this guard, and here it is the number of walks that grows. A walk that reaches a container moves the memo's generation whether it changed anything or not, so the memo never holds one of these. The generated C is unchanged for every other test. Co-Authored-By: Claude Code <noreply@anthropic.com>
2 of 4 tasks
kieranklaassen
pushed a commit
to kieranklaassen/spinel
that referenced
this pull request
Oct 5, 2026
… compiler `h[:a1] = h[:a0]; h[:a2] = h[:a1]; ...` with a String of h changed in place cost the compiler n! walks for n such stores: on c6bbdfb eight take 13 seconds, and nine do not finish in 90. The same in an Array's slots, in a Hash an instance variable holds, in one a parameter holds, and in two Hashes that store each other's elements. The walk that demands a container's Strings as handles follows a boxed stored value to the element read it is, and strbuf_demand_elem_arg starts the walk of that read's container. Since matz#7369 a read whose walk is under way answers 0, which ended the recursion. A second read of the same container was still another read, so the stores were walked once more for it with one read fewer left to meet, and again inside that. The walk is of the stores into the container a local or an instance variable holds, whichever element is read. A read of that container met inside the walk now answers 0 as the same read does: the walk on the stack reaches every store the inner one would. Each of the n stores walks the others once. An element of another element (`r[0][:b] = r[0][:a]`) is keyed by its read as before; that walk did not grow this way. The nearest changes upstream are matz#7369, which this completes, and the walk-speed changes merged since (matz#7365's memo and the per-method one beside it, matz#7384, matz#7386, matz#7388, matz#7390, matz#7392, matz#7396 to matz#7402). Those make one walk cheaper or remember a walk that changed nothing; none touches this guard, and here it is the number of walks that grows. A walk that reaches a container moves the memo's generation whether it changed anything or not, so the memo never holds one of these. The generated C is unchanged for every other test. Co-Authored-By: Claude Code <noreply@anthropic.com>
kieranklaassen
pushed a commit
to kieranklaassen/spinel
that referenced
this pull request
Oct 5, 2026
… compiler `h[:a1] = h[:a0]; h[:a2] = h[:a1]; ...` with a String of h changed in place cost the compiler n! walks for n such stores: on c6bbdfb eight take 13 seconds, and nine do not finish in 90. The same in an Array's slots, in a Hash an instance variable holds, in one a parameter holds, and in two Hashes that store each other's elements. The walk that demands a container's Strings as handles follows a boxed stored value to the element read it is, and strbuf_demand_elem_arg starts the walk of that read's container. Since matz#7369 a read whose walk is under way answers 0, which ended the recursion. A second read of the same container was still another read, so the stores were walked once more for it with one read fewer left to meet, and again inside that. The walk is of the stores into the container a local or an instance variable holds, whichever element is read. A read of that container met inside the walk now answers 0 as the same read does: the walk on the stack reaches every store the inner one would. Each of the n stores walks the others once. An element of another element (`r[0][:b] = r[0][:a]`) is keyed by its read as before; that walk did not grow this way. The nearest changes upstream are matz#7369, which this completes, and the walk-speed changes merged since (matz#7365's memo and the per-method one beside it, matz#7384, matz#7386, matz#7388, matz#7390, matz#7392, matz#7396 to matz#7402). Those make one walk cheaper or remember a walk that changed nothing; none touches this guard, and here it is the number of walks that grows. A walk that reaches a container moves the memo's generation whether it changed anything or not, so the memo never holds one of these. The generated C is unchanged for every other test. Co-Authored-By: Claude Code <noreply@anthropic.com>
kieranklaassen
pushed a commit
to kieranklaassen/spinel
that referenced
this pull request
Oct 6, 2026
… compiler `h[:a1] = h[:a0]; h[:a2] = h[:a1]; ...` with a String of h changed in place cost the compiler n! walks for n such stores: on c6bbdfb eight take 13 seconds, and nine do not finish in 90. The same in an Array's slots, in a Hash an instance variable holds, in one a parameter holds, and in two Hashes that store each other's elements. The walk that demands a container's Strings as handles follows a boxed stored value to the element read it is, and strbuf_demand_elem_arg starts the walk of that read's container. Since matz#7369 a read whose walk is under way answers 0, which ended the recursion. A second read of the same container was still another read, so the stores were walked once more for it with one read fewer left to meet, and again inside that. The walk is of the stores into the container a local or an instance variable holds, whichever element is read. A read of that container met inside the walk now answers 0 as the same read does: the walk on the stack reaches every store the inner one would. Each of the n stores walks the others once. An element of another element (`r[0][:b] = r[0][:a]`) is keyed by its read as before; that walk did not grow this way. The nearest changes upstream are matz#7369, which this completes, and the walk-speed changes merged since (matz#7365's memo and the per-method one beside it, matz#7384, matz#7386, matz#7388, matz#7390, matz#7392, matz#7396 to matz#7402). Those make one walk cheaper or remember a walk that changed nothing; none touches this guard, and here it is the number of walks that grows. A walk that reaches a container moves the memo's generation whether it changed anything or not, so the memo never holds one of these. The generated C is unchanged for every other test. Co-Authored-By: Claude Code <noreply@anthropic.com>
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
2 participants
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.
Refs #7376 (stacked on #7378, which is stacked on #7377)
This pull request is stacked on #7378 (itself on #7377) and contains their commits with the same SHAs; review those first. Only the last commit is new here.
What changes
The memo of
widen_boxed_array_sourcesstarts a new generation at every write the walk makes, and one of those writes is the join of the store's element kind into a boxed parameter'sboxed_push_elemandboxed_known_elem. In the first round after the optimistic ones nearly every walk reaches a parameter that has not taken the kind yet, so the memo was dropped again and again. On a 289k-line machine-generated program the compiler was still in that round 150 seconds into its compilation; the rounds before it had taken a few hundred walk steps each.That join no longer starts a generation:
infer_write_container_usage, the entry condition inbind_args_paramsand the backstop inanalyze.c, are outside the walk).UNKNOWN, then one kind, then the boxed kind. A visit recorded as changing nothing found the kind already joined in, and still finds it after any later join.lv->type != TY_POLY) is of the parameter's type, whose writes still start a generation.The join still reports the change, so
SPINEL_WBAS_SHADOW=1still stops the compiler if a skipped visit would have joined something.Why the results do not change
SPINEL_WBAS_SHADOW=1 spinel -con everytest/*.rbof e5e8f79 (5,746 files × raise/promote = 11,492 compilations): no internal error.spinel -cwith The boxed-source walks of one parameter-binding pass share what they found unchanged #7378 and with this change, from the same worktree, with the same input paths, both overflow modes, everytest/*.rbof e5e8f79: the generated C is identical for all 11,492.Before / after
Walk steps per binding pass, counted with a local counter (not part of this change), in the first non-optimistic round:
The total over the whole compilation of the 18k-line program goes from 231,812 to 215,551 steps; the 289k-line program now gets through that round and the following ones (619k, 308k, 201k, 159k steps).
Testing
make gate:SPINEL_INT_OVERFLOW=promote make test: 5909 pass, 15 fail, 13 error. Every failure is one master e5e8f79 already had; none is new.The gate ran on a branch based on 92510d6 that combines this change (with #7377 and #7378 under it) with #7375 and the other compile-time fixes from the same investigation. #7378 has since gained a commit (99e1459, from review) that only ends the shared generation in more places; this branch was rebased onto it without conflicts.
Summary by CodeRabbit