perf(codegen): inline non-BigInt bitwise ops instead of js_dynamic_bit* calls - #6860
Merged
Conversation
added 2 commits
July 26, 2026 21:36
…t* calls bcryptjs _encipher's Feistel `^` lowered to a js_dynamic_bitxor runtime call per round: the accumulators l/r are Any-typed (the `let l = lr[off]` inference does not propagate the Int32Array element type), so the bitwise-op lowering failed the is_numeric_expr check and bailed to the BigInt-aware dynamic helper before reaching the inline ToInt32+xor path. Post-opt -O3, _encipher held 12 js_dynamic_* bitwise calls (4 bitxor, 4 shr, 2 ushr, 2 bitand). For the six bitwise ops, when BOTH operands are provably not a BigInt, lower inline (ToInt32 <op> ToInt32 + sitofp) reusing the existing all-numeric path: - New predicate type_analysis::is_provably_not_bigint (Binary/Unary handled structurally, so anyA ^ anyB — possibly bigint^bigint — is not misclassified). - New flow set collectors::not_bigint_locals (greatest fixpoint): an Any-typed local is non-BigInt when every assignment to it is a non-BigInt expression. Wired onto FnCtx as RepresentationFacts::not_bigint_locals. The BigInt bail is preserved: a mixed bigint/number op still throws TypeError and bigint^bigint still computes a BigInt (both operands must pass the gate). The inline path keeps the NaN-safe guarded toint32_wrap for operands not proven finite (an out-of-bounds typed-array read is undefined/NaN, still non-BigInt). Gated behind PERRY_INLINE_NONBIGINT_BITWISE (default on; =0 reverts); keyed into the object cache. Optimized _encipher: 12 js_dynamic_* bitwise calls -> 0. enc.ts min-of-5 3651ms -> 1845ms (Node 26.5.0 ~135ms; the un-hoisted per-read guard is a separate follow-up). Byte-exact vs Node, flag on and off.
📝 WalkthroughWalkthroughAdds flow analysis for locals that cannot hold ChangesNon-BigInt Bitwise Lowering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HIR
participant TypeFacts
participant FnCtx
participant BinaryLowering
participant RuntimeHelper
HIR->>TypeFacts: collect non-BigInt local facts
TypeFacts->>FnCtx: provide not_bigint_locals
FnCtx->>BinaryLowering: analyze bitwise operands
BinaryLowering->>BinaryLowering: emit inline ToInt32/ToUint32 lowering
BinaryLowering->>RuntimeHelper: use dynamic BigInt-aware helper when proof is unavailable
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
🧹 Nitpick comments (1)
crates/perry-codegen/src/collectors/not_bigint_locals.rs (1)
35-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider dedicated unit tests for this fixed-point algorithm.
The greatest-fixpoint write analysis here (and its self-referential short-circuit in
expr_not_bigint'sBinaryarm) is subtle: e.g.l = l ^ xis judged safe by short-circuiting on the self-reference, which is only sound because a mixed BigInt/Number bitwise op throws rather than silently coercing (solnever actually receives a bad value when the op would produce one). This reasoning is correct as written, but a future edit toexpr_not_bigint(or to JS bitwise-op throw semantics assumptions) could silently break it without any test catching the regression. A few targeted#[cfg(test)]cases directly oncollect_not_bigint_locals(self-referential accumulator, mutual-reference locals,let x;with no init, BigInt-literal write invalidating the local) would materially de-risk future changes to this soundness-critical pass.Also applies to: 92-166
🤖 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/collectors/not_bigint_locals.rs` around lines 35 - 86, Add targeted #[cfg(test)] unit tests for collect_not_bigint_locals covering a self-referential accumulator, mutually referencing locals, an uninitialized let x;, and a BigInt-literal write. Assert each case’s expected membership in the returned HashSet, keeping the tests focused on the greatest-fixpoint behavior and expr_not_bigint Binary self-reference handling.
🤖 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/type_analysis/numeric.rs`:
- Around line 335-375: Update is_provably_not_bigint to handle Expr::Update
before the is_numeric_expr shortcut, using the update target’s
flow-analysis/type information so increments and decrements of BigInt locals are
not classified as definitively non-BigInt. Preserve the existing behavior for
updates proven non-BigInt, and add coverage in
test_gap_inline_nonbigint_bitwise.ts for increment/decrement on a BigInt local
in a mixed bitwise expression.
---
Nitpick comments:
In `@crates/perry-codegen/src/collectors/not_bigint_locals.rs`:
- Around line 35-86: Add targeted #[cfg(test)] unit tests for
collect_not_bigint_locals covering a self-referential accumulator, mutually
referencing locals, an uninitialized let x;, and a BigInt-literal write. Assert
each case’s expected membership in the returned HashSet, keeping the tests
focused on the greatest-fixpoint behavior and expr_not_bigint Binary
self-reference handling.
🪄 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: df527325-431e-4146-acbb-faed000f7801
📒 Files selected for processing (15)
changelog.d/6860-inline-nonbigint-bitwise.mdcrates/perry-codegen/src/codegen/closure.rscrates/perry-codegen/src/codegen/entry.rscrates/perry-codegen/src/codegen/function.rscrates/perry-codegen/src/codegen/method.rscrates/perry-codegen/src/collectors/hir_facts.rscrates/perry-codegen/src/collectors/mod.rscrates/perry-codegen/src/collectors/not_bigint_locals.rscrates/perry-codegen/src/expr/binary.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/type_analysis.rscrates/perry-codegen/src/type_analysis/numeric.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/object_cache/object_cache_tests.rstest-files/test_gap_inline_nonbigint_bitwise.ts
…bigint++) CodeRabbit review on #6860. `(bigintLocal++) & 5` computed garbage instead of throwing the spec TypeError (mixed bigint/number), and `(bigintLocal++) * 2` silently produced NaN. Two-part fix: 1. is_numeric_expr(Expr::Update) was unconditionally `true`. A pre/post inc/dec is `ToNumeric(x) ± 1` — a Number, EXCEPT when x is a BigInt (stays BigInt). So it is NOT unconditionally numeric: mirror the LocalGet arm (Number/Int32 declared type) and additionally honor the integer-range collectors (integer_locals / unsigned_i32_locals) so an Any-typed loop counter proven to hold an i32 keeps the numeric fast path. An Any local not proven integer (could be BigInt) is excluded — so a bigint-local Update operand no longer takes the all-numeric operand path (which toint32's the BigInt to 0). 2. is_provably_not_bigint gained an explicit Expr::Update { id, .. } arm before the is_numeric_expr shortcut, mirroring LocalGet(id) (not_bigint_locals / integer_locals / unsigned_i32_locals). With (1) making l_prim false for a bigint Update, the bitwise lowering now reaches this gate, which returns false, bailing to the BigInt-aware dynamic helper → throws / computes bigint. Gap-test cases (byte-exact vs Node, flag on/off, one under PERRY_GC_FORCE_EVACUATE=1): (b++)&num / (b--)|num / (b++)>>1 on a BigInt local throw TypeError; (b++)^bigint computes a BigInt (9n). Unit tests on collect_not_bigint_locals: self-referential accumulator, mutually-referencing locals, uninitialized let, BigInt-literal write invalidation, and taint propagation through a copy. Lever unchanged: optimized _encipher still has 0 js_dynamic_* bitwise calls; enc.ts still byte-exact (lr0=2135713266 lr1=-1949122846). integer-counter arithmetic keeps the numeric fast path via integer_locals, so this is output-preserving for numbers (a number-holding Any Update just gains a semantics-preserving coerce).
Contributor
Author
|
Thanks for the review — both points addressed, and the correctness bug turned out to need one more piece than the diagnosis suggested. 1.
Result (byte-exact vs Node, flag on/off, one under This is output-preserving for numbers — integer counters keep the numeric fast path via 2. Unit tests for the fixpoint. Added Re-verified: optimized (The |
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/type_analysis/numeric.rs`:
- Around line 392-395: Update the Expr::Update branch in the numeric proof to
also recognize locals whose declared types are Number or Int32, mirroring the
declared-type check used by is_numeric_expr around lines 129-132. Preserve the
existing not_bigint_locals, integer_locals, and unsigned_i32_locals checks while
ensuring declared numeric locals qualify for the inline bitwise path.
🪄 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: 18d1dfb1-a121-49fa-b6b6-6509a5caa343
📒 Files selected for processing (3)
crates/perry-codegen/src/collectors/not_bigint_locals.rscrates/perry-codegen/src/type_analysis/numeric.rstest-files/test_gap_inline_nonbigint_bitwise.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test-files/test_gap_inline_nonbigint_bitwise.ts
- crates/perry-codegen/src/collectors/not_bigint_locals.rs
Comment on lines
+392
to
+395
| Expr::Update { id, .. } => { | ||
| return ctx.not_bigint_locals.contains(id) | ||
| || ctx.integer_locals.contains(id) | ||
| || ctx.unsigned_i32_locals.contains(id); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
Include declared numeric types in the update proof.
This early return bypasses is_numeric_expr, so updates to Number/Int32 locals not present in the integer sets conservatively miss the inline bitwise path (for example, numberLocal++ ^ "1"). Mirror the declared-type check from Lines 129-132 here.
Proposed fix
Expr::Update { id, .. } => {
- return ctx.not_bigint_locals.contains(id)
+ return matches!(
+ ctx.local_types.get(id),
+ Some(HirType::Number) | Some(HirType::Int32)
+ ) || ctx.not_bigint_locals.contains(id)
|| ctx.integer_locals.contains(id)
|| ctx.unsigned_i32_locals.contains(id);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| Expr::Update { id, .. } => { | |
| return ctx.not_bigint_locals.contains(id) | |
| || ctx.integer_locals.contains(id) | |
| || ctx.unsigned_i32_locals.contains(id); | |
| Expr::Update { id, .. } => { | |
| return matches!( | |
| ctx.local_types.get(id), | |
| Some(HirType::Number) | Some(HirType::Int32) | |
| ) || ctx.not_bigint_locals.contains(id) | |
| || ctx.integer_locals.contains(id) | |
| || ctx.unsigned_i32_locals.contains(id); |
🤖 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/type_analysis/numeric.rs` around lines 392 - 395,
Update the Expr::Update branch in the numeric proof to also recognize locals
whose declared types are Number or Int32, mirroring the declared-type check used
by is_numeric_expr around lines 129-132. Preserve the existing
not_bigint_locals, integer_locals, and unsigned_i32_locals checks while ensuring
declared numeric locals qualify for the inline bitwise path.
proggeramlug
pushed a commit
that referenced
this pull request
Jul 27, 2026
…context A typed-array element read through a parameter (`function f(S: Int32Array)`) that feeds arithmetic rather than `| 0` — the bcryptjs `_encipher` S-box shape `n = S[l >>> 24]; n += S[...]` — previously compiled to a per-read `call js_typed_array_get(ptr, idx)`. The i32 fast path only fires in a ToInt32 (`| 0`) context, so these numeric-context reads stayed on the runtime call. Lower them to an inline checked f64 load instead: the same guard as the checked i32 read (pointer + inline-storage PERRY_TA_VIEW_GUARD==0 + kind-cache), a header bounds check, a bare per-kind load widened to f64 (sitofp/uitofp/fpext/direct), and — bit-exact with js_typed_array_get — the TAG_UNDEFINED double on OOB / negative. Guard misses defer to a new memory-safe cold helper js_typed_array_read_f64 (validates the pointer is a registered typed array before any header deref, mirroring js_typed_array_read_int32). Gated on a proven non-negative integer index (a fractional `S[3.9]` must read undefined, not element 3). Covers every numeric kind incl. Uint32 (unsigned widening) and the float kinds. Env flag PERRY_TA_PARAM_F64_READ (default on), keyed into the object cache. Measured on the real _encipher shape (2.1M calls, quiet M1, min-of-9, byte-exact): 1787ms -> 1351ms, ~1.32x; the per-read guard costs only ~3% over a bare load, so no guard hoisting is needed. Stacks with #6860 (inline non-BigInt bitwise): the dynamic-bitwise baseline was ~3541ms.
proggeramlug
pushed a commit
that referenced
this pull request
Jul 27, 2026
…context A typed-array element read through a parameter (`function f(S: Int32Array)`) that feeds arithmetic rather than `| 0` — the bcryptjs `_encipher` S-box shape `n = S[l >>> 24]; n += S[...]` — previously compiled to a per-read `call js_typed_array_get(ptr, idx)`. The i32 fast path only fires in a ToInt32 (`| 0`) context, so these numeric-context reads stayed on the runtime call. Lower them to an inline checked f64 load instead: the same guard as the checked i32 read (pointer + inline-storage PERRY_TA_VIEW_GUARD==0 + kind-cache), a header bounds check, a bare per-kind load widened to f64 (sitofp/uitofp/fpext/direct), and — bit-exact with js_typed_array_get — the TAG_UNDEFINED double on OOB / negative. Guard misses defer to a new memory-safe cold helper js_typed_array_read_f64 (validates the pointer is a registered typed array before any header deref, mirroring js_typed_array_read_int32). Gated on a proven non-negative integer index (a fractional `S[3.9]` must read undefined, not element 3). Covers every numeric kind incl. Uint32 (unsigned widening) and the float kinds. Env flag PERRY_TA_PARAM_F64_READ (default on), keyed into the object cache. Measured on the real _encipher shape (2.1M calls, quiet M1, min-of-9, byte-exact): 1787ms -> 1351ms, ~1.32x; the per-read guard costs only ~3% over a bare load, so no guard hoisting is needed. Stacks with #6860 (inline non-BigInt bitwise): the dynamic-bitwise baseline was ~3541ms.
proggeramlug
added a commit
that referenced
this pull request
Jul 27, 2026
…context (#6883) * perf(codegen): inline checked-f64 typed-array-param reads in numeric context A typed-array element read through a parameter (`function f(S: Int32Array)`) that feeds arithmetic rather than `| 0` — the bcryptjs `_encipher` S-box shape `n = S[l >>> 24]; n += S[...]` — previously compiled to a per-read `call js_typed_array_get(ptr, idx)`. The i32 fast path only fires in a ToInt32 (`| 0`) context, so these numeric-context reads stayed on the runtime call. Lower them to an inline checked f64 load instead: the same guard as the checked i32 read (pointer + inline-storage PERRY_TA_VIEW_GUARD==0 + kind-cache), a header bounds check, a bare per-kind load widened to f64 (sitofp/uitofp/fpext/direct), and — bit-exact with js_typed_array_get — the TAG_UNDEFINED double on OOB / negative. Guard misses defer to a new memory-safe cold helper js_typed_array_read_f64 (validates the pointer is a registered typed array before any header deref, mirroring js_typed_array_read_int32). Gated on a proven non-negative integer index (a fractional `S[3.9]` must read undefined, not element 3). Covers every numeric kind incl. Uint32 (unsigned widening) and the float kinds. Env flag PERRY_TA_PARAM_F64_READ (default on), keyed into the object cache. Measured on the real _encipher shape (2.1M calls, quiet M1, min-of-9, byte-exact): 1787ms -> 1351ms, ~1.32x; the per-read guard costs only ~3% over a bare load, so no guard hoisting is needed. Stacks with #6860 (inline non-BigInt bitwise): the dynamic-bitwise baseline was ~3541ms. * docs(changelog): add fragment for #6883 --------- Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Jul 28, 2026
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
1 participant
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.
Summary
Kill the f64-bitwise →
js_dynamic_bit*runtime-call cascade in hot integerkernels. bcryptjs's
_encipherFeistel network runsr ^= n ^ P[i]per round,but the accumulators
l/rareAny-typed (thelet l = lr[off]inferencedoes not propagate the
Int32Arrayelement type), so every^/&/>>/>>>on them failed the
is_numeric_exprcheck and bailed to the BigInt-awarejs_dynamic_bit*runtime helper — one opaque call per round. Post-opt -O3,_enciphercontained 12@js_dynamic_*bitwise calls (4bitxor, 4shr,2
ushr, 2bitand).This lowers the six bitwise ops (
& | ^ << >> >>>) inline(
ToInt32 <op> ToInt32 + sitofp) whenever both operands are provably not aBigInt, reusing the existing all-numeric inline machinery — even when the
operands are not statically
is_numeric_expr.How
type_analysis::is_provably_not_bigint(ctx, e)— true for avalue whose
ToNumericcan never yield a BigInt: numerics, booleans, strings,null/undefined/typeof/void/comparisons, numeric typed-array elementreads, arithmetic/bitwise results (a BigInt result needs both operands
BigInt), and locals proven non-BigInt.
collectors::not_bigint_locals(greatest fixpoint): anAny-typed local is non-BigInt when every assignment to it (init + everyLocalSet, including inside closure bodies) is a non-BigInt expression. Thisis what proves bcryptjs's
l/r(init from anInt32Arrayelement, thenonly ever
^=/+=-updated) are never a BigInt. Wired ontoFnCtxas aper-region fact (
RepresentationFacts::not_bigint_locals).expr/binary.rs: before the BigInt bail, for the six bitwise ops only, ifboth operands
is_provably_not_bigint, fall through to the inline path.PERRY_INLINE_NONBIGINT_BITWISE(default on;=0/off/falsereverts to the runtime call). Keyed into the object cache like the sibling
codegen env flags.
Correctness
routes to
js_dynamic_bit*, so a mixedbigint <op> numberstill throwsTypeErrorandbigint <op> bigintstill computes a BigInt. Theis_provably_not_bigintgate requires both operands to pass, and theexisting
is_bigint_exprguard already caught statically-BigInt operandsupstream.
toint32_wrapfor any operandnot proven finite — an out-of-bounds typed-array read is
undefined/NaN(still non-BigInt), and
toint32_wrapmaps NaN/±Inf to0per spec.toint32_fastis used only whereis_known_finitealready proves finiteness.not_bigint_localsis a greatest fixpoint that only everremoves locals on a possibly-BigInt write; it may under-approximate but
never over-approximates. Declared types are trusted exactly as
is_numeric_expralready trusts them.Results (reproduction: a faithful
_encipher, 2.1M calls)@js_dynamic_bitxorin optimized_encipher: 4 → 0 (and all 12js_dynamic_*bitwise calls → 0; inlinexor/and/lshr i32now present).~13.7x Node — the per-read typed-array guard is still un-hoisted (a separate
follow-up); the win here is purely from removing the per-round runtime call.
lr0=2135713266 lr1=-1949122846), flag on and off.Tests
test-files/test_gap_inline_nonbigint_bitwise.ts(byte-exact vs Node):typed-array-read operands, untyped-
anyelement reads, an out-of-boundsundefinedoperand, and BigInt operands (bigint^bigintcomputes a BigInt;bigint <op> numberthrowsTypeError). Verified byte-identical flag on,flag off, and under
PERRY_GC_FORCE_EVACUATE=1.object_cache_tests::key_changes_with_codegen_env_varsextended to coverPERRY_INLINE_NONBIGINT_BITWISE.Summary by CodeRabbit
New Features
& | ^ << >> >>>when both operands are provably non-BigInt, improving performance in integer-heavy workloads.PERRY_INLINE_NONBIGINT_BITWISEto enable/disable the optimization (disabled when set to0).Bug Fixes
TypeErrorfor mixed BigInt/number bitwise operations.Tests