perf(object/json): admit JSON.parse receivers to the object-write fast paths (#8098) - #8118
Conversation
…t paths (#8098) A `JSON.parse` object carries `class_id == 0`, and both guarded object-write fast paths — the whole-loop numeric clone and the static/dynamic write PICs — rejected it on exactly that, so every property write to parsed data took the generic `[[Set]]` path for the life of the program. Measured on the committed #6812 controlled pair (`receiver_class_id_zero` vs `key_dot`, identical 120,000,000 writes and identical `sink 122876400`), in instructions retired because wall clock on the development host is unusable: 150.08e9 -> 1.169e9, a 128.3x reduction landing at 1.009x of the object-literal cell. Scattered `record.field =` writes through a helper (the PIC path, the shape real code has) go 30.37e9 -> 2.50e9, 12.1x. `class_id != 0` was standing in for three per-object exclusions the generic path still applies (`NATIVE_MODULE_CLASS_ID`, `Object.prototype`, and a `URL` instance whose own slots are live views). None is derivable from the ShapeId, so this adds an explicit opt-in per-object mark instead: `OBJ_FLAG_PLAIN_ORDINARY`, set at birth by the JSON direct parser and the tape materializer, re-tested by every generated guard. Unmarked class-less receivers are unchanged. The bit is free in the emitted guard — `_reserved` is already loaded there for the blocking-flag test. Also fixes, in the same file: `JSON.parse("{}")` initialized eight inline field slots into an allocation that has `max(0, INLINE_SLOT_FLOOR)` = two, a 48-byte overwrite past the object on every empty-object parse. The allocator has initialized every slot it allocates since #4717, so the fill was redundant too.
658688a to
663f219
Compare
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (10)
📝 WalkthroughWalkthroughJSON-parsed objects are marked as plain ordinary objects. Static, dynamic, and numeric write fast paths accept marked receivers while preserving native-module and unmarked-object exclusions. Parsing and write semantics gain runtime and regression coverage. ChangesJSON object write optimization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant JSONParser
participant ObjectAllocator
participant PutValue
participant WritePIC
JSONParser->>ObjectAllocator: Allocate and mark JSON object
ObjectAllocator-->>JSONParser: Return marked receiver
PutValue->>WritePIC: Validate receiver kind and shape
WritePIC-->>PutValue: Execute optimized property write
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: ✨ 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 |
|
Verified the two claims I could check without building. Both hold. The drive-by memory-safety fix is real, and the arithmetic is exact6 JSValues × 8 bytes = 48 bytes past the object, on every Your reason for shipping it without a test is the right call and worth keeping in the changelog: under a forward bump allocator the overflow lands in not-yet-allocated space on every constructible path, so it only bites at a block boundary or on the malloc path. A test that cannot fail would be worse than the note. Worth flagging that this is the second consequence of #7928's floor change to surface today — the #6812 audit found the issue body still asserting the floor is 4 and no matrix cell re-measured after it moved. A change to that constant evidently has a wider blast radius than its own PR. The
|
|
Landing-equivalent validation is clean. Merging. I merged current No version bump, no manifest or lockfile change. What is landing128.3× fewer instructions on And a 48-byte heap overflow on every The two things that make this reviewable rather than just fastThe precedent, not the measurement. That the read PIC has admitted The disclosed cost. Marking the tape paths as well as the parser is what separates fixing the benchmark from fixing the workload — Merging with |
1 participant
Closes #8098.
What was wrong
A
JSON.parseobject carriesclass_id == 0. Both guarded object-write fastpaths rejected it on exactly that:
object_array_numeric_write_slots(
proxy/put_value.rs), which is what printedPERRY_OBJECT_ARRAY_WRITE_GUARD_REJECT: first receiver is not an eligible regular shared-shape object;dyn_ic_try_store(which also serves the outlined ways 5-8), plus the generated hit paths in
perry-codegen/src/expr/proxy_reflect.rs, which ANDedclass_id != 0intoevery one of the four inline ways.
So every
record.field = …on parsed data took the generic[[Set]]walk forthe life of the program.
JSON.parseis how essentially all external dataenters a Perry program, and the rejection was on the receiver's identity, so
no amount of loop-shape or key-form work in #6812 could reach it.
Measurements
Wall clock on the development host is unusable — the same cell measured
11,729 ms, 17,884 ms and 21,098 ms on the same binary. Everything below is
instructions retired (
/usr/bin/time -l), which reproduced to within 0.02%.Both arms are full
cargo build --release -p perry -p perry-runtime-static -p perry-stdlib-staticbuilds snapshotted with sha256, withPERRY_RUNTIME_DIRpinned to the snapshot; the compiler, both archives and the emitted benchmark
binary all
cmpas different between arms.The committed #6812 controlled pair (
benchmarks/object-write-6812/matrix.ts),identical 120,000,000 writes and identical
sink 122876400:receiver_class_id_zerokey_dot(reference)After the fix
receiver_class_id_zerois 1.009x ofkey_dot— parity withthe object-literal cell, which is the issue's acceptance bar. The trace line is
gone.
The shape real code actually has is not the clone-matched nest but scattered
.field =writes through a helper, which take the PIC. A fixture doing24,000,000 such writes over 2000 parsed records (
sink 48001997000on everyarm, node included):
12.1x fewer instructions; vs node this goes from 66x to ~5x.
No regression on the other 25 cells
Identical
(writes, sink)on all 26. Four cells moved in wall clock by morethan a couple of percent, so they were re-measured in instructions:
key_alternating_dynamic(dyn-key IC)storage_inlinerhs_allocatingrhs_pointer(write PIC, 96M writes)rhs_pointer's +0.87% is the honest cost and it reconciles exactly: the emittedguard gains 2 instructions per write, 96e6 x 2 = 1.92e8 = 0.87% of 2.2e10. Cells
that take the whole-loop clone pay nothing — the clone has no per-write guard.
Why not the two obvious fixes
Not a synthetic
class_idfor parsed objects (the__AnonShape_<hash>+js_register_anon_shape_class_idmechanism object literals use).class_id != 0means "class instance" at roughly thirty runtime sites —
console.logformatting,instanceof,getPrototypeOf,json/stringify.rs's fast template whichexplicitly requires
class_id == 0,url/search_params,symbol/properties—and only about a third of them carry the anon-shape escape. That is the whole
introspection surface of parsed data, plus a JSON-stringify regression.
Not dropping the clause. It was standing in for three per-object exclusions
the generic path still applies verbatim
(
field_set_by_name/fast_paths.rs::try_existing_own_data_overwrite):NATIVE_MODULE_CLASS_ID,Object.prototype, and aURLinstance — whosepathname/search/… own slots are live views whose setters rebuildhref(
field_set_by_name/tail.rs). None is derivable from the ShapeId: two objectsshare a ShapeId iff they share a keys-array allocation, and the
shape-transition cache deliberately converges distinct objects onto one shared
array, so a prime-time-only exclusion loses to ordering — a plain object primes
the site at t0, a
URLthat acquires the same keys array at t1 > t0 then hitsit. The generated hit path re-checks only per-object state, so the
discriminator has to be per-object.
What landed
An explicit, opt-in, per-object mark:
OBJ_FLAG_PLAIN_ORDINARY, bit 9 ofGcHeader::_reserved, object-only and disjoint from the array-onlyGC_ARRAY_ARGUMENTS_OBJECTbyobj_typeexactly as bits 11 and 12 already are(
array_gc_headerrefuses any header that is notGC_TYPE_ARRAY). The JSONdirect parser and the tape materializer set it at birth; one
write_fast_path_receiver_kind_ok(obj, flags)replaces theclass_id == 0clause at all four runtime guards, and the two emitted guards OR the bit into
their kind test.
It fails safe — every other class-less receiver is unmarked and keeps the full
[[Set]]walk, so no existing population changes behaviour — and it is free inthe generated guard, because
_reservedis already loaded there.Worth recording: the read PIC has admitted
class_id == 0all along(
field_get_set/ic_miss.rsprimes on any regular descriptor-free shapedreceiver; the emitted read guard has no
class_idcompare at all). Reads ofparsed objects were already on the ShapeId fast path — only writes were not.
#8067/#8086 supplied the rest:
js_object_alloc_class_inline_keysbirth-stampsa parsed receiver with a real ShapeId, and
PARSE_SHAPE_CACHEgives repeatedparses of one shape a single
GC_FLAG_SHAPE_SHAREDkeys array, so the whole2400-receiver prefix carries one ShapeId.
Drive-by memory-safety fix
JSON.parse("{}")initialized eight inline field slots into an allocationthat has
max(0, INLINE_SLOT_FLOOR)= two of them (the floor dropped 4 -> 2in #7928) — a 48-byte overwrite past the object on every empty-object parse,
the exact "heap buffer overflow into adjacent arena objects" that
js_object_alloc_with_parentdocuments. The hand-rolled fill was redundant aswell: the allocator has initialized every slot it allocates since #4717.
No unit test: under a forward bump allocator the overflow lands in
not-yet-allocated space on every path a test can construct, so a deterministic
corruption probe is not available — it only bites at an arena block boundary or
on the malloc path. The arithmetic is in the code comment.
Coverage
Three new runtime unit tests (
cargo-test-visible), each sabotage-verifiedwith a real rebuild that recompiled
perry-runtime:class_id != 0only)mark_object_plain_ordinarycallOBJ_FLAG_PLAIN_ORDINARY0x200 -> 0x1000json_parse_receivers_are_admitted_to_the_whole_loop_write_clonedrives theshipped
js_json_parseend-to-end, asserts the premises (class_id == 0, areal shared ShapeId), then clears the mark on one receiver and requires
the guard to refuse — same objects, same ShapeId, same slots, only the mark
differs.
json_parse_receivers_prime_the_static_write_pic— same discrimination on thePIC prime path.
plain_ordinary_object_flag_matches_the_emitted_write_pic_literalpins thebit against the literal
perry-codegenemits.object_array_numeric_write_guard_requires_complete_uniform_proofkeeps its class-id-zero rejection for an unmarked receiver and gains the
marked-accepts and native-module-still-rejects halves.
Payloads are a few bytes with an object root, so
js_json_parsetakes the eagerdirect parser and no lazy tape stands between the probes and the objects they
inspect (#7635). The tape is not material to this benchmark — it needs
1 KB <= len <= 16 MBand a[root — but it IS material to the workloadsthe issue is about, which is why
json_tape.rsandjson_tape/iterative.rsaremarked too.
test-files/test_gap_json_parse_object_writes.tsis parity against node 26.5.1for the semantics the
class_id != 0clause used to keep parsed objects awayfrom: deleted keys, added keys, frozen/sealed/non-extensible receivers (strict
TypeErrors), accessor and non-writable descriptors installed over a parsedslot, prototype mutation with a shadowing setter, null prototypes, dynamic-key
writes, a parsed object used as a prototype, an empty parsed object grown by
name, a polymorphic site mixing parsed objects / literals / class instances, and
__proto__/constructoras genuine own data keys. It is byte-identical tonode before and after — by design; it is a semantics regression guard for
the newly-enabled fast paths, not a witness for the perf fix, whose witnesses
are the sabotage-verified unit tests above.
Local validation
cargo test -p perry-runtime --lib-> 2354 passed, 0 failed, 4 ignored(baseline 2351/0/4 + the 3 new tests).
cargo test -p perry-codegen --no-fail-fast-> 27 suites, 1434 passed,11 failed — byte-for-byte the documented pre-existing set (CI: required contexts
lintandcargo-testare red for every PR (andlintonmainitself), so every merge bypasses them #8092); thefailing names are the buffer-read / typed-feedback / back-edge-poll cluster,
none of them touched here.
lint:cargo fmt --all -- --check,check_file_size.sh,workspace_architecture.py --check,addr_class_inventory.py,class_id_collisions.py,gc_store_site_inventory.py,gc_runtime_root_holders.py,gc_pin_sites.py,check_gc_env_knobs.py,check_gc_doc_claims.py,check_locale_independent_io.py,raw_handle_debt.py,local_binding_type_audit.py,global_sink_isolation.py,check_test_registration.py— all green.No version bump (maintainer bumps at merge).
Summary by CodeRabbit
Performance
JSON.parse, including repeated, static, and dynamic writes.Bug Fixes
Tests