perf(codegen): reach integer modulo fast path for i32 counters#6388
Conversation
📝 WalkthroughWalkthroughThe changes register WebSocket ChangesInteger modulo lowering
WebSocket manifest metadata
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.
🧹 Nitpick comments (1)
crates/perry-codegen/tests/native_proof_regressions/integer_modulo.rs (1)
1-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider adding unit test coverage for CI enforcement.
Per the coding guideline, acceptance coverage should be in unit tests visible to
cargo-testbecause integration suites undercrates/*/tests/*.rsdo not run on every PR. These regression tests are in an integration test file and may not run in CI on every PR, leaving the optimization without enforced coverage. Consider adding equivalent#[cfg(test)]unit tests insrc/or migrating these tests to a unit test module.As per coding guidelines: "Put acceptance coverage in unit tests visible to
cargo-test, because integration suites undercrates/*/tests/*.rsdo not run on every PR."🤖 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/tests/native_proof_regressions/integer_modulo.rs` around lines 1 - 145, Move the regression coverage from the integration test module into a #[cfg(test)] unit-test module under src, or add equivalent unit tests there, so cargo-test executes it on every PR. Preserve the existing helpers and assertions from factorial_shaped_ir, assert_integer_modulo_with_negative_zero_repair, and assert_floating_modulo, including coverage for eligible literals, unsafe or dynamic divisors, floating operands, and unsigned operands.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@crates/perry-codegen/tests/native_proof_regressions/integer_modulo.rs`:
- Around line 1-145: Move the regression coverage from the integration test
module into a #[cfg(test)] unit-test module under src, or add equivalent unit
tests there, so cargo-test executes it on every PR. Preserve the existing
helpers and assertions from factorial_shaped_ir,
assert_integer_modulo_with_negative_zero_repair, and assert_floating_modulo,
including coverage for eligible literals, unsafe or dynamic divisors, floating
operands, and unsigned operands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d8e79bba-bb56-45e9-ae1b-1406e4cfee1f
📒 Files selected for processing (5)
crates/perry-api-manifest/src/entries/part_1.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/tests/native_proof_regressions.rscrates/perry-codegen/tests/native_proof_regressions/integer_modulo.rstest-files/test_gap_numeric_remainder_i32.ts
|
Thanks for this — the captured/parameter asymmetry is a real win. One blocker before it can land, and it's a correctness one rather than a style nit. What's failing
The test asserts the emitted IR still contains Why that assertion existsThe shape it guards is So this isn't the fast path being too fast; it's the fast path being taken on a value that hasn't been proven numeric. Where to lookYour change makes the numeric specialization survive through a capture. The likely cause is that the "is this operand statically numeric?" predicate now answers yes for a value that only looks numeric because it came through the newly-specialized path — the specialization is being treated as a type proof, when it's a representation choice. A dynamically-materialized POD field read has to stay boxed until coerced, regardless of how the array is reached. Worth checking whether the same over-trust reaches other operand kinds — anywhere a boxed fallback can now skip its coerce. Note on the gate
Happy to look further if you want a second pair of eyes on the predicate. |
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/tests/native_proof_regressions.rs`:
- Line 1320: Move
pod_field_read_after_dynamic_materialization_uses_dynamic_numeric_sub and the
newly added integer_modulo coverage from crates/*/tests integration files into a
#[cfg(test)] unit-test module under the relevant src/ directory, preserving
their assertions and setup so cargo test discovers them on every PR.
🪄 Autofix (Beta)
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: 553e2788-c264-469f-995f-bb34dfe4741c
📒 Files selected for processing (1)
crates/perry-codegen/tests/native_proof_regressions.rs
|
|
||
| #[test] | ||
| fn pod_field_read_after_dynamic_materialization_uses_number_coerce() { | ||
| fn pod_field_read_after_dynamic_materialization_uses_dynamic_numeric_sub() { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move acceptance coverage to unit tests.
As per coding guidelines, put acceptance coverage in unit tests visible to cargo-test, because integration suites under crates/*/tests/*.rs do not run on every PR. Please move this regression test (and any other newly added coverage, such as the integer_modulo tests) into a #[cfg(test)] unit test module within the src/ directory to ensure they are reliably executed in CI.
🤖 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/tests/native_proof_regressions.rs` at line 1320, Move
pod_field_read_after_dynamic_materialization_uses_dynamic_numeric_sub and the
newly added integer_modulo coverage from crates/*/tests integration files into a
#[cfg(test)] unit-test module under the relevant src/ directory, preserving
their assertions and setup so cargo test discovers them on every PR.
Source: Coding guidelines
2 participants
Summary
Expr::Integerdivisor to the existing integer remainder loweringExpr::Number, unsafe or dynamic divisors, unsigned counters, and non-i32 operandsws.readyStatedispatch entry in the API manifestPerformance
Paired Perry before/after gate
Five runs per build on the same machine using fresh no-cache binaries for
benchmarks/suite/13_factorial.ts:frem doublesrem i64fmodumull/ushr/mls_fmodreference in the benchmark objectsum:49950000000×5sum:49950000000×5Interleaved runtime comparison
To reduce scheduler and thermal noise, each runtime was warmed up three times and then measured in 25 rotating, interleaved rounds on the same machine:
sum:49950000000×25sum:49950000000×25sum:49950000000×25This reproduces the public PR #6376 starting point, which reported Perry at 1,555 ms versus 99 ms for Node and Bun. With the integer path reachable, Perry's robust median is now approximately 2% faster than both local references; there is no remaining repeatable factorial gap to chase.
Validation
cargo fmt --all -- --checkcargo check -p perry-codegencargo test -p perry-codegen --test native_proof_regressions integer_modulo::— 3 passedcargo test -p perry-codegen --test manifest_consistency— 5 passed./run_parity_tests.sh --filter test_gap_numeric_remainder_i32— 1 passed./benchmarks/compare.sh --quick --runs 5 --json-out /tmp/perry-i32-modulo-quick.json --warn-only— all five correctness checks passedExisting unrelated failures
The full
cargo test -p perry-codegenrun still encounters existing failures innative_owned_uint8array_get_fallback_uses_uint8array_helperandpod_field_read_after_dynamic_materialization_uses_number_coerce; neither path is modified by this pull request.Summary by CodeRabbit
readyState, and updated the supported API reference.%code generation to route specific signed i32 counter-slot cases with strictly positive i32 literal divisors through the correct integer remainder path, preserving JavaScript remainder behavior (including-0) and avoiding an unwanted floating path.wsmethod listings.