perf(codegen): elide the length computation of a statement-position push - #7600
Conversation
|
Warning Review limit reached
Next review available in: 7 seconds You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughArray push lowering now receives expression-discard state. Discarded pushes skip updated-length computation and return a zero placeholder. Consumed pushes preserve their new-length result across numeric, object, spread, boxed, and captured-array paths. Regression tests cover these contexts. ChangesArray push discarded-result handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-codegen/src/expr/array_push.rs`:
- Around line 28-49: Update emit_array_handle_length so value_discarded does not
bypass observable Proxy length access: ensure the push runtime helper performs
and preserves the required length lookup/trap before discarding the result, or
guard the optimization to proven ordinary arrays only. Add a regression case
using a Proxy length trap that records calls and throws, confirming the trap
still executes for statement-position push calls.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f94b3193-fc7d-43b8-b594-77c803e91828
📒 Files selected for processing (4)
changelog.d/7600-discarded-push-length-elision.mdcrates/perry-codegen/src/expr/array_push.rscrates/perry-codegen/src/expr/dispatch.rstest-files/test_array_push_expression_value.ts
| /// The expression's result: the new length per ES2024 `Array.prototype.push`. | ||
| /// | ||
| /// `js_array_length` is NOT a field read — it resolves Proxy arrays through | ||
| /// the `get` trap and probes the registered-Set/Map side tables — and a | ||
| /// statement-position `arr.push(x);` discards its result, so on push-heavy | ||
| /// workloads it was 8–13% of the run computing a number nobody reads. | ||
| /// `value_discarded` is the `mem::take`n per-expression signal from | ||
| /// `dispatch::lower_expr` (#7590: it reaches exactly the statement's own | ||
| /// expression, never an operand — a consumed `n = arr.push(x)` always | ||
| /// computes the real length). When set, the placeholder constant is returned | ||
| /// without emitting the call. | ||
| fn emit_array_handle_length( | ||
| ctx: &mut FnCtx<'_>, | ||
| array_handle: &str, | ||
| value_discarded: bool, | ||
| ) -> String { | ||
| if value_discarded { | ||
| return double_literal(0.0); | ||
| } | ||
| let blk = ctx.block(); | ||
| let len_i32 = blk.call(I32, "js_array_length", &[(I64, array_handle)]); | ||
| blk.sitofp(I32, &len_i32, DOUBLE) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve observable Proxy get traps.
Line 30 states that js_array_length resolves Proxy arrays through the get trap. Lines 44-45 skip that operation for discarded results. A discarded JavaScript result can skip only unobservable work. It cannot skip a trap that can run user code or throw.
Preserve the required Proxy behavior before returning the placeholder. Return the length from the push runtime helper, or elide the lookup only after proving that the receiver is an ordinary array. Add a regression case with a length trap that records calls and throws.
The PR objective requires Proxy behavior to be preserved.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-codegen/src/expr/array_push.rs` around lines 28 - 49, Update
emit_array_handle_length so value_discarded does not bypass observable Proxy
length access: ensure the push runtime helper performs and preserves the
required length lookup/trap before discarding the result, or guard the
optimization to proven ordinary arrays only. Add a regression case using a Proxy
length trap that records calls and throws, confirming the trap still executes
for statement-position push calls.
arr.push(x) evaluates to the new length, computed by js_array_length -- which is NOT a field read: it resolves Proxy arrays through the `get` trap and probes the registered-Set/Map side tables. A statement-position push discards that result, so push-heavy workloads spent 8-13% of the run computing a number nobody reads (measured on push_cls; json_pipeline's build_out loop is the same shape). Gated on the mem::take'n per-expression signal from dispatch::lower_expr (#7590/#7591): it reaches exactly the statement's own expression and never an operand, so a consumed `n = arr.push(x)` always computes the real length. All eight push/push-spread return points thread it as a parameter -- reading the field there would reintroduce the #7590 leak in a subtler form, since handlers consult it after lowering operands. test_array_push_expression_value.ts consumes the value in call-argument, assignment, arithmetic, conditional, nested-push, object-element, spread and boxed-array position -- the discarded form keeps working no matter what, so only a consuming test can catch a regression here (#7590's lesson). Verified in traced IR: the discarded loop push emits zero js_array_length calls, the consumed push exactly one per specialization.
f70beef to
cf880a5
Compare
Audit before merge — verified, merged as v0.5.1342IR mechanism confirmed on my own probe (discarded loop push + one consumed The decisive sabotage is the one that recreates #7590, and the matrix test That is the point of a consuming test matrix — the discarded form worked the My probe showed 1.08× rather than 2.77× — expected: it grows a 20M-element Note for the record: the promised gap-suite comment was not posted (same as |
Gap suite result (as promised at filing)Full run on this branch (which also carries the merged #7594/#7596 GC pacing changes, so this doubles as their post-merge gap verification — the run at #7596 filing time was voided by a deleted worktree):
|
1 participant
Follow-up to #7590/#7591 — the perf half that was deliberately kept out of the correctness fix.
What
arr.push(x)evaluates to the new length, computed byjs_array_length— which is not a field read: it resolves Proxy arrays through thegettrap and probes the registered-Set/Map side tables. A statement-positionarr.push(x);discards that result, so push-heavy code pays an out-of-line runtime call per push for a number nobody reads (8–13% of push-heavy runs;json_pipeline'sbuild_outloop is exactly this shape — 1 source-level.lengthbut 458 emitted call sites, one inside the hot loop ⇒ 20M calls).This PR elides the length computation when — and only when — the push is the statement's own expression.
Why this is safe now (and was not before #7591)
The gate is the
mem::taken per-expression signaldispatch::lower_exprintroduced in #7591. It reaches exactly the statement's own expression and never an operand, son = arr.push(x),sink(a.push(10)),a.push(1) + 100,a.push(1) > 0 ? 7 : 9all compute the real length. Gating on the rawctx.discard_expr_valuefield is precisely the #7590 bug — it produced wrong values in all four of those positions when I first tried this as a perf change, which is what led to finding #7590 in the first place.All eight push/push-spread return points receive the signal as a parameter, not a field read — handlers consult it after lowering their operands, by which point the field has been taken again.
Verified live in the IR
--trace llvmon a probe with a discarded loop push + one consumed push: the discarded push emits zerojs_array_lengthcalls; the consumed push exactly one per function specialization. (A green behavior test alone would be vacuous — the discarded form works whether or not the elision fires.)Measurements
Interleaved A/B, each arm linked by its own compiler+runtime pair (separate target dirs, identical package sets, both runtimes current
main):Pure statement-position push loop (20M pushes, runtime-dependent bounds, escaping array):
2.77× — more than the call itself: the out-of-line call was also blocking loop optimization around it. Both arms compute identical results.
json_pipeline(shipped workload, 200k records): 2,495 → 2,423 ms total (~3 %), output hash identical — modest because that workload is GC-bound (#7592); its statement-positionout.push({...})is exactly the elided shape.Testing
test-files/test_array_push_expression_value.tsconsumes the push value in call-argument, assignment, arithmetic, conditional, nested-push, object-element, spread, and boxed-array (runtime fall-through path) position, plus discarded forms that must keep pushing. Byte-identical tonode --experimental-strip-types.That matrix is the point (#7590's lesson): the discarded form kept working the entire time the original bug was live, so only a consuming test can catch a signal leak.
perry-codegentest suite: 8 targets, all green (671 tests in the main target), 0 failures.cargo fmt --check,scripts/check_file_size.shclean.Summary by CodeRabbit
Performance
array.push()results by avoiding unnecessary length calculations.push()results are used.Bug Fixes
Tests
push()in assignments, conditions, arithmetic, function calls, and other expression contexts.