perf(codegen): inline strict === against a string literal (interp 2.29s -> 1.89s) - #7767
Conversation
|
Warning Review limit reached
Next review available in: 45 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 (2)
📝 WalkthroughWalkthroughThe compiler now lowers strict string equality inline. It handles pooled pointers, SSO values, heap strings, boxed values, and runtime fallbacks. Tests cover IR dispatch, encoding helpers, JavaScript equality semantics, and varied string representations. ChangesStrict string equality optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ExpressionLowering
participant InlineStringEquality
participant RuntimeStringEquality
ExpressionLowering->>InlineStringEquality: Lower strict comparison with a string literal
InlineStringEquality->>InlineStringEquality: Check pointers, SSO values, tags, lengths, and endpoints
InlineStringEquality->>RuntimeStringEquality: Compare remaining heap or boxed values
RuntimeStringEquality-->>ExpressionLowering: Return equality result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/perry-codegen/src/expr/compare_tests.rs (1)
72-112: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a census for the two arms the changelog claims eliminate the call.
The current positives assert only
streqlit.tag. Two claims stay unpinned:
- a short literal emits the
streqlit.ssoblock, which is what makes"+" === "+"work across acharAtresult and the pooled literal;- a literal of 2 bytes or fewer emits no
js_string_equalscall, which is the "no call at all" claim.Both are single
containsassertions on IR you already capture.🧪 Proposed additional tests
/// A literal short enough for an inline immediate must emit the SSO arm, or a /// `charAt` result never matches the pooled literal. #[test] fn a_short_literal_emits_the_sso_immediate_arm() { let ir = cmp_ir( "streq_sso", CompareOp::Eq, Expr::LocalGet(X), Expr::String("+".to_string()), ); assert!(ir.contains("streqlit.sso"), "{ir}"); } /// For <= 2 bytes the endpoint checks settle the answer, so no content call /// may survive. #[test] fn a_two_byte_literal_needs_no_content_call() { let ir = cmp_ir( "streq_two", CompareOp::Eq, Expr::LocalGet(X), Expr::String("if".to_string()), ); assert!(ir.contains("streqlit.tag"), "{ir}"); assert!( !ir.contains("call i32 `@js_string_equals`("), "a <= 2-byte literal still reaches the content compare:\n{ir}" ); }🤖 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/compare_tests.rs` around lines 72 - 112, Add tests alongside the existing comparison tests: update the short-literal coverage around cmp_ir to assert that comparing against "+" emits the "streqlit.sso" arm, and add a two-byte literal case such as "if" that asserts "streqlit.tag" is present and no "call i32 `@js_string_equals`(" appears. Keep these as IR contains assertions and preserve the existing Eq/Ne coverage.
🤖 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/src/expr/compare_tests.rs`:
- Around line 72-112: Add tests alongside the existing comparison tests: update
the short-literal coverage around cmp_ir to assert that comparing against "+"
emits the "streqlit.sso" arm, and add a two-byte literal case such as "if" that
asserts "streqlit.tag" is present and no "call i32 `@js_string_equals`(" appears.
Keep these as IR contains assertions and preserve the existing Eq/Ne coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 18d80687-439f-4569-82ac-d519da99b619
📒 Files selected for processing (5)
changelog.d/7767-inline-strict-string-equality.mdcrates/perry-codegen/src/expr/compare.rscrates/perry-codegen/src/expr/compare_tests.rscrates/perry-codegen/src/expr/mod.rstest-files/test_strict_eq_string_literal_inline.ts
`n.kind === "num"` compiled to a `js_eq` -> `js_jsvalue_equals` call pair; that pair plus its memcmp was ~21% of gc-handoff/apps/interp.ts. When one operand is a string literal both of a string's runtime representations are known at compile time — the pooled StringHeader pointer and, for <= 5 bytes, the canonical SSO immediate — so identity settles the true case in one icmp, a non-STRING_TAG operand is decided false by tag alone, and a heap string is filtered by byte_len plus its first and last byte before any call. The no-literal string arms gain the two shortcuts that need no compile-time facts (identical bits, SSO x SSO), keeping their existing fallbacks. Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
…ands Initializing an `any` local with a string literal refines its static type to `string`, which routes the comparison to the both-strings arm and made both negatives vacuous — they asserted the absence of a dispatch that was never going to fire and the presence of a helper the refined path never calls. Claude-Session: https://claude.ai/code/session_015JgLM9UWGa6WAMix7CvhQJ
946f9dd to
da3fd4a
Compare
Merging as v0.5.1449Verified independentlySemantics: The IR census bites. Neutering the admission ( The dispatch table is the designEnumerating every runtime representation a string can have and resolving each against compile-time knowledge is what makes this exact rather than heuristic: pooled-pointer identity in one The pooled-identity argument is the subtle half and it is stated correctly: Restraint on the no-literal arms is also right: MeasurementsThe equality family falling ~21% → ~2% on Gates 21/21. |
The finding
n.kind === "num"— the shape every tree-walking interpreter, reducer anddiscriminated-union dispatch is built out of — compiled to a
js_eq→js_jsvalue_equalscall pair. Ongc-handoff/apps/interp.tsthat pair plus thememcmpunder it was ~21% of the program's runtime, against 30% for the usercode itself.
interp.tsis the only program in the perf corpus that resemblesreal software, and this was its largest single remaining cost.
The mechanism
The call was never necessary. When one operand is a string literal, both of a
string's runtime representations are known at compile time:
icmp eqon the NaN-boxed bits — one instructioncharAt,JSON.parse)icmp eqagainst a compile-time immediateSTRING_TAG⇒ false, no memory touchedbyte_lenvs a compile-time constant ⇒ falsei8loads inside bytes the length check proved the header owns ⇒ falsejs_string_equalsFor literals of ≤ 2 bytes the sequence is total — no call exists on any path.
{ kind: "num" }stores the same pooled pointer the comparison site loads, andGC evacuation rewrites the pool root and the object slot together, so identity
survives collection. That is what makes the true case one
icmp.The two string-equality arms with no literal operand (
names[i] === name)gained only the shortcuts that need no compile-time facts: identical bits, and
SSO × SSO with differing bits. Their existing fallbacks are untouched — which
matters for the legacy arm, since it keeps
js_get_string_pointer_unified'snumber-coercing behaviour for operands whose
stringannotation lies. Thatcomposition materializes an SSO operand onto the heap, so routing SSO × SSO
around it also removes two throwaway allocations per comparison.
Semantics
Exact
===, and in one respect stricter than what it replaces: the oldboth_stringsarm reached equality throughjs_get_string_pointer_unified,which coerces a number to its decimal string, so
(5 as any) === "5"could comeback true. Pinned against Node in
test-files/test_strict_eq_string_literal_inline.ts:NaN !== NaN,+0 === -0,distinct heap strings with equal content are equal, int32 and double
representations of the same Number are equal,
new String("num") !== "num",null vs undefined, object identity, and the multi-byte-UTF-8 cases where "first
byte" is not "first character" (
"é"vs"è","日本"vs"月本").Measurements — absolute seconds, quiet M1 mini, best-of-5
Outputs verified byte-identical to
node --experimental-strip-typesbeforetiming, on every row.
misses 0✓Profile share of the equality family on
interp_big.ts(symbolicated, two runs):js_jsvalue_equals+js_eq+memcmpevalNode(user code) is now 35% of the profile.Tests
test-files/test_strict_eq_string_literal_inline.ts— the ECMAScript edgesabove, diffed against Node.
crates/perry-codegen/src/expr/compare_tests.rs— an IR census in the per-PRcargo-testgate: thestreqlit.*blocks present andjs_eqabsent, so afast path that is implemented but never reached fails here rather than
silently. Paired negatives: loose
==must keep its coercing helper, and acomparison with no literal operand must not use the dispatch. Plus numeric pins
on the hand-built SSO immediate, which
perry-codegencannot share withperry-runtime— if that encoding drifts,"+" === "+"across acharAtresult and a literal silently becomes false.
Measured and rejected
Marking
js_string_equalsnounwind willreturn readonly(the #6082 allowlist)so a dispatch chain could keep its discriminant in a register: measured null
(1.89 vs 1.89 interleaved, six pairs). Once the fast arms contain no calls at
all, LLVM already keeps the load in a register along the hot path, and the
remaining
js_string_equalssites are cold. Dropped rather than shipped asunmeasured surface on a soundness-sensitive allowlist.
Summary by CodeRabbit
Performance
Bug Fixes
===semantics across strings, boxed values, symbols, numbers, objects, and other types.Tests