perf(codegen): representation-selection Phase 3a — canonical string locals (tagged-at-rest Str rep) - #6909
Merged
Merged
Conversation
added 2 commits
July 27, 2026 22:40
…ocals (tagged-at-rest Str rep) Phase 3a of docs/representation-selection-rfc.md (§4 Str row, §5.5, §5.6): SlotRep::Str marks function locals proven to hold NaN-box string bits (STRING_TAG heap or SHORT_STRING_TAG SSO) at rest. Storage, shadow-slot GC binding, and every alias/refcount demote stay exactly the pre-phase model (tagged-at-rest needs zero GC changes); the rep is a compile-time proof the string-op lowerings consume to tag-dispatch inline instead of routing every operand through the opaque js_get_string_pointer_unified (which heap- materializes SSO and number-coerces): - 's += rhs' self-append: 3-arm dispatch — both-heap -> raw js_string_append(h,h) (keeps the refcount==1 in-place path), both-string SSO-involved -> js_string_concat_box, else -> the exact legacy sequence. - '.length' on statically-string receivers: SSO inline len-byte extract / heap bare load i32 of utf16_len / js_value_length_f64 cold arm, replacing the ~18-op GC-type-byte tower. - '==='/'<' with a canonical-Str operand: both-heap -> direct js_string_equals/js_string_compare on raw handles; else one SSO-aware call (js_jsvalue_equals / new js_string_compare_value) — never number-coercing. - charCodeAt/at/codePointAt: proven-heap receiver -> bare and-mask handle. - string+value coerce-concat: literal operands unbox inline (bitcast+and). - StringRef materialization: inline or-STRING_TAG+bitcast, null cold arm. Gated by PERRY_CANONICAL_STR_LOCALS (default on), keyed into the object cache. Eligibility mirrors Phase 1's exclusions (closure-ref'd, boxed, module globals, async/generator bodies) plus a ctx-free pre-pass excluding non-string-proven writes, equality compares against non-proven-string operands (the other_side_is_any hazard), and catch bindings.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis change adds Phase 3a canonical string locals through ChangesCanonical string locals
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant FunctionCompiler
participant FnCtx
participant StringLowering
participant Runtime
FunctionCompiler->>FnCtx: configure canonical-Str eligibility
FnCtx->>StringLowering: expose SlotRep::Str local
StringLowering->>Runtime: dispatch heap, SSO, or boxed operation
Runtime-->>StringLowering: return string result or comparison
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 |
…ched compare ops, unused ctx param)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
crates/perry-codegen/src/expr/mod.rs (1)
778-783: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc sentence is truncated.
"so the two phases A/B independently" is missing a verb — presumably "can be A/B tested independently."
🤖 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/mod.rs` around lines 778 - 783, Update the documentation for the `repsel_context_allows_canonical_str` field to complete the truncated sentence with the intended meaning that the two phases can be A/B tested independently. Do not change the field or its behavior.crates/perry-codegen/src/stmt/let_stmt.rs (1)
1296-1296: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
note_canonical_i32_localis no longer i32-specific.It already takes the
repand printscanonical-{rep:?}, so a rename tonote_canonical_local(3 call sites: here, the canonical-i32 branch, and the definition inexpr/slot_rep.rs) would stop the name from contradicting theSlotRep::Strargument.🤖 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/stmt/let_stmt.rs` at line 1296, Rename note_canonical_i32_local to note_canonical_local at its definition in expr/slot_rep.rs and all three call sites, including this SlotRep::Str usage and the canonical-i32 branch. Preserve the existing rep argument and canonical-{rep:?} behavior.crates/perry-codegen/src/expr/slot_rep.rs (1)
358-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
syntactic_str's call arm keys on method name only, so array/object methods misclassify as string-producing.
arr.slice(),arr.concat(x), and a user object's.replace()all match this arm regardless of receiver, so aLocalSetof such a value onto astring-declared local won't mark the local ineligible. Every specialized lowering re-checks the runtime tag (0x7FFF/0x7FF9) and falls back, so this is a deopt rather than a correctness break — but the doc comment above claims the predicate "under-approximates," which isn't true for these names. Consider requiring a string-ish receiver (or at leastsyntactic_str(receiver)) for the name-overlapping subset.🤖 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/slot_rep.rs` around lines 358 - 368, Update the call arm of syntactic_str so method-name matches also require a string-ish receiver, such as syntactic_str(receiver), especially for names shared with array or object methods like slice, concat, and replace. Preserve recognition of genuine string-producing calls while ensuring arr.slice(), arr.concat(), and user-object methods are not classified as strings; keep the predicate’s documented under-approximation behavior accurate.crates/perry-codegen/src/lower_string_method.rs (1)
1374-1376: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDerive the shifted string-tag comparands from
crate::nanbox.
crates/perry-codegen/src/lower_string_method.rscurrently repeats32767/32761forSTRING_TAG >> 48andSHORT_STRING_TAG >> 48. Keeping the arithmetic derived close tocrate::nanbox::STRING_TAG/crate::nanbox::SHORT_STRING_TAGavoids silently drifting if the layout constants change.🤖 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/lower_string_method.rs` around lines 1374 - 1376, Update the tag comparisons in the string-method lowering logic around the bitcast/lshr sequence to derive the shifted comparands from crate::nanbox::STRING_TAG and crate::nanbox::SHORT_STRING_TAG rather than hardcoded 32767/32761 values. Preserve the existing heap-versus-short-string detection behavior while ensuring both comparisons track the nanbox constants if their layout changes.crates/perry-codegen/src/expr/compare.rs (1)
296-371: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated heap-tag dispatch prologue between the two new arms.
The bits/tag/
both_heapcomputation plus the 3-block diamond is byte-identical in the equality and relational arms; only the callee pair and the result-folding differ. A small helper (emit_both_heap_dispatch(ctx, &l, &r, "streq"|"strcmp", heap_call, boxed_call) -> String) would keep the two lowerings from drifting apart as more ops join Phase 3a.Also applies to: 412-483
🤖 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.rs` around lines 296 - 371, Extract the duplicated bits/tag computation and heap-versus-boxed three-block dispatch from the equality and relational lowering arms into a shared helper, such as emit_both_heap_dispatch, near the relevant comparison lowering code. Have the helper accept the lowered operands, block-name prefix, and heap/boxed callees, returning the merged comparison result; update both arms to reuse it while preserving their distinct result-folding behavior.
🤖 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 `@changelog.d/6909-repsel-p3a-canonical-str-locals.md`:
- Around line 12-14: Correct the changelog description of the `s += rhs`
self-append case so `js_string_append` is shown receiving the LHS and RHS
handles, not the same handle twice. Preserve the existing distinctions for
both-heap, both-string-SSO, and legacy fallback behavior.
In `@crates/perry-runtime/src/string/compare.rs`:
- Around line 104-141: Restructure the comparison flow around bytes_of so all
number-to-string coercions, including js_number_to_string allocations, complete
before obtaining any heap-string pointers. Separate the allocating number
conversion from the non-allocating string/SSO view creation, retain each
converted number in its Option<Vec<u8>>, then create views and perform
from_raw_parts only after both operands are stable.
---
Nitpick comments:
In `@crates/perry-codegen/src/expr/compare.rs`:
- Around line 296-371: Extract the duplicated bits/tag computation and
heap-versus-boxed three-block dispatch from the equality and relational lowering
arms into a shared helper, such as emit_both_heap_dispatch, near the relevant
comparison lowering code. Have the helper accept the lowered operands,
block-name prefix, and heap/boxed callees, returning the merged comparison
result; update both arms to reuse it while preserving their distinct
result-folding behavior.
In `@crates/perry-codegen/src/expr/mod.rs`:
- Around line 778-783: Update the documentation for the
`repsel_context_allows_canonical_str` field to complete the truncated sentence
with the intended meaning that the two phases can be A/B tested independently.
Do not change the field or its behavior.
In `@crates/perry-codegen/src/expr/slot_rep.rs`:
- Around line 358-368: Update the call arm of syntactic_str so method-name
matches also require a string-ish receiver, such as syntactic_str(receiver),
especially for names shared with array or object methods like slice, concat, and
replace. Preserve recognition of genuine string-producing calls while ensuring
arr.slice(), arr.concat(), and user-object methods are not classified as
strings; keep the predicate’s documented under-approximation behavior accurate.
In `@crates/perry-codegen/src/lower_string_method.rs`:
- Around line 1374-1376: Update the tag comparisons in the string-method
lowering logic around the bitcast/lshr sequence to derive the shifted comparands
from crate::nanbox::STRING_TAG and crate::nanbox::SHORT_STRING_TAG rather than
hardcoded 32767/32761 values. Preserve the existing heap-versus-short-string
detection behavior while ensuring both comparisons track the nanbox constants if
their layout changes.
In `@crates/perry-codegen/src/stmt/let_stmt.rs`:
- Line 1296: Rename note_canonical_i32_local to note_canonical_local at its
definition in expr/slot_rep.rs and all three call sites, including this
SlotRep::Str usage and the canonical-i32 branch. Preserve the existing rep
argument and canonical-{rep:?} behavior.
🪄 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: 28f617a0-7f65-4299-904c-ed012b35d9ed
📒 Files selected for processing (20)
changelog.d/6909-repsel-p3a-canonical-str-locals.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/expr/compare.rscrates/perry-codegen/src/expr/mod.rscrates/perry-codegen/src/expr/property_get.rscrates/perry-codegen/src/expr/slot_rep.rscrates/perry-codegen/src/lower_call/capture_writeback.rscrates/perry-codegen/src/lower_string_method.rscrates/perry-codegen/src/native_value/materialize.rscrates/perry-codegen/src/runtime_decls/strings.rscrates/perry-codegen/src/stmt/let_stmt.rscrates/perry-codegen/tests/shadow_slot_hygiene.rscrates/perry-runtime/src/string/compare.rscrates/perry-runtime/src/string/tests.rscrates/perry/src/commands/compile/object_cache.rscrates/perry/src/commands/compile/object_cache/object_cache_tests.rstest-files/test_gap_repsel_canonical_str_locals.ts
added 3 commits
July 27, 2026 22:55
… SSO concat arm on both-string tags
Review pass caught two arm-layout hazards in the canonical self-append:
(1) a heap destination with an SSO rhs routed to js_string_concat_box,
which copies the whole accumulator per iteration — O(n^2) for loops
appending computed short parts; dest-heap now ALWAYS reaches
js_string_append (rhs materialized via the legacy unified helper when not
proven heap). (2) the SSO arm fired on a lying rhs, and concat_box treats
non-strings as empty where the legacy path ToString-coerces ('ab' += 42
must stay 'ab42'); the arm now requires string tags on BOTH sides. Gap
test extended with runtime-SSO destinations (JSON-parsed) for both cases.
…-string views in js_string_compare_value; review cleanups CodeRabbit review of #6909: - GC hazard (critical): the old single-pass bytes_of could hold a raw pointer into operand A's heap StringHeader payload while operand B's number coercion called js_number_to_string — an allocation that can run a GC cycle and MOVE A's string under evacuation, leaving the from_raw_parts read dangling. Restructured into two phases: ALL allocating coercions complete first (decimal bytes retained in owned Vecs), then only non-allocating views (heap payload / SSO scratch / owned buffers) are taken. Unit test extended with number-vs-heap and number-vs-SSO mixes in both operand orders; verified end-to-end with a 20k-iteration number-vs-fresh-heap-string compare probe under PERRY_GC_FORCE_EVACUATE=1 (stable, identical to the flag-off arm). - compare.rs: extracted the duplicated both-heap dispatch prologue into canonical_str_cmp_dispatch (shared by the Eq and relational arms). - Derived the lshr-48 tag comparands from crate::nanbox (STRING_TAG_TOP16_I64 / SHORT_STRING_TAG_TOP16_I64, assert-covered) instead of magic strings, across compare/property_get/lower_string_method. - slot_rep syntactic_str: receiver-ambiguous method names (slice, concat, replace, ...) now also require a syntactically-string receiver, so arr.slice() no longer classifies as a string-producing write; only the universal ToString/number-formatting family stays name-only. - Renamed note_canonical_i32_local -> note_canonical_local (it prints the rep and now also serves SlotRep::Str). - Completed the truncated repsel_context_allows_canonical_str doc sentence; fixed the changelog fragment's append(lhs_h, rhs_h) wording.
Contributor
Author
|
Review nitpicks addressed in f569123:
Re-validated after the changes: shadow-slot hygiene suite (11/11), the Phase 3a gap test byte-exact vs node in all four arms (flag on/off × |
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.
Phase 3a of
docs/representation-selection-rfc.md(§4 Str row, §5.5 boundary contracts, §5.6 GC rules): canonical string locals, tagged-at-rest. Builds on Phase 1 (#6903,SlotRep) and Phase 2 (#6905).Design: tagged-at-rest
SlotRep::Strmarks a function local proven to hold NaN-box string bits —STRING_TAG|ptrfor heap strings or inlineSHORT_STRING_TAGSSO bits — at rest. Unlike canonical-i32, the rep does not move storage: the local keeps itsctx.localsdouble slot, itsjs_shadow_slot_bindGC binding (the mark/evacuation path is NaN-box-driven and handles these bits unchanged — zero GC changes), and every alias/refcount demote site keeps firing. SSO values stay by-value at rest (RFC §4). What the rep buys is a compile-time proof the string-op lowerings consume to tag-dispatch inline instead of routing operands through the opaquejs_get_string_pointer_unified(which heap-materializes every SSO unbox and silently number-coerces).What gets deleted (per lowering)
s += part(fn-local accumulator)js_string_append(h,h)(in-place refcount==1 path kept) / both-string-SSO →js_string_concat_box/ else → exact legacy sequence.length, statically-string receiverlshr 40; and 0xFF; heap → bareload i32ofutf16_len; else → samejs_value_length_f64slow call===/<with a canonical-Str operandjs_string_equals/js_string_compareon raw handles; else → one SSO-aware call (js_jsvalue_equals/ newjs_string_compare_value) — never number-coercingcharCodeAt/at/codePointAtand POINTER_MASK; SSO/lie → legacy cold arm"lit" + valuecoerce-concatbitcast; and(literals are proven-heap: pool handles fromjs_string_from_bytes)StringRefmaterializationjs_nanbox_stringcallor STRING_TAG; bitcast, null cold arm preserves the allocate-empty contractEligibility (under-approximate, mirror of Phase 1)
Selected at the
Stmt::Letsite forString/StringLiteral-typed locals in sync function bodies. Excluded: closure-referenced, boxed vars, module globals (incl. all entry/top-level code — same init-split constraints as Phase 1), async/generator/was_plain_async, catch bindings, locals with a non-string-proven reassignment, and locals equality-compared against a non-proven-string operand (thecompare.rsother_side_is_anyhazard) — via a ctx-free pre-pass (collect_canonical_str_ineligible_locals).Correctness (RFC §5.5 acceptance contract)
Every specialized site keeps a fallback arm: the hot arms fire only on runtime-tag-verified string bits, and anything else (a lying
stringannotation) routes to the exact pre-phase sequence or a strictly-more-correct non-coercing helper — never a new silent coercion. The 6 alias-demote sites (let b = a, scalar array/object stores, write-barrier choke point) are untouched and keep firing, so the in-place+=fast path stays alias-sound.test-files/test_gap_repsel_canonical_str_locals.ts: alias+=discipline (local, scalar-array, object-field), SSO round-trip (+=/.length/===/relational at ≤5 bytes, JSON-parsed SSO vs heap literal), lying-annotation acceptance, non-ASCII (2-byte UTF-8, CJK, surrogate-pair emoji) byte-exactness, route-compare and char-scan shapes. Verified byte-exact vs node in all four arms: flag on / flag off / each ×PERRY_GC_FORCE_EVACUATE=1.shadow_slot_hygiene.rs: new test proves a canonical-Str local keeps its shadow-slot binding, the+=heap arm callsjs_string_appendwith no unified call, and.lengthtakes the tag dispatch, not the generic tower.js_string_compare_value(heap×SSO mixes, UTF-16 code-unit order, number-coercion parity with the legacy arm, invalid ranking).PERRY_CANONICAL_STR_LOCALS(default on) keyed into the object cache + key test.Structural proof (post-
opt -O3, before → after)Function-scoped kernel exercising the three recon shapes (
+=accumulator / route-compare / char-scan), per-functionjs_get_string_pointer_unifiedcall counts:+=accumulator (3 sites, loop body)strapp.rcold/strapp.coldfallback arms)===+<vs literals)streq.heapcallsjs_string_equalsdirectly,strcmp.heapcallsjs_string_comparedirectly.length+charCodeAtloop)strlen.heap/strrecv.heaparms contain zero calls (bareload i32/ bareand-mask)Block-level audit of the emitted IR confirms the hot arms contain only the intended direct helper (
js_string_append/js_string_equals/js_string_compare/ no call at all for.lengthand the receiver mask); every remaining unified call sits in a cold fallback arm or an entry-once demote. App-pattern kernels (string_concat_csv.ts,string_template_interp.ts): module-wide unified calls 1 → 0, withrows[i].lengththe 3-arm tag dispatch (heap arm = bareload i32).A review pass hardened the self-append arm layout: a heap destination ALWAYS reaches
js_string_append(so an SSO rhs cannot break the refcount==1 in-place accumulator into O(n²) copies), and the SSO-concat arm requires string tags on BOTH sides (a lying rhs keeps the legacy ToString coercion —"ab" += 42stays"ab42").Wall-clock benchmarks deferred — the bench box was unreachable at submission; timing follows as a PR comment when it returns.
Deferred (reported residuals)
bench_string_heavy's top-level+=loop keeps the legacy shape.indexOf,split,slice, …) keep the legacy unified unbox; same dispatcher can be applied incrementally.@.str.N.raw : i64constant global per literal was not added: literal operands already reach proven-heap handles viaload+bitcast+and(folds post-O3), with no per-literal binary growth.Summary by CodeRabbit
New Features
.length, concatenation, equality, relational comparisons, and character-access methods.Bug Fixes
undefinedhandling, Unicode correctness, and transitions from short to longer strings.Tests