Bytecode cache: inline 1–3 character strings, alias long strings from the payload; follow-ups from #497 review - #499
Conversation
…ncoding it; jsc shell page-touch tool retains its mapping only after a successful decode, stops at an empty frontier, clamps the residency delta
|
Warning Review limit reached
On-demand reviews are free for the next 28 days. After that, they cost $0.25 per reviewed file. Or wait 14 minutes for your next included review. View limit detailsLimit details: You’ve used the included review currently available. Your 68 included PR review attempts over the past 7 days set your current allowance at 1 review per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughChangesCached bytecode safety
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, targeted follow-ups. The one inline nit is a contrived edge case in a jsc-shell-only test helper and doesn't block.
Extended reasoning...
Overview
Two files touched with four small changes: (1) CachedJSValue::encode in CachedTypes.cpp adds a RELEASE_ASSERT(str.data.impl()) before the existing dereference, replacing a TODO comment; (2) functionBytecodeCachePageTouch in jsc.cpp moves the process-lifetime ref() to after the decode-success check, adds && !frontier.isEmpty() to the depth loop, and clamps the resident-page delta at 0.
Security risks
None. The CachedTypes.cpp change converts a pre-existing potential null-deref (UB) into a controlled RELEASE_ASSERT crash — strictly safer. The jsc.cpp changes are confined to a shell-only test/measurement helper that, per the PR description and prior PRs in this series, is not linked into Bun.
Level of scrutiny
Low-to-medium. The RELEASE_ASSERT is a defensive hardening of code that already dereferenced the same pointer on the next line, so it cannot introduce new failure modes — only surface an existing one earlier and deterministically. The remaining three changes are in a test helper: the empty-frontier guard and the unsigned-subtraction clamp are trivially correct micro-fixes.
Other factors
The bug hunter flagged one nit: moving ref() after the null check means that on the key-mismatch path (where decodeCodeBlockImpl has already materialized a borrowing UnlinkedCodeBlock before comparing keys), the mmap can be released while an orphaned block on the GC heap still aliases it. This is real but requires deliberately calling the helper with a cache generated from a different source in the same process — a contrived misuse of a test-only function. It does not affect the intended use or anything shipped in Bun, so I'm treating it as a non-blocking nit that the inline comment adequately covers.
…e may already have borrowers)
There was a problem hiding this comment.
LGTM — thanks for reverting the ref() move; the added comment captures why the mapping must be retained even on a rejected decode.
What was reviewed:
ref()is back before decode (comment-only change now), so the persistent-payload invariant holds on the key-mismatch path.!frontier.isEmpty()early-exit and thesize_tresidency clamp (comparison before subtraction, no wraparound) — both in the jsc-shell test helper only.RELEASE_ASSERT(str.data.impl())replaces a null-deref with an explicit crash on the encode path; strictly safer than before.
Extended reasoning...
Overview
Two files touched. jsc.cpp changes are confined to functionBytecodeCachePageTouch, a Linux-only jsc-shell test helper that is not linked into Bun: (1) a comment added above the pre-existing cachedBytecode->ref() explaining why the mapping is retained even when decode is rejected — this is the revert of the change I flagged in the previous run, now with rationale; (2) && !frontier.isEmpty() added to the BFS loop so it stops once there are no more nested code blocks to visit; (3) the residency delta residentAfter - residentBefore clamped at 0. CachedTypes.cpp replaces a // TODO: This seems wrong? with RELEASE_ASSERT(str.data.impl()) before the existing dereference in CachedJSValue::encode.
Security risks
None. The jsc.cpp hunk is a diagnostic/test helper in the standalone shell. The CachedTypes.cpp hunk converts a pre-existing potential null-deref (UB) into a RELEASE_ASSERT, which is strictly a hardening change on the bytecode-cache encode path.
Level of scrutiny
Low. Three of the four hunks are in a test-only tool; the fourth turns UB into a controlled crash without altering the success path. The one behaviorally interesting change from the original revision (moving ref() after decode) was reverted in response to my earlier comment, so the net diff there is comment-only.
Other factors
- Verified
residentBefore/residentAfterare bothsize_t(jsc.cpp:1826/1860); the>comparison guards the subtraction so there is no unsigned wrap. - The frontier-empty check is a pure early-exit — the inner loop over an empty vector was already a no-op, so behavior is identical, just fewer wasted iterations.
tryGetValue()returnsGCOwnedDataScope<const String&>;.data.impl()is theStringImpl*that was already dereferenced on the next line, so the assert fires exactly where the old code would have crashed anyway.- Author confirmed the revert in the thread; no other outstanding review comments.
… that would point at their record Record offsets are multiples of 4 and the empty sentinel ends in binary 11, so a slot whose low two bits are 01 holds (length, chars) instead of an offset; it decodes straight to AtomStringImpl::add. Applies to identifiers, string constants (CachedJSValue) and every other CachedPtr to a string. Minified code is mostly such names: on the 30 MB bundle, pages touched decoding every top-level function 49.1 -> 47.5 MB, that decode 465 -> 359 ms, decode-everything 752 -> 692 ms, cache 87.2 -> 86.5 MB (distinct short strings were already stored once, so the disk win is small).
Jarred-Sumner
changed the title
There was a problem hiding this comment.
Thanks for reverting the ref() placement — the persistent-payload invariant now holds on the rejected-decode path. I re-reviewed and didn't find any bugs. Because this changes the on-disk bytecode cache encoding (a new tag scheme in m_offset that relies on record offsets being 4-aligned and the empty sentinel ending in 11), a human look would still be worthwhile.
Checked: the 01 tag is disjoint from valid relative offsets (Offset is int32_t, s_invalidOffset = 0x7FFFFFFF, encoder allocations honour alignof(T)); inlineString().leakRef() is balanced by the isNewAllocation finalizer / adoptRef on both lean and non-lean paths; CachedPtr::get() returning nullptr for inline strings is only reached by callers that already handle the empty case; the length-0 case is excluded so isEmpty() still owns the null-string meaning.
Extended reasoning...
Overview
Two parts: (1) an encoding optimization in CachedTypes.cpp that packs 1–3 character Latin-1 strings directly into the 4-byte m_offset slot of VariableLengthObject instead of writing a separate record, using low-bit tag 01 to distinguish from real offsets (00) and the empty sentinel (11); (2) follow-up fixes to the bytecodeCachePageTouch jsc-shell helper — early-exit when the BFS frontier empties, clamp the residency delta at 0, and (after my earlier comment was addressed) keep the process-lifetime ref() before decode with an explanatory comment.
The inline-string path is wired into CachedPtr::encode/decode, CachedRefPtr::decode (lean path), CachedPtr::get, and CachedJSValue::encode/decode. A RELEASE_ASSERT replaces a pre-existing TODO where tryGetValue() could theoretically return a null impl.
Security risks
None identified. The bytecode cache is read from files the embedder controls; the new tag scheme doesn't introduce untrusted-input parsing beyond what already exists. The bit_cast between uint32_t and int32_t is well-defined (same size). No new heap writes from attacker-controlled offsets.
Level of scrutiny
High. CachedTypes.cpp is production bytecode-cache serialization for Bun — a bit-packing bug here would corrupt decoded code blocks or leak/double-free StringImpls across every cached script. The correctness of the tag scheme rests on an alignment invariant (both the target record and the &m_offset field land at 4-byte-aligned payload offsets so their difference has low bits 00) that was established in #494/#497 and is asserted by the author but not statically enforced at the hasInlineString() check site. That's the kind of invariant a human maintainer of this subsystem should sign off on.
Other factors
- My earlier inline finding (moving
ref()after the null check) was addressed by the author and the thread is resolved; the current diff keepsref()before decode. - Ref-counting on the inline-string decode path checks out:
leakRef()yields +1; on the non-lean pathisNewAllocation=trueschedules a finalizer deref and the returned pointer is adopted then re-ref'd (net +1 held by theRefPtr, matching the existing contract for newly-decoded impls); on the lean path it's directlyadoptRef'd. tryEncodeInlineStringrejects symbols, 16-bit strings, and length 0, soisEmpty()remains the sole null representation andCachedUniquedStringImpl's symbol path is unaffected.- The jsc.cpp changes are test-helper-only and now strictly safer than before.
- JSTests/stress reportedly passes through the disk cache in both lean and non-lean modes.
Given the subtlety of the format change and its blast radius, deferring rather than auto-approving.
Preview Builds
|
…ayload keep their characters in the mapping StringImpl::createWithoutCopying over the cached bytes, handed to AtomStringImpl::add, which adopts the impl in place unless an equal atom already exists; only the header is allocated and refcounting never touches the payload. Shorter strings still copy (a small malloc beats pinning part of a page).
Jarred-Sumner
changed the title
There was a problem hiding this comment.
I re-reviewed after the ref() revert and didn't find any bugs. Given this changes the on-disk cache format (bit-tagged offsets) and adds payload-aliased StringImpls whose lifetime rides on the persistent-payload contract, a human look would still be worthwhile.
Checked: the inline-string tag (low bits 01) is disjoint from s_invalidOffset (0x7FFFFFFF, low bits 11) and from real record offsets (encoder allocates at alignof(T) and the int32_t m_offset field is itself 4-aligned, so relative offsets have low bits 00).
Checked: the createWithoutCopying path is gated on canBorrowPayload(), same guard as the existing instruction-stream / metadata / expression-info borrows.
Checked: CachedRefPtr::decode and CachedJSValue::decode both handle the inline case before dereferencing buffer(); CachedPtr::get() returns nullptr for inline strings so callers fall back to a full decode.
Extended reasoning...
Overview
Two performance optimizations to the bytecode cache in CachedTypes.cpp, plus small follow-ups in the jsc shell test helper (jsc.cpp):
- Inline short strings: 1–3 char Latin-1 strings are packed directly into the 4-byte
m_offsetslot ofVariableLengthObjectusing a low-bit tag (01), instead of pointing at a separate string record. Encode/decode paths added toVariableLengthObject, wired intoCachedPtr,CachedRefPtr, andCachedJSValue. - Alias long strings: strings ≥48 chars decoded from a persistent payload use
StringImpl::createWithoutCopyingover the mmap'd bytes instead of copying, gated ondecoder.canBorrowPayload(). - Follow-ups:
RELEASE_ASSERTontryGetValue()inCachedJSValue::encode; empty-frontier early exit and clamped residency delta infunctionBytecodeCachePageTouch; a comment on the pre-decoderef()(my earlier finding on this PR was addressed by reverting theref()move).
Security risks
The payload-aliasing path creates StringImpls whose character storage lives in the mmap'd cache file. If the persistent-payload contract is ever violated (mapping unmapped while such a string is live), reads become use-after-munmap. The gate is the same canBorrowPayload() used by the existing instruction-stream/metadata borrows introduced in earlier PRs, so this doesn't add a new contract — but it does widen the set of borrowers to include general-purpose atom strings that can escape into arbitrary JS values, which is a larger blast radius than the previous borrowers (which stay inside UnlinkedCodeBlock). No injection/auth surface.
Level of scrutiny
High. This changes the bytecode cache wire format and introduces a bit-tagging scheme whose correctness depends on encoder alignment invariants (all string-record offsets and all m_offset field placements being multiples of 4). It runs on every cached-script load in Bun. A subtle mistake here would manifest as wrong string constants or memory corruption, not a clean failure. This is well outside the "simple/mechanical" bar for auto-approval.
Other factors
- The bug hunting system found nothing on the current revision.
- My one prior finding (moving
ref()after the null check in the jsc-shell helper) was addressed — the ref is back before decode with an explanatory comment. - The PR description reports JSTests/stress passing through the disk cache in both persistent+lean and non-persistent+non-lean modes.
jsc.cppchanges are test-helper only and low risk.
1 participant
Inline short strings. Record offsets are multiples of 4 and the empty sentinel ends in binary
11, so a 4-byte string slot whose low two bits are01can hold (length, up to 3 Latin-1 chars) instead of the offset of a string record; it decodes straight toAtomStringImpl::add. Applies to identifiers, string constants (CachedJSValue) and every otherCachedPtrto a string. Minified code is mostly such names — on the 30 MB / 58-library bundle from #497:Long strings alias the payload. Strings of 48+ characters decoded from a persistent payload use
StringImpl::createWithoutCopyingover the cached bytes (adopted in place byAtomStringImpl::add), so only the header is allocated and the characters stay in clean file-backed pages; refcounting never touches the payload. ~2–3 MB of string bytes on the bundle above; shorter strings still copy.Follow-ups from the #497 review (CodeRabbit, post-merge):
CachedJSValue::encode:RELEASE_ASSERTthattryGetValue()resolved (pre-existing// TODO: This seems wrong? What if this fails.) instead of dereferencing a possibly-null impl.bytecodeCachePageTouch(jsc shell test helper): stop when the frontier is empty; clamp the residency delta at 0; keep taking the process-lifetime ref before decoding (a rejected decode may already have borrowers), with a comment saying so.JSTests/stress through the disk cache (persistent + lean, non-persistent + non-lean decoder): clean.