Repository navigation
The walk widening a boxed store's array sources skips a value it already found unchanged - #7377
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.
|
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; 5 remain after this review. 📝 WalkthroughWalkthrough
ChangesBoxed-array source traversal
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The memoization change is ready to merge after normal checks; no actionable risk remains from the reviewed traversal paths. Architecture SummaryArchitecture risk: 🟡 Medium · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
Reliability and maintainability
🚥 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 |
|
A correction to the description: the comparison of every |
|
My previous comment was wrong about this one: the comparison had in fact finished before the job was stopped, and only the last step (comparing the outputs) was missing. It is done now.
|
2 participants
Fixes #7376
What changes
widen_boxed_array_sources(withwiden_boxed_elem_sources,src/analyze_pass.c) follows a value back to where its arrays are built, to depth 6, and remembered nothing of what it had walked. Unlike a predicate, it writes as it goes: it widens typed arrays, pins locals and method values to the general Array, and joins the store's element kind into a boxed parameter. So it cannot simply remember its answers; it remembers only the visits that changed nothing.widen_arg_array(its fresh-array pin writes the local's type back even when it answers 0), the pin blocks (which also writelv->typewithout a change), and a boxed parameter's element kinds when they move. A walk from outside opens and closes a generation of its own, so nothing carries from one walk to the next.Why the results do not change
spinel -cwas run on master and on the change from the same worktree, with the same input paths, in both overflow modes:for(test/*.rbmatchingboxed|widen|method|proc|splice|poly_arr|push|lambda|for_), in both modes: the generated C is identical for 1,875 of the 1,876 compilations, and the remaining one is refused with the same message by both compilers. Machine-generated programs of 300 to 6,900 lines (9 programs, both modes): identical.The comparison of every
test/*.rbwith this change alone is running; its result will be added to this pull request as a comment.Before / after
The generator from the issue,
spinel -c, counting the bodies ofwiden_boxed_array_sourcesthat run (a local counter, not part of this change):The steps went from about 7.5 times per doubling of
Kto about 4 times: within one walk each node is now expanded a bounded number of times, but every call site still starts a walk of its own over the caller's locals. Sharing what one walk found between the walks of one binding pass makes it linear; that is #7378, stacked on this one. K = 128 goes from 79 s to 27 s with this change alone; the rest is mostly the walk fixed in #7375.A 6.9k-line machine-generated program: C generation 55 s → 4.3 s with this change and #7375.
(Timings are from a shared machine under load.)
Testing
make gate:The stamp line above names the
origin/masterthe clone had fetched when the gate finished (725905f); the tree the gate ran was based on e5e8f79, as described below.SPINEL_INT_OVERFLOW=promote make test: 5861 pass, 15 fail, 14 error. The 29 failures are exactly the set master e5e8f79 fails with (0 new, 0 fixed).The gate ran on a branch based on e5e8f79 that combines this change, and #7378 stacked on it, with the other compile-time fixes from the same investigation (#7375, #7365, #7367, #7369, since merged). This branch was then rebased onto 92510d6 without conflicts; after the rebase, the generated C for 72 programs (63 tests and 9 machine-generated programs) in both modes was checked again against master and is unchanged.
Summary by CodeRabbit