perf(codegen): inline charCodeAt and stop routing it through the dynamic bitwise helper (#7592) - #7601
Conversation
…mic bitwise helper (#7592) The FNV-1a phase of honest_bench's json_pipeline spent 85% of its leaf profile in four opaque runtime calls per character over a 68 MB string. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
… json_pipeline header (#7592) Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
Also splits the inline charCodeAt lowering into a sibling module to stay under the 2000-line file cap. Claude-Session: https://claude.ai/code/session_01Y1QZ5wUP9gRSwpiweT4Wix
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ 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 PR adds guarded inline lowering for ChangesInline
Nursery-cap policy
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TypeAnalysis
participant Codegen
participant Runtime
TypeAnalysis->>Codegen: classify proven String charCodeAt call as numeric
Codegen->>Codegen: validate receiver, index, ASCII content, and bounds
Codegen->>Runtime: call runtime charCodeAt for unsupported cases
Codegen-->>Runtime: load ASCII byte for guarded fast-path cases
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
Post-rebase absolute numbers (branch on
|
| phase | #7592's table (v0.5.1335) | #7594 base |
this PR on #7596 |
|---|---|---|---|
| readFileSync | 89 ms | 76 | 59 |
| JSON.parse | 742 | 659 | 650 |
| build_out | 57,409 | 9,191 | 3,970 |
| JSON.stringify | 1,451 | 266 | 273 |
| writeFileSync | 90 | 19 | 20 |
| fnv1a | 1,250 | 1,247 | 108 |
| total | 60,431 | 11,495 | 5,087 |
| peak RSS | 1,064 MB† | 1,389 MB | 1,414 MB |
200k records, median of 3: build_out 1,397 ms, fnv1a 43 ms, total
1,842 ms, peak RSS 609.3 MB.
† the 1,064 MB row is from the original report on a different host (M1 Max)
and is not comparable to the pinned-host column. On one host, #7594 →
#7596 is +1.8% RSS and this PR is +0.0% — the interleaved A/B in the
description has both arms at 1,389.2 MB at 500k and 598.7 MB at 200k.
build_out and parse are now the whole remaining gap (4.6 s of 5.1 s). Both
are out of scope here; the design for the next build_out step (the double
copy #7596 names as its follow-up) is written up on #7592.
Broader correctness sweep
The is_numeric_expr change affects any charCodeAt / indexOf /
lastIndexOf / search / localeCompare on a statically-string receiver, so
I ran every gap test that uses one — 26 files, including
test_gap_atob_binary_high_bytes (the 0–255 binary-string round-trip),
test_gap_string_locale_2781_2845_2897, test_gap_sso_concat_string_index,
test_gap_repsel_canonical_str_locals, test_gap_object_string_wrappers,
test_gap_7232_i32_chain_double_rounding and test_gap_bigint.
26 pass, 0 diff, 0 node_fail, byte-identical to the Node 26.5.1 oracle.
Audit before merge — verified end to end, merged as v0.5.1340Reproduced the headline on my own probe — a runtime-built 33.5M-char ASCII
10.3×, identical output hash — consistent with the claimed 11.2× across Sabotage re-verified, both halves:
Root-dominance run because codegen changed (this is the class of change the Gates re-run here: Also verified the discipline items: Where #7592 stands after this
Remaining, per the phase split: |
1 participant
Closes the hash-phase half of #7592.
#7594and#7596tookbuild_outfrom 57 s to ~5–6 s; this is the phase that was next-largest behind it.
I re-measured before touching anything, and two of the ticket's three
figures were stale
#7592's phase table was captured before #7594/#7596. Re-running the
instrumented workload on the pinned quiet host at
v0.5.1338:JSON.stringifystringifyhad already been fixed by the GC pacing work — its old number wasGC pause charged to the phase it happened to land in. So there is no
stringify change in this PR: at 267 ms it is 2.5% of the run, and touching
JSON.stringify's shape-template path for that would be risk without areturn. The fnv1a phase was real.
Leaf profile: 85% of the hash phase was FFI, not work
PERRY_DEBUG_SYMBOLS=1+sample, isolated hash driver over the workload'sown 68 MB output, pinned quiet host:
js_string_char_code_atjs_dynamic_bitxorjs_string_index_to_i32js_get_string_pointer_unifiedh = (h ^ s.charCodeAt(i)) | 0was making four opaque runtime calls percharacter over 68 million characters. Every one of those helpers is a
handful of instructions; the cost is the FFI boundary — and an opaque call on
a loop's critical path also blocks LICM, so the loop-invariant receiver unbox
and header loads could never be hoisted out.
Two independent defects
1.
charCodeAtwas not statically a Number.is_numeric_exprhas armsfor locals, class fields,
Math.*, typed-array reads and user functions, butnone for a String-method call. So
h ^ s.charCodeAt(i)failedexpr/binary.rs's "both operands are statically primitive" test and routedthrough
js_dynamic_bitxor— the BigInt-aware helper — to compute an integerxor. Fixed by teaching
is_numeric_exprabout the String methods that lowerto a raw double.
The admitted set is exactly
charCodeAt,indexOf,lastIndexOf,search,localeCompare— each verified against its lowering inlower_string_method.rs(allsitofpof an i32 helper, or a helperdocumented to return a plain f64).
codePointAtis deliberately excluded:it returns
undefinedout of range, which is a NaN-box tag, not a number —claiming it numeric would hand a tagged value straight to an inline
fadd.at/charAt(strings) andstartsWith/endsWith/includes(booleans) areexcluded for the same reason. The claim is gated on the receiver taking
codegen's proven-string routing, mirroring
lower_call/property_get.rs'scondition exactly, so an
any-typed receiver — which may be a user objectwith its own
charCodeAt— is never claimed.2.
charCodeAtitself had no inline path. Added one: a guard chain thatreproduces exactly what
js_string_char_code_at+js_string_index_to_i32compute, falling back to those same two calls for anything it cannot prove.
STRING_TAGreceiver, handle ≥ 4096 (the runtime'sis_valid_string_ptrmagnitude check);
0.0 <= index < 2^31-1as ordered comparisons — a NaN-boxed index(a string, a bool,
undefined, a real NaN) fails both and takes the slowarm where the full
ToIntegerOrInfinityincluding uservalueOfruns.This also makes the subsequent
fptosiin-range, so it can never be poison;utf16_len == byte_len— the runtime's ownis_ascii_stringpredicate.Equality implies every byte is one UTF-16 code unit, hence every byte
< 0x80, so no WTF-8 / lone-surrogate / astral payload can reach the byte
load (Runtime split()/parseFloat() read past exact-sized slice allocations -> intermittent AV (c0000005) on hot paths #6085's bounded walk still owns those);
index < utf16_len.No allocation and no call occur between the receiver re-read and the byte
load, so no collection can move the header underneath the fast path.
No new env knob: it rides
PERRY_STATIC_STRING_LOWERING, the same gate thesibling inline
.lengthfast path uses (already keyed into the object cacheand the repsel knob-isolation harness).
The layout coupling is pinned, not assumed
The fast path reads
StringHeaderat offsets 0, 4 and 20. Offset 0 isalready an established contract (the inline
.lengthload), but it wasresting on a doc comment.
perry-codegencannot depend onperry-runtime,so the two sides get a
constassertion at the struct definition(
STRING_HEADER_ABI_MATCHES_CODEGEN): reordering, resizing or paddingStringHeadernow fails the runtime build, at the definition, instead ofsilently miscompiling every
.lengthandcharCodeAtin every user program.Result
Both arms built from one target dir with an identical package set
(
-p perry -p perry-runtime-static -p perry-stdlib-static), run interleavedon the pinned quiet host (
perry-macos.local, load 1.5–2.3 throughout).Output file
cmp-identical and the reported hash identical on every row.Isolated hash driver over the workload's own 68 MB output, 3 reps each,
three interleaved pairs:
11.2x, and the run-to-run spread is under 0.2% on both arms.
Full pipeline, median of 2–3 interleaved pairs after warmup:
Peak RSS is unchanged (−0.1% at 200k, −0.002% at 500k) — this is a codegen
change with no allocation behaviour, so it adds nothing on top of #7594's and
#7596's RSS cost.
build_outis statistically identical across the arms (200k: 2,349 vs2,351 ms; 500k: 9,127–9,192 vs 8,694–9,224, a ±5% band both sides). The first
500k pair I ran showed 8,461 vs 9,216 and I did not report it as a regression —
re-running showed it was a cold-page-cache first-run outlier on the base arm.
The post-fix leaf profile of the hash driver is a single symbol:
Every runtime call is gone from the hot path.
The A/B above is at the
#7594base (08940c877), because that is where Istarted before
#7596landed. That is the right base for this claim: the hashphase allocates nothing and never touches the collector, so GC pacing cannot
move it — and indeed
#7596is abuild_outchange. The branch is rebased on#7596and re-measured; absolute post-rebase numbers are in the thread.Also here
scavenge_nursery_cap_effective_bytesgained a
max(influx_driven, old_gen_reclaimable/2)term with no coverage,and the sibling
cap_scale_grows_on_heavy_influx_and_shrinks_when_quietonly looks like coverage: it asserts against the effective cap in a
unit-test thread whose old-gen is ~empty, so the proportional term
contributes 0 and that test stays green with the term deleted. The policy
is now a pure function of its two inputs and is tested directly.
json_pipelineworkload's header comment. It carried a v0.5.29block claiming
process.argv.slice(2)returns garbage, that iterating alarge
JSON.parseresult corrupts records above ~200, and that the drivertherefore runs Perry on the 100-record fixture only. perf: json_pipeline at 500k records is 97.6x bun (60.4s vs 618ms) while the same workload at 100 records BEATS bun — a scaling cliff, not a constant factor #7592 disproved the
second and third; I probed the first and it is also false. Deleted rather
than left to misdirect the next reader.
Validation
cargo test -p perry-runtime --lib --no-fail-fast— 1847 passed, 0 failed.cargo test -p perry-codegen --lib --no-fail-fast— 676 passed, 0 failed.test_gap_7592_char_code_at_inline.ts, byte-identical to theNode 26.5.1 oracle. It is deliberately adversarial to the fast path: ASCII
heap string, SSO receiver, empty string, accented (multi-byte) payload,
astral surrogate pair, lone surrogate, a full 0–255 binary-string round-trip
(every byte ≥ 128 makes
byte_len > utf16_len, so those must take the slowarm), out-of-range on both ends,
±Infinity,NaN, fractional and-0.5indices, string/bool/null/undefined/
{valueOf}indices, the no-arg form, anullable-string receiver, and the FNV-1a loop itself over each shape.
and the mutation reverted:
char_code_at_on_a_string_receiver_is_statically_numericfails, the other two pass;
..._emits_the_inline_ascii_readfails, the othertwo pass;
char_code_at_on_an_unproven_receiver_keeps_the_runtime_loweringfails,the other two pass;
nursery_cap_becomes_tenured_proportional_above_the_crossoverfails;The first pass of the codegen assertions was itself vacuous — they matched
@js_dynamic_bitxoranywhere in the module, which every module declares.They now match
call double @js_dynamic_bitxor. The negative control caughtthat: it was passing on the declaration alone.
StringHeaderABI assertion is sabotage-verified too — changingoffset_of!(byte_len)from 4 to 8 fails the runtime build withevaluation panicked, and restoring it compiles.raw_handle_debt.py998/998,addr_class_inventory.pypass,class_id_collisions.pypass,check_file_size.shpass (the inline loweringpushed
lower_string_method.rsto 2100 lines, so it moved to a siblingmodule
lower_string_method/char_code_at.rs),cargo fmt --all -- --checkclean,
cargo clippyon both changed crates introduces no new warning.CI has a deep backlog, so the above is local validation — that is what this
PR is standing on.
Summary by CodeRabbit
String.prototype.charCodeAtperformance with a faster path for ASCII strings.indexOf,lastIndexOf,search, andlocaleCompare.