Remember a POLY parameter's hand-on answers per query in fwd_param_appends - #7390
Merged
Merged
Conversation
…er method and depth fwd_poly_param_handed_on and fwd_param_appends follow a POLY parameter through the calls and supers it is handed to, four levels deep, to see whether one of them appends to it. Only a rest's answer was remembered (fwd_rest_bits); a plain parameter was asked afresh along every path, so a value handed to many methods, each handing it on to many, cost the number of paths up to the bound -- fan-out to the fifth power, with the callee's whole call list walked at each leaf. A synthetic program of eight layers of 32 methods, each handing its two POLY parameters to 32 methods of the next layer, spent 108 s here; a 141k-line machine-generated program compiles 70-80 s faster with this change. Each ask from outside now remembers, for that ask only, what each (method, parameter, depth) answered and which bound-cut flags it left, and replays both on a repeat. The depth stays in the key because the bound cuts a deeper ask shorter, and nothing is remembered while a rest forwarder is being asked, where an answer leans on that forwarder's partial bits. The passes between asks change the types the answers read, so each ask starts empty. The generated C is 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; 0 remain after this review. 📝 WalkthroughWalkthroughForwarding analysis now memoizes POLY-parameter results by method, parameter, and recursion depth for each query. Cached results include taint. The analyzer bypasses memoization during rest-forwarder evaluation and retains the existing mutation check and depth limit. ChangesForwarding analysis memoization
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Refactor · Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant fwd_poly_param_handed_on
participant fwd_param_appends
participant ForwardingMemo
participant an_param_mutated_in_place
fwd_poly_param_handed_on->>ForwardingMemo: Start a query at depth 0
fwd_poly_param_handed_on->>fwd_param_appends: Check the forwarded parameter
fwd_param_appends->>ForwardingMemo: Look up method, parameter, and depth
alt Cached result
ForwardingMemo-->>fwd_param_appends: Return result and taint
else Cache miss
fwd_param_appends->>an_param_mutated_in_place: Check in-place mutation
fwd_param_appends->>ForwardingMemo: Store result and taint
end
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This change speeds up forwarding analysis in the compiler by caching repeated results within a single query. No actionable merge-blocking risk was found. The author reports identical generated output across the corpus. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 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
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>
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.
Fixes #7389.
fwd_poly_param_handed_onandfwd_param_appendsfollow a POLY parameter through the calls andsupers it is handed to. A plain parameter's answer was not remembered, so the same (method, parameter, depth) was asked once per call path reaching it.Change
src/analyze.conly. Each ask from outside (fwd_param_appendsorfwd_poly_param_handed_onat depth 0, with no rest forwarder being asked) starts a fresh memo, an open-addressed table stamped with a generation. Within the ask,fwd_param_appendsremembers, for each (method, parameter, depth), its answer and the cut-short flags (g_fwd_taint) its walk left, and on a repeat returns the answer and sets the same flags.g_fwd_taintexactly as asking again would;fwd_rest_bits' OPEN flag and the emitters' -1 answers depend on them.g_fwd_rest_depth > 0): there an answer leans on that forwarder's partial bits.Results
Calls of
fwd_param_appendsfor the issue's generator,spinel -c(raise mode; measured with the memo foran_subtree_hands_to_appenderfrom a separate pull request applied -- the first two counts are the same without it):gen.rb 8 16 16(2,339 lines)gen.rb 8 24 24(4,851 lines)The 141k-line machine-generated program from the issue (four other slow passes disabled for the measurement): 283 s -> 201 s in raise mode, 295 s -> 228 s in promote mode, whole compile. Timings are from a heavily loaded machine; the call counts are exact.
The generated C does not change
test/*.rb(5,746 programs) compiled withspinel -cin both--int-overflowmodes, on master (e5e8f79) and on this change, built in turn in the same checkout and fed the same relative paths: the generated C, stderr and exit status are identical for all 11,492 compilations except the 8 (4 programs, 2 modes each) that embedRUBY_DESCRIPTION, whose only difference is the revision string.Gate
make gatewas run on 92510d6 with this change merged together with other fixes from the same investigation (#7375, #7377, #7378, #7384, and others):Promote mode
SPINEL_INT_OVERFLOW=promote make teston the same combined branch:Tests: 5909 pass, 15 fail, 13 error. All 28 failing tests are among the 29 that master fails in promote mode at e5e8f79; none is new. The 29th (raise_rejects_invalid_arguments) no longer fails, which appears to come from master moving from e5e8f79 to 92510d6.Summary by CodeRabbit