structuredClone/postMessage: add fast path for bare primitive values - #36591
structuredClone/postMessage: add fast path for bare primitive values#36591robobun wants to merge 4 commits into
Conversation
A non-cell JSValue (int32, double, boolean, null, undefined, and BigInt32 where enabled) has no heap identity and is safe to carry across threads as-is, so structured clone is the identity function on it. The existing fast-path block in SerializedScriptValue::create was gated on value.isCell(), so these values fell through to the full CloneSerializer: transfer-list scan, ObjectPool/HashSet setup, Vector<uint8_t> heap alloc, version header + tag writes, and a symmetric CloneDeserializer on receive. Add FastPath::Primitive, storing the JSValue directly on the SerializedScriptValue, and take it for any non-cell input inside the existing canUseFastPath guard (so SerializationForStorage callers like bun:jsc serialize() keep producing real wire bytes). structuredClone() itself additionally returns the value unchanged when there is nothing to transfer, skipping the SerializedScriptValue allocation entirely.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
WalkthroughChangesPrimitive structured-clone fast path
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@test/js/web/structured-clone-fastpath.test.ts`:
- Line 6: In the primitive fast-path tests, remove the section banner and
transient timing-history comments, including the related comments around the
additional referenced lines. Keep only one concise comment documenting the
durable invariant that primitive cloning must bypass the serializer path.
- Around line 9-26: Add boundary cases to the cases array used by the
structured-clone fast-path test: include INT32_MIN and INT32_MAX values, the
supported BigInt32 minimum and maximum limits, and retain an out-of-range BigInt
case to verify fallback behavior. Use the project’s existing limit symbols or
conventions where available.
- Around line 39-54: Update test/js/web/structured-clone-fastpath.test.ts lines
39-54 in the MessageChannel test to reject the awaiting promise through
port2.onmessageerror and close both ports in a finally block. Update lines 79-82
in the worker request test to route worker.onmessageerror to rejection of the
current request; ensure each failure path cannot leave a promise pending.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7e387f67-4433-41e6-8c86-4fa4cf108efb
📒 Files selected for processing (4)
src/jsc/bindings/webcore/SerializedScriptValue.cppsrc/jsc/bindings/webcore/SerializedScriptValue.hsrc/jsc/bindings/webcore/StructuredClone.cpptest/js/web/structured-clone-fastpath.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
…e messageerror to rejection
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked: the !value.isCell() guard correctly excludes the empty JSValue on JSVALUE64 (encodes as 0, which satisfies isCell()), heap BigInts, and Symbols (all cells → fall through to the full serializer as before). canUseFastPath already gates on forStorage == No, so bun:jsc serialize() still emits wire bytes. The StructuredClone.cpp identity return runs after convertDictionary and only when transfer.isEmpty(), so transfer-list validation and detach semantics are preserved. Both m_fastPath switches (computeMemoryCost, deserialize) have the new Primitive arm.
Extended reasoning...
This is a small, well-contained perf optimization that mirrors the existing FastPath::String/SimpleObject pattern (which already stores bare non-cell JSValues in SimpleCloneableValue for cross-thread transfer). The only finding is a cosmetic redundant-guard nit. Deferring rather than approving because this is C++ JSC structured-clone code on a cross-thread path — worth a maintainer glance even though I found nothing wrong with the mechanism.
…imitive early return
|
Updated 9:27 PM PT - Jul 31st, 2026
✅ @robobun, your commit cf904695806fe833ebefe61822e22ee84cb25ea4 passed in 🧪 To try this PR locally: bunx bun-pr 36591That installs a local version of the PR into your bun-36591 --bun |
There was a problem hiding this comment.
No bugs found, but this is a perf optimization in SerializedScriptValue (cross-thread JSValue carrying + a timing-ratio assertion in the new test), so worth a human look.
Checked: canUseFastPath already excludes SerializationForStorage::Yes and non-empty transfer lists, so bun:jsc serialize() and transfer-detach still hit the full serializer. Both m_fastPath switches (computeMemoryCost, deserialize) have the new Primitive arm. Storing a bare non-cell JSValue cross-thread matches the existing SimpleCloneableValue pattern used by SimpleObject/SimpleArray. The StructuredClone.cpp identity return runs after convertDictionary, so invalid transfer options still throw first.
Extended reasoning...
Overview
Adds FastPath::Primitive to SerializedScriptValue so bare non-cell JSValues (int32/double/bool/null/undefined/BigInt32) skip the full CloneSerializer. Touches SerializedScriptValue.{h,cpp} (new enum arm, ctor, member field, two switch arms, early return in create), StructuredClone.cpp (identity return for non-cell + empty transfer), and adds ~140 lines of tests to structured-clone-fastpath.test.ts. My earlier nit about the now-redundant if (value.isCell()) wrapper was addressed in cf90469.
Security risks
None identified. Non-cell JSValues are immediate 64-bit encodings with no heap identity, so carrying them across threads as-is is safe — the same pattern is already used for SimpleCloneableValue property values in the existing SimpleObject/SimpleArray fast paths. No user-controlled parsing, no allocation sizing from untrusted input.
Level of scrutiny
Medium-high. This is hot-path C++ in the JSC bindings that runs on every postMessage/structuredClone, and the SerializedScriptValue object crosses thread boundaries (Worker, BroadcastChannel). The change is small and follows an established in-file pattern, but perf claims and cross-thread value handling in JSC bindings are exactly the kind of thing REVIEW.md flags for maintainer sign-off. The new timing-ratio test (primTime < mapTime * 0.25) uses best-of-3 over 2k iterations with a ~50x margin between the observed ratio (~0.005) and the threshold, which looks robust, but timing-based assertions in CI deserve a second opinion.
Other factors
- Verified both
switch (m_fastPath)sites (memory cost, deserialize) cover the new arm; no other consumers of the enum. canUseFastPathgates onforStorage == No,forTransfer == No, and empty transfer/port lists, sobun:jscserialize()and transfer-detach are unaffected (both covered by new tests).- The empty-
JSValueedge case (isCell()==true on JSVALUE64) is unchanged from pre-PR behavior — it fails the!isCell()test and reachesasCell()exactly as before. - One minor unaddressed CodeRabbit nit remains about the wording of the perf-test comment ("off the fast path it runs at roughly half the Map cost"); not blocking.
2 participants
What
port.postMessage(1),structuredClone(42), and the like fell off the structured-clone fast path into the fullCloneSerializer. This addsFastPath::Primitiveso a bare non-cellJSValue(int32, double, boolean,null,undefined, andBigInt32where enabled) is carried as-is.Why
The
canUseFastPathblock inSerializedScriptValue::createonly looked atvalue.isCell()candidates (strings,JSArray, flat objects). A non-cell value fell past the block into the full serializer: transfer-list scan,ObjectPool/HashSetsetup,Vector<uint8_t>heap alloc, version header + tag writes, plus a symmetricCloneDeserializeron receive.A non-cell
JSValuehas no heap identity and is safe to carry across threads as-is. The existingSimpleObjectfast path already stores primitive property values as bare cross-threadJSValues, so this follows the same established pattern for the top-level value.How
SerializedScriptValue.h: addFastPath::Primitive,m_fastPathPrimitive,createPrimitiveFastPath, and the matching private ctor.SerializedScriptValue.cpp: insideif (canUseFastPath)and before thevalue.isCell()branch,if (!value.isCell()) return createPrimitiveFastPath(value);. Ordering is load-bearing: onJSVALUE64the emptyJSValuesatisfiesisCell() == trueand keeps going down the existing branch;SerializationForStoragecallers (bun:jscserialize()) are already excluded bycanUseFastPathand keep producing real bytes. Add thePrimitivearm to both thecomputeMemoryCostanddeserializeswitches.StructuredClone.cpp: when the transfer list is empty and the value is non-cell, return it directly without constructing aSerializedScriptValue.Measurement
Release build, 1M iterations of
structuredClone(42):structuredClone(42)structuredClone(new Map())(full serializer)Testing
Added to
test/js/web/structured-clone-fastpath.test.ts:-0/NaN/Infinity, booleans,null,undefined, small/large/negative BigInt) viastructuredClone,MessageChannel, and a cross-threadWorkerecho.bun:jscserialize()still produces real bytes for every primitive (theSerializationForStorage::Yespath is unchanged).structuredClone(42, { transfer: [buf] })still detaches the buffer.structuredClone(42)runs at <0.25x the cost ofstructuredClone(new Map())(fails at ~0.5x on the previous build, passes at ~0.005x here).Regression-checked against
test/js/web/workers/{structured-clone,structuredClone-classes,message-channel,message-port-pipe,worker-postmessage-transfer}.test.ts,test/js/web/broadcastchannel/broadcast-channel.test.ts, andtest/js/node/worker_threads/worker_threads.test.ts(553 tests, all pass).[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file