fix(runtime): a built-in callee receives the primitive, not a ToObject wrapper - #9800
fix(runtime): a built-in callee receives the primitive, not a ToObject wrapper#9800proggeramlug wants to merge 4 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 |
proggeramlug
added
the
run-extended-tests
|
Re-measured on current | arm | 400 cpu s | 400 idle12 cpu s | 3300 cpu s | 3300 idle12 cpu s | typing cpu r2 | echo p90 ms | turn r2 cpu s | FP settled 400 MB | FP end-turn 400 MB | peak RSS 400 MB | FP settled 3300 MB | peak RSS 3300 MB | startup s | Primitive-method fallback counter on the candidate: Allocation-site categories, streamed turn: |
…l origins, allocation-site sampling Three instruments for the cc-perf campaign, all inert unless asked for. `PERRY_GC_DIAG=1` gains the lines that say WHY the collector ran: `[gc-trigger]` (every predicate input at each decision site), `[gc-full]` (the arm behind each synchronous full mark-sweep, counted per site), `[gc-budgeted] start/done` (steps, per-phase step time, root-scan share), `[gc-charge]` (mutator-assist / synchronous-full time per calling site, resolved to JS display names) and `[gc-survival]` (per copying minor, the root that first reached each surviving byte — shadow stack, native stack map, named scanner, remembered set by old-parent type — with transitive reach charged to the originating root through a parallel worklist origin vector). `PERRY_ALLOC_SITE_SAMPLE=<bytes>` samples the arena allocation sites byte- proportionally across the runtime allocators AND the codegen inline bump path (the mirrored inline block limit is capped at one interval while sampling, so the fast path returns to the runtime once per interval). The survival test is sabotage-checked: disabling the drain propagation charges the 40 elements to `worklist_drain` and the test fails on that row. The knob's OFF state and magnitude parse are pinned next to the other GC knobs. `gc_diag_enabled()` gets the per-thread test override the census already has, so the diag paths are testable without touching the process environment. Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
0857acc to
99f2e38
Compare
The A/B this PR never had: same binary, one branch, both arms
400-character streamed reply, three repeats per arm
3300-character streamed reply, two repeats per arm
Typing + short turn (
|
| arm | startup (s) | typing CPU r2 | turn CPU r2 | echo p90 r2 (ms) | r3 turn CPU | idle 10 s CPU |
|---|---|---|---|---|---|---|
| base | 2.21 | 1.53 | 0.94 | 33 | 56.49 | 5.91 |
| gate 0 | 2.17 | 0.80 | 1.00 | 23 | 20.24 | 1.02 |
| gate 1 | 2.14 | 0.92 | 1.67 | 29 | 17.66 | 0.77 |
| node | 1.30 | 0.09 | 0.05 | 2 | 0.18 | 0.01 |
Verdict: flat, exactly as the PR body predicted. Every gate-0/gate-1 pair
is inside its own arm's spread, in both directions and on both metrics.
Why it is flat, proved rather than assumed
PERRY_GC_DIAG=1 on a 400-character reply, both arms:
[gc-primitive-dispatch] minor: iter_result_objects=167946
(no builtin_callee_primitive_calls line — never emitted)
(no string_wrappers line — never emitted)
(no primitive-method fallback histogram — never emitted)
The counter this branch carries increments at the fork itself, before the
gate, so it counts the population the change converts regardless of which arm
is running. It is never emitted, so zero calls reach the built-in-callee fork
during a claude-code reply and no String wrapper is minted at all. The
change therefore removes nothing on this workload and cannot regress it — which
also settles the one row that reads worse (3300 settled footprint 454/466
against 430/421): the two arms executed the same code, so that difference is
run-to-run variance in a metric the campaign documents as bimodal, not an effect
of this diff.
Behaviour, on a workload that DOES reach the fork
Two offline probes compiled with the same binary and run in both arms plus node.
Probe A, 51 cases — reflection and coercion around boxed strings
(hasOwnProperty, propertyIsEnumerable, Object.keys, getOwnPropertyNames,
spread, in, Object.prototype.toString.call, Object(str)[i],
getOwnPropertyDescriptor, sloppy/strict user callees, writes through this,
fn.call/fn.apply with a string thisArg, Number/Boolean/BigInt/Symbol
receivers): byte-identical between the two arms, and byte-identical to node.
Probe B, 46 cases, chosen to FORCE the fork — direct calls to the built-ins
least likely to have a native dispatch arm (anchor, big, blink, bold,
fixed, fontcolor, fontsize, italics, link, small, strike, sub,
sup, toWellFormed, isWellFormed, localeCompare, normalize,
toLocale{Upper,Lower}Case, the Object.prototype inheritors, and the same
set again on a 200-character receiver): byte-identical between the arms, and
identical to node on 45 of 46. The one differing line is identical in BOTH arms
and is a separate pre-existing defect (a dynamically-keyed property read on a
primitive — const k = "charAt"; typeof "abc"[k] is "undefined" in perry and
"function" in node, while "abc"[k](1) returns "b"); it is filed
separately and is not this change.
Probe B also moves the counters the way the mechanism predicts:
gate 0: string_wrappers=4 index_properties=17 builtin_callee_primitive_calls=3
gate 1: string_wrappers=3 index_properties=12 builtin_callee_primitive_calls=3
Three calls reach the fork; one of them has a five-character string receiver;
with the change on, that wrapper and its five own index properties are gone and
nothing else moves. That is the smallest possible complete demonstration: same
population counted in both arms, one wrapper removed, output unchanged.
Status
The metric condition is met and the mechanism is proved, so the reason this is
still a draft is now only the stack: it sits on #9794 and #9795, which I have
just rebased onto current main (c7361c87c) after they went CONFLICTING.
I will take it out of draft as soon as the rebased stack's checks are green —
there is nothing left to measure on it.
Provenance: cc_gc5 is a runtime-only branch on 1d63fa91f, so it links the
same emitted-JS object as cc_base_new; the codegen id derived from
perry-codegen + perry-hir is identical (22c732a67c78a2ac) for both.
Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m
99f2e38 to
a88f81a
Compare
…iptors on the primitive-string path
Three allocations a JS program can never observe, found with the
allocation-site sampler (`PERRY_ALLOC_SITE_SAMPLE`) on the compiled
claude-code TUI, where they are the largest attributed source of garbage in
both the streaming turn and the render pass that follows it.
1. A one-ASCII-character string is now the canonical per-thread header.
`js_string_char_at` minted a fresh 32-byte heap string per character read,
and everything that walks a string a character at a time goes through it:
`s[i]`, `charAt`, string spread, the String-wrapper index installer. There
are 128 possible contents. The table has the same residency contract as the
small-integer string table next to it (longlived arena, `refcount = 0` so it
is never mutated in place, pinned out of the young generation) and rides
that table's existing root scanner rather than registering a 96th one.
2. Runtime-internal constant property names resolve through the intern table.
The `globalThis` builtin lookup, `x.constructor`, `toString` resolution and
primitive-method dispatch each built a fresh heap string for a literal name
on every call; `js_get_global_this_builtin_value` alone accounted for 133 MB
of the 990 MB one 3300-character reply allocates. `string::canonical_key`
routes them through the content-keyed per-thread table that
`js_string_materialize_to_heap` already uses, which is also what the
property read/write fast paths require of a key.
3. A `String` wrapper no longer stores a property descriptor per character.
ECMA-262 §10.4.3 gives every in-range index of a String exotic object
`{ writable: false, enumerable: true, configurable: false }` — a fact of the
class and the boxed length, not per-object state — so `get_property_attrs`
answers it from the wrapper's payload. Storing it cost, per boxed character,
a Rust `String`, a `PROPERTY_DESCRIPTORS` entry only a full collection's
dead-owner prune could reclaim, an owner-index entry, and one program-wide
`prop_plan_epoch_bump()`. A sloppy method call on a string primitive boxes
its receiver, so the TUI paid all of it per rendered line. A real stored
descriptor still wins, so `Object.freeze`/`defineProperty` on a wrapper are
unchanged.
`PERRY_GC_DIAG=1` also gains `[gc-primitive-dispatch]`: which
`<Builtin>.prototype.<method>` names reach the primitive-method fallback, how
often, and how many wrapper index properties they cost — the counter that says
whether a boxing fix ran.
Claude-Session: https://claude.ai/code/session_01YPfnmWZmSpSWpmnoXvH8z2
`codePointAt` had a `String.prototype` thunk but no arm in the native string-method dispatch, so every call fell through to `call_primitive_builtin_prototype_method`: resolve `globalThis.String.prototype.codePointAt`, clone that closure to rebind `this`, and — the thunk not being registered strict — run `ToObject` on the receiver, minting a `String` wrapper whose own index properties are one per UTF-16 code unit. The new `[gc-primitive-dispatch]` counter says how much that cost: on the compiled claude-code TUI, `codePointAt` is the ONLY method name that reaches the fallback at all, and it reaches it 99,008 times per 400-character streamed reply — 99,008 `globalThis` lookups, 99,008 closure clones and 99,008 String wrappers, because grapheme-aware text measurement calls it once per character. The arm is the sibling of `charCodeAt` one line above it and reads the receiver the same way. The test asserts the WRAPPER COUNT rather than the return value: the fallback computes the same number, so an answer-only test would pass with the arm deleted. A positive control pins that the counter can move.
…t wrapper ECMA-262 §10.3.1: a BUILT-IN function's [[Call]] does not run OrdinaryCallBindThis. It receives `thisArg` unchanged and performs whatever coercion it needs itself — which every thunk in `primitive_proto_thunks` already does, accepting the raw primitive BEFORE it looks for a wrapper payload (`string_receiver_or_throw`, `number_receiver_or_throw`, ...). `call_primitive_closure_value` boxed for them anyway, and for a string receiver the `ToObject` wrapper materialises an own index property per UTF-16 code unit. The `codePointAt` arm (PerryTS#9761) showed what one method name reaching this path costs: 99,008 wrappers per 400-character claude-code reply. This closes the class rather than the instance — the next builtin with a prototype thunk but no native dispatch arm costs a lookup, not a wrapper per character. Only a sloppy USER callee still gets the wrapper. `builtin_closure_length` is the registry that separates the two, and the test pins both directions: a `String.prototype` method closure reads as a built-in, a closure the runtime merely allocated does not — without the negative case the predicate could be "always true" and still pass.
a88f81a to
32f150b
Compare
|
Changelog fragment renamed to carry this PR's number, after checking the gate's
Verified the rename against the gate directly ( Claude-Session: https://claude.ai/code/session_014UZWia6L37DpA93VLtNK9m |
jdalton
left a comment
jdalton
left a comment
There was a problem hiding this comment.
Review of 32f150b22349f9da6886f7180ec13da0a7e4d671 (2026-09-07).
Blocking rooting bug in the added codePointAt arm: js_string_code_point_at(s_ptr, arg_i32(0)) keeps the pre-coercion raw receiver pointer while arg_i32 can execute user valueOf and trigger moving GC. This function already has s_handle/receiver_string() and the adjacent slice arm explicitly coerces first and re-fetches the receiver for this reason. Compute the index first, then call the helper with receiver_string(). Add a dynamically dispatched, heap-string codePointAt({valueOf(){ /* allocating/collecting work */ return 1; }}) regression under forced evacuation and assert the coercion runs once. The new arm must not inherit the adjacent older raw-pointer shape.
Validation scope: source/diff inspection; I have not run this PR's build or test suite locally.
2 participants
Stacked on #9794 and #9795.
What
ECMA-262 §10.3.1: a built-in function's
[[Call]]does not runOrdinaryCallBindThis. It receivesthisArgunchanged and performs whatevercoercion it needs itself — which every thunk in
primitive_proto_thunksalready does, accepting the raw primitive before it looks for a wrapper
payload (
string_receiver_or_throw,number_receiver_or_throw, …).call_primitive_closure_valueboxed for them anyway. For a string receiverthat
ToObjectwrapper materialises an own index property per UTF-16 codeunit, so a builtin with a
String.prototypethunk but no native dispatch armcosts O(receiver length) allocations per call.
#9795 fixed exactly one such name. This closes the class: only a sloppy user
callee is owed the wrapper now, which is the distinction the spec draws.
Why it is worth landing even though the rig shows no delta
After #9795,
[gc-primitive-dispatch]reports zero trips through thefallback on the claude-code TUI — the native tower answers every method name
that workload calls, so this change removes nothing there and the rig table
is expected to be flat within noise. Its value is that the next missing arm
costs a lookup instead of a wrapper per character, and that the boxing was a
spec divergence in its own right (
String.prototype.at.call("abc", 0)andfriends must observe the primitive, not a wrapper).
I would rather land it on the spec argument plus the class-of-bug argument than
claim a number it does not produce.
Test
builtin_prototype_method_closures_are_distinguishable_from_ordinary_onespinsthe discriminator in both directions — a
String.prototypemethod closurereads as a built-in, a closure the runtime merely allocated does not. Without
the negative case the predicate could be "always true" and still pass, which
would silently strip the wrapper from every sloppy user callee.
Behavioural coverage for the sloppy-user-callee half needs real JS closures and
a populated
globalThis, which the runtime unit-test thread does not have; itcomes from the extended JS gates (label applied).
cargo test -p perry-runtime --release -- --test-threads=1: 3156 passed,0 failed.