JSC/WTF: startup snapshot support (immortal snapshot heap, restore hooks, deterministic placement) - #397
JSC/WTF: startup snapshot support (immortal snapshot heap, restore hooks, deterministic placement)#397Jarred-Sumner wants to merge 4 commits into
Conversation
Preview Builds
|
…ilds without a local WebKit; SNAPSHOT.md: shipping state
6e1b91f to
c3de0a2
Compare
Jarred-Sumner
changed the title
c3de0a2 to
8e80331
Compare
b5ebcff to
623bbd8
Compare
Jarred-Sumner
changed the title
8c4fde2 to
3288387
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds immortal startup snapshots, snapshot restoration across JavaScriptCore and WTF, borrowed persistent bytecode-cache payloads, cache-based code-block re-decoding, snapshot-aware structures and watchpoints, and related memory accounting. ChangesStartup snapshot and bytecode support
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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: 32
🤖 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 `@Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp`:
- Around line 357-376: Move UnlinkedCodeBlock::componentSizesForCensus() into
the existing namespace JSC block before its original closing brace, and wrap
both its declaration in UnlinkedCodeBlock.h and definition with the
USE(BUN_JSC_ADDITIONS) guard. Ensure the API is absent consistently when the
feature is disabled.
- Around line 103-110: Guard the snapshot-specific logic in the visitation path
with USE(BUN_JSC_ADDITIONS), including the Heap::isStartupSnapshotCell check,
optional locker, and snapshot lock/aging skip. Preserve the existing upstream
locking and aging behavior when the feature is disabled.
In `@Source/JavaScriptCore/bytecode/Watchpoint.cpp`:
- Around line 236-237: Update the watchpoint transfer logic around
snapshotSideChain and m_set.takeFrom so snapshot-resident source sets remain
unchanged, including m_setIsNotEmpty. Replace the unlinking transfer with a
deferred representation that records the watchpoints without detaching or
mutating other->m_set; retain side-chain handling only for post-restore
additions.
- Around line 106-135: Guard the Bun-specific startup-snapshot implementation
consistently with USE(BUN_JSC_ADDITIONS): wrap the side-chain storage and
helpers in Source/JavaScriptCore/bytecode/Watchpoint.cpp (lines 106-135), the
side-chain declaration in Source/JavaScriptCore/bytecode/Watchpoint.h (line
282), and the startup-snapshot executable path in
Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp (lines 242-272).
In `@Source/JavaScriptCore/heap/BlockDirectory.cpp`:
- Around line 357-379: Update BlockDirectory::makeAllBlocksImmortal to assert
the sweeper is suspended before mutating allocators or directory bit vectors,
and iterate m_localAllocators while holding m_localAllocatorsLock. Verify and
enforce at the caller that a complete synchronous sweep finishes before this
method runs, preserving destructor execution for all blocks previously marked
unswept.
In `@Source/JavaScriptCore/heap/Heap.cpp`:
- Around line 1270-1278: Change Heap::rememberSnapshotCell from returning bool
to void in both its Heap.h declaration and Heap.cpp definition, and remove the
unconditional return true statement while preserving the existing snapshot
bookkeeping and mark-stack behavior.
- Around line 3157-3165: Reformat the diagnostic and string-freezing loops in
the surrounding Heap logic so each declaration, increment, conditional, and side
effect occupies its own line. In particular, expand the multi-statement lambda
body near the verboseStartupSnapshotFreeze diagnostic and the corresponding
loops around the string-freezing logic, preserving behavior while following the
file’s one-statement-per-line style.
- Around line 1244-1256: Update Heap::resetPacingAfterSnapshotRestore to
initialize m_maxHeapSize with the same minHeapSize(m_heapType, m_ramSize)
expression used by the Heap constructor, rather than Options::mediumHeapSize(),
so restored heaps retain their type- and RAM-specific fresh-heap limit.
- Around line 3707-3713: Remove the conditional visitor.dumpMarkerData(cell)
call guarded by Options::verboseVerifyGC() in the GC verifier error path,
retaining the unconditional call so marker data is emitted exactly once.
- Around line 1258-1266: In Heap::evacuateTablesForStartupSnapshot, extract the
repeated copy-and-swap operations for m_weakGCHashTables, m_snapshotWrittenEver,
and m_snapshotUnlinkedCodeBlocks into a small rehomeTable helper. Document that
the helper moves backing storage out of snapshot pages, and use it for each
table while preserving the existing empty-table behavior.
- Around line 3240-3241: Update the visitor append in the
m_snapshotUnlinkedCodeBlocks loop to pass entry.value directly as the
JSCell-derived pointer, removing the reinterpret_cast while preserving the
existing JSValue construction and unbarriered visitation.
- Around line 3137-3155: Scope HeapIterationScope only around the
forEachLiveCell walk in this startup snapshot preparation block, then let it
close before the loop that calls Structure::prepareForStartupSnapshot(). Keep
DeferGC and the property-table counting loop after the iteration scope so
materializePropertyTable() can allocate normally.
In `@Source/JavaScriptCore/heap/Heap.h`:
- Line 500: Annotate m_snapshotWrittenEver and m_snapshotRememberedThisCycle
with WTF_GUARDED_BY_LOCK(m_snapshotRememberedLock), then update
snapshotWrittenCellCount() to acquire m_snapshotRememberedLock before reading
the set size. Preserve the accessor’s existing return value while ensuring all
guarded set access follows the lock discipline.
- Around line 487-500: Guard all startup-snapshot additions with
USE(BUN_JSC_ADDITIONS): in Source/JavaScriptCore/heap/Heap.h:487-500, wrap the
listed declarations and matching snapshot members; in
Source/JavaScriptCore/heap/BlockDirectory.cpp:357-379 and its related ranges
289-291, 314-315, and 348-355, guard makeAllBlocksImmortal and immortalBits
changes, matching BlockDirectory.h; in
Source/JavaScriptCore/heap/HeapInlines.h:59-66 guard
Heap::isStartupSnapshotCell; in Source/JavaScriptCore/heap/IsoSubspace.h:57
guard abandonLowerTierPreciseFreeListForSnapshot; and in
Source/JavaScriptCore/runtime/Structure.h:785-789 guard the snapshot methods and
side lock while keeping lock() directly returning m_lock when disabled.
In `@Source/JavaScriptCore/heap/IsoSubspace.h`:
- Line 57: Reformat abandonLowerTierPreciseFreeListForSnapshot by moving its
explanatory comment above the method and placing the existing manual
SentinelLinkedList removal loop on separate lines inside the body. Retain the
loop-based cleanup rather than introducing clear().
In `@Source/JavaScriptCore/runtime/CachedTypes.cpp`:
- Line 496: Add static_assert(std::is_trivially_copyable_v<T>) inside both
CachedArray::borrow() and CachedVector::borrow(), before returning the raw
buffer, so these methods only compile for trivially copyable element types.
- Around line 648-654: Add a static_assert(std::is_trivially_copyable_v<T>)
guard at the start of the CachedTypes::borrow() template, matching the existing
CachedArray::borrow() implementation, while preserving its current empty-span
and buffer-return behavior.
In `@Source/JavaScriptCore/runtime/IntlSegmenter.cpp`:
- Around line 115-122: Update the iterator recreation logic in the relevant
IntlSegmenter method to check the ubrk_open status before assigning
m_startupSnapshotEpoch. On failure, preserve the old epoch and propagate the ICU
error before any cloneUBreakIterator call; only update the epoch after
successfully creating m_segmenter.
In `@Source/JavaScriptCore/runtime/IntlSegmenter.h`:
- Around line 71-74: Guard all Bun startup-snapshot-specific declarations,
state, implementations, and call paths with USE(BUN_JSC_ADDITIONS): in
Source/JavaScriptCore/runtime/IntlSegmenter.h lines 71-74, guard the snapshot
epoch and iterator restoration API; in
Source/JavaScriptCore/heap/PreciseAllocation.h lines 90-92 and
PreciseAllocation.cpp lines 262-277, guard immortal allocation state, accessors,
and GC behavior; and in Source/JavaScriptCore/heap/SlotVisitor.h line 211 and
SlotVisitor.cpp lines 347-378, guard snapshot root traversal and cell
visitation. Preserve the existing non-snapshot behavior when the flag is
disabled.
In `@Source/JavaScriptCore/runtime/Structure.cpp`:
- Around line 47-51: Update Structure::snapshotSideLock to use a file-scope lock
array and a named stripe-count constant, deriving the index mask from that same
constant instead of magic values. Add a concise comment documenting the >> 5
shift, the 16-byte Structure::atomSize assumption, and the resulting possible
stripe sharing.
- Around line 53-59: Update Structure::prepareForStartupSnapshot to require a
quiescent world before changing the startup-snapshot lock selector: assert
worldIsStopped() (or use an equivalent mechanism that waits for all concurrent
getConcurrently() readers), then call setIsSnapshotStructure(true). Keep the
existing property-table pinning behavior unchanged.
In `@Source/JavaScriptCore/runtime/Structure.h`:
- Line 857: Update the DEFINE_BITFIELD declaration for
isStartupSnapshotStructure to use StartupSnapshotStructure as the upper-name
argument, generating the consistent setIsStartupSnapshotStructure setter, and
update the corresponding call in Structure::prepareForStartupSnapshot.
In `@Source/JavaScriptCore/runtime/VM.cpp`:
- Around line 604-647: Guard the startup-snapshot APIs and option consistently
with USE(BUN_JSC_ADDITIONS): wrap VM::completeAllJITPlansBeforeStartupSnapshot,
VM::refreshStackBoundsAfterSnapshotRestore, and
VM::didRestoreFromStartupSnapshot in VM.cpp; guard the DateCache restore
implementation in Source/JavaScriptCore/runtime/JSDateMath.cpp (567-572) and its
declaration in Source/JavaScriptCore/runtime/JSDateMath.h (164-166); and expose
verboseStartupSnapshotFreeze in Source/JavaScriptCore/runtime/OptionsList.h
(444) only when the Bun feature is enabled.
In `@Source/JavaScriptCore/runtime/VM.h`:
- Around line 994-997: Guard the startup-snapshot lifecycle API in VM.h around
startupSnapshotEpoch(), didRestoreFromStartupSnapshot(),
refreshStackBoundsAfterSnapshotRestore(), and
completeAllJITPlansBeforeStartupSnapshot() with USE(BUN_JSC_ADDITIONS), and
apply the same guard to the matching epoch storage in VM.h at lines 1292-1292.
In Source/JavaScriptCore/runtime/IntlSegmenter.cpp lines 105-123, guard
snapshot-epoch iterator recreation with USE(BUN_JSC_ADDITIONS); no
USE(BUN_EVENT_LOOP) changes are requested.
In `@Source/WTF/wtf/FastMalloc.cpp`:
- Around line 770-774: Move the definitions of g_snapshotImmortalRangeLo and
g_snapshotImmortalRangeSpan out of FastMalloc.cpp into the snapshot-related
translation unit that restores or establishes the snapshot range, and initialize
them there during restore before concurrent readers can call
isInSnapshotImmortalRange(). Publish both values atomically or under appropriate
synchronization, preserving the declarations in RefCounted.h.
In `@Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp`:
- Around line 93-112: Track whether snapshotDeterministicHint provided a hint
through the POSIX and Darwin allocation paths in
Source/WTF/wtf/posix/OSAllocatorPOSIX.cpp at lines 93-112, 156-161, 238-243, and
267-268, and call snapshotDeterministicHintDidMap only for mappings that
consumed that hint. In the HAVE(MAP_ALIGNED) path, also update the cursor using
the successful returned mapping before subsequent hints are issued; leave
unrelated mappings from advancing s_snapshotHintNext.
In `@Source/WTF/wtf/posix/ThreadingPOSIX.cpp`:
- Around line 692-705: Update the FAST_TLS-disabled restoration logic around
s_key so threadSpecificSet(s_key, this) executes only after pthread_key_create
successfully recreates the exact s_key. Track whether restoration succeeded and
fail snapshot restoration or otherwise abort when all 512 attempts complete
without matching s_key, rather than using an unallocated key.
In `@Source/WTF/wtf/RefCounted.h`:
- Around line 30-34: Move g_snapshotImmortalRangeLo,
g_snapshotImmortalRangeSpan, and isInSnapshotImmortalRange out of RefCounted.h
into a dedicated SnapshotImmortalRange.h, then include that header in every file
using these symbols. Restore the existing RefCountedBase comment directly above
class RefCountedBase and remove the snapshot-specific declarations and comment
from RefCounted.h.
In `@Source/WTF/wtf/StackBounds.cpp`:
- Around line 98-105: Synchronize all reads and writes of the Linux-only
s_oldestEnviron in StackBounds initialization and
forgetOldestEnvironForStartupSnapshotRestore(). Use an atomic compare-exchange
or an appropriate lock so concurrent initialization and reset cannot race, while
preserving the existing null-reset behavior.
In `@Source/WTF/wtf/text/StringImpl.h`:
- Around line 331-332: Update Heap::freezeCurrentHeapAsImmortalStartupSnapshot()
to iterate over every snapshot StringImpl, call
settleLazyHeaderWritesForStartupSnapshot(), then call makeStaticForSnapshot()
before m_objectSpace.freezeAllBlocksAsImmortal(). Ensure all strings are settled
and promoted before heap freezing.
- Line 386: Update the snapshot-promotion flow around makeStaticForSnapshot()
and freezeCurrentHeapAsImmortalStartupSnapshot() so refcount operations cannot
race with setting s_refCountFlagIsStaticString. Keep the mutator suspended
through the flag promotion, or replace the separate isStatic()/RMW sequence with
a single atomic transition protocol that prevents stale ref()/deref() operations
from updating or destroying the object.
In `@Source/WTF/wtf/threads/Signals.cpp`:
- Around line 625-629: Reset handlers.useHardenedHandler to false before
initMachExceptionHandlerThread() and before re-registering the current thread in
the handlers.useMach path. Update the Mach handler initialization flow so the
flag is set to true only after task_register_hardened_exception_handler()
succeeds, preventing thread_adopt_exception_handler() from running for an
unregistered handler.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: dfc6116e-1730-4310-8d10-c88826694124
📥 Commits
Reviewing files that changed from the base of the PR and between 78d45d3 and 32883877193836085f684e324ca7b837e8880650.
📒 Files selected for processing (62)
Source/JavaScriptCore/bytecode/BytecodeRewriter.cppSource/JavaScriptCore/bytecode/ExpressionInfo.cppSource/JavaScriptCore/bytecode/ExpressionInfo.hSource/JavaScriptCore/bytecode/InstructionStream.hSource/JavaScriptCore/bytecode/UnlinkedCodeBlock.cppSource/JavaScriptCore/bytecode/UnlinkedCodeBlock.hSource/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cppSource/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.hSource/JavaScriptCore/bytecode/Watchpoint.cppSource/JavaScriptCore/bytecode/Watchpoint.hSource/JavaScriptCore/heap/BlockDirectory.cppSource/JavaScriptCore/heap/BlockDirectory.hSource/JavaScriptCore/heap/BlockDirectoryBits.hSource/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/heap/Heap.hSource/JavaScriptCore/heap/HeapInlines.hSource/JavaScriptCore/heap/IsoSubspace.hSource/JavaScriptCore/heap/JITStubRoutineSet.cppSource/JavaScriptCore/heap/LocalAllocator.hSource/JavaScriptCore/heap/MarkedBlock.cppSource/JavaScriptCore/heap/MarkedBlock.hSource/JavaScriptCore/heap/MarkedBlockSet.hSource/JavaScriptCore/heap/MarkedSpace.cppSource/JavaScriptCore/heap/MarkedSpace.hSource/JavaScriptCore/heap/PreciseAllocation.cppSource/JavaScriptCore/heap/PreciseAllocation.hSource/JavaScriptCore/heap/SlotVisitor.cppSource/JavaScriptCore/heap/SlotVisitor.hSource/JavaScriptCore/runtime/CachePayload.hSource/JavaScriptCore/runtime/CachedBytecode.hSource/JavaScriptCore/runtime/CachedTypes.cppSource/JavaScriptCore/runtime/CachedTypes.hSource/JavaScriptCore/runtime/IntlSegmenter.cppSource/JavaScriptCore/runtime/IntlSegmenter.hSource/JavaScriptCore/runtime/JSDateMath.cppSource/JavaScriptCore/runtime/JSDateMath.hSource/JavaScriptCore/runtime/OptionsList.hSource/JavaScriptCore/runtime/Structure.cppSource/JavaScriptCore/runtime/Structure.hSource/JavaScriptCore/runtime/StructureInlines.hSource/JavaScriptCore/runtime/VM.cppSource/JavaScriptCore/runtime/VM.hSource/WTF/wtf/AutomaticThread.cppSource/WTF/wtf/AutomaticThread.hSource/WTF/wtf/CryptographicallyRandomNumber.cppSource/WTF/wtf/CryptographicallyRandomNumber.hSource/WTF/wtf/FastMalloc.cppSource/WTF/wtf/OSRandomSource.cppSource/WTF/wtf/OSRandomSource.hSource/WTF/wtf/ParkingLot.cppSource/WTF/wtf/ParkingLot.hSource/WTF/wtf/RandomDevice.cppSource/WTF/wtf/RandomDevice.hSource/WTF/wtf/RefCounted.hSource/WTF/wtf/StackBounds.cppSource/WTF/wtf/StackBounds.hSource/WTF/wtf/Threading.hSource/WTF/wtf/posix/OSAllocatorPOSIX.cppSource/WTF/wtf/posix/ThreadingPOSIX.cppSource/WTF/wtf/text/StringImpl.hSource/WTF/wtf/threads/Signals.cppSource/WTF/wtf/threads/Signals.h
| // Snapshot (immortal) code blocks are immutable and never jettisoned: no lock, no aging, so their pages stay clean. | ||
| bool isSnapshot = Heap::isStartupSnapshotCell(thisObject); | ||
| std::optional<Locker<JSCellLock>> locker; | ||
| if (!isSnapshot) { | ||
| locker.emplace(thisObject->cellLock()); | ||
| if (visitor.isFirstVisit()) | ||
| thisObject->m_age = std::min<unsigned>(static_cast<unsigned>(thisObject->m_age) + 1, maxAge); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Guard the snapshot-specific visitation path with USE(BUN_JSC_ADDITIONS).
Heap::isStartupSnapshotCell and the snapshot lock/aging skip are Bun startup-snapshot features. The coding guidelines require these to be compiled behind USE(BUN_JSC_ADDITIONS) so upstream behavior is unchanged when the feature is off.
♻️ Proposed guard
+#if USE(BUN_JSC_ADDITIONS)
// Snapshot (immortal) code blocks are immutable and never jettisoned: no lock, no aging, so their pages stay clean.
bool isSnapshot = Heap::isStartupSnapshotCell(thisObject);
+#else
+ constexpr bool isSnapshot = false;
+#endif
std::optional<Locker<JSCellLock>> locker;
if (!isSnapshot) {
locker.emplace(thisObject->cellLock());
if (visitor.isFirstVisit())
thisObject->m_age = std::min<unsigned>(static_cast<unsigned>(thisObject->m_age) + 1, maxAge);
}As per coding guidelines: "Guard Bun-specific features with USE(BUN_JSC_ADDITIONS)".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Snapshot (immortal) code blocks are immutable and never jettisoned: no lock, no aging, so their pages stay clean. | |
| bool isSnapshot = Heap::isStartupSnapshotCell(thisObject); | |
| std::optional<Locker<JSCellLock>> locker; | |
| if (!isSnapshot) { | |
| locker.emplace(thisObject->cellLock()); | |
| if (visitor.isFirstVisit()) | |
| thisObject->m_age = std::min<unsigned>(static_cast<unsigned>(thisObject->m_age) + 1, maxAge); | |
| } | |
| `#if` USE(BUN_JSC_ADDITIONS) | |
| // Snapshot (immortal) code blocks are immutable and never jettisoned: no lock, no aging, so their pages stay clean. | |
| bool isSnapshot = Heap::isStartupSnapshotCell(thisObject); | |
| `#else` | |
| constexpr bool isSnapshot = false; | |
| `#endif` | |
| std::optional<Locker<JSCellLock>> locker; | |
| if (!isSnapshot) { | |
| locker.emplace(thisObject->cellLock()); | |
| if (visitor.isFirstVisit()) | |
| thisObject->m_age = std::min<unsigned>(static_cast<unsigned>(thisObject->m_age) + 1, maxAge); | |
| } |
🤖 Prompt for 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.
In `@Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp` around lines 103 - 110,
Guard the snapshot-specific logic in the visitation path with
USE(BUN_JSC_ADDITIONS), including the Heap::isStartupSnapshotCell check,
optional locker, and snapshot lock/aging skip. Preserve the existing upstream
locking and aging behavior when the feature is disabled.
Source: Coding guidelines
There was a problem hiding this comment.
Being handled as one sweep over the whole PR — see the reply on Heap.h.
There was a problem hiding this comment.
@Jarred-Sumner, acknowledged. I will treat this as part of the PR-wide USE(BUN_JSC_ADDITIONS) guard sweep.
You are interacting with an AI system.
3288387 to
578ca7b
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 25
🤖 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 `@CLAUDE.md`:
- Around line 152-155: Update the Startup Snapshots documentation to associate
freezeCurrentHeapAsImmortalStartupSnapshot and didRestoreFromStartupSnapshot
with Heap.cpp, and associate the VM-related symbol with runtime/VM.cpp according
to each symbol’s defining file. Preserve the existing startup-snapshot
description.
In `@Source/JavaScriptCore/bytecode/ExpressionInfo.cpp`:
- Around line 901-907: Update ExpressionInfo::byteSize() to return only
sizeof(ExpressionInfo) when m_borrowedPayload is set, excluding the persistent
payload allocation for instances created by createBorrowed(). Preserve the
existing payload-inclusive size calculation for owned ExpressionInfo instances.
In `@Source/JavaScriptCore/bytecode/InstructionStream.h`:
- Around line 227-231: Protect borrowed instruction streams by adding
RELEASE_ASSERT(!m_isBorrowed) at the start of
InstructionStream::didMutateBuffer(). In
Source/JavaScriptCore/bytecode/InstructionStream.h lines 227-231, this is the
direct mutation guard; in Source/JavaScriptCore/runtime/CachedTypes.cpp lines
1547-1550, verify code blocks decoded through the borrowing path never reach
BytecodeRewriter and select the copying path for any code block that
generatorification may rewrite.
- Around line 129-138: Add a debug-only freshness assertion inside
MutableRef::unwrap(), validating that the writer span from
m_buffer->mutableSpan() matches the current read handle before computing the
pointer. Keep the assertion adjacent to the existing reinterpret_cast and ensure
it is compiled out in release builds without changing the mutable access
behavior.
- Around line 200-206: Update didMutateBuffer() to assert !m_isBorrowed before
rebinding m_bytes, preventing borrowed InstructionStreamWriter instances from
replacing the cache-backed span with an empty span.
- Around line 208-219: Update both move constructor and move assignment operator
of InstructionStream to reset the moved-from object after transferring state:
clear other.m_bytes and set other.m_isBorrowed to false. Preserve the
destination’s existing buffer selection, and treat references to the moved-from
stream as invalid.
In `@Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.cpp`:
- Around line 369-370: Replace the inline rare-data size calculation in the
census logic with the existing UnlinkedCodeBlock::RareData::sizeInBytes result,
preserving the m_rareData guard and avoiding duplicate accounting of RareData
members.
- Line 120: Update the size calculation in the relevant UnlinkedCodeBlock path
to pass the already-held locker to RareData::sizeInBytes instead of
NoLockingNecessary. Preserve the existing behavior for snapshot and non-snapshot
cells while ensuring the argument accurately documents the held cell lock.
In `@Source/JavaScriptCore/bytecode/UnlinkedCodeBlock.h`:
- Line 329: Document the purpose of the conditional store in
UnlinkedCodeBlock::resetAge: explain that checking m_age before resetting it
avoids dirtying a clean snapshot page when the age is already zero. Preserve the
existing guarded assignment.
- Around line 175-176: Update the ComponentSizes struct declaration to
default-initialize all eight size_t members to zero, ensuring both
value-initialized and default-initialized instances start with deterministic
component sizes. Keep componentSizesForCensus unchanged.
In `@Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cpp`:
- Around line 250-271: Narrow the DeferGC scope in the function containing
generateUnlinkedFunctionCodeBlock so it covers only the operations that require
deferred collection, while retaining GC safety through
vm.heap.setSnapshotUnlinkedCodeBlockFor. Ensure any generated or decoded
UnlinkedFunctionCodeBlock is strongly rooted across allocating parsing,
code-block creation, and cache-update operations, or keep deferral active until
the snapshot entry is inserted.
- Around line 314-324: Bind m_cachedRecordOffset to the CachedBytecode instance
or generation from which it was produced, preserving that identity across
decodeCachedCodeBlocks(). In the re-decoding path around
ensureBytecodeCacheDecoder and decodeFunctionCodeBlockFromExecutableRecord,
reject the operation when the provider’s current cachedBytecode does not match
the stored identity, rather than relying only on the size check.
In `@Source/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.h`:
- Around line 345-346: Preserve the 96-byte size of UnlinkedFunctionExecutable
while retaining the cached-record offset state. Adjust the member layout or
representation around m_cachedRecordOffset and the first union so the value fits
existing padding or otherwise avoids increasing the object size; verify the
resulting allocation remains 96 bytes.
In `@Source/JavaScriptCore/bytecode/Watchpoint.cpp`:
- Around line 149-152: Update the WatchpointSet destructor after draining the
side-chain list to remove this set’s entry from snapshotSideChains() while
holding s_snapshotSideChainsLock. Add a removeSnapshotSideChain() helper
alongside snapshotSideChain(), guard it with isSnapshotWatchpointSet(this), and
invoke it after the existing drain completes.
In `@Source/JavaScriptCore/heap/Heap.h`:
- Around line 490-493: Protect m_snapshotUnlinkedCodeBlocks in
snapshotUnlinkedCodeBlockFor, setSnapshotUnlinkedCodeBlockFor, and
clearSnapshotUnlinkedCodeBlocks with m_snapshotRememberedLock, and mark the
member accordingly. In the “Img” constraint, copy the map’s values while holding
that lock, then release it before iterating or visiting the copied entries,
matching the existing m_snapshotWrittenEver handling.
In `@Source/JavaScriptCore/runtime/CachedTypes.cpp`:
- Around line 1092-1093: Update the decoder branch using CachedArray::borrow()
so it only calls ExpressionInfo::createBorrowed when the borrowed payload is
non-null. When borrow() returns nullptr for an empty payload, fall through to
the existing non-borrowed creation path, avoiding a borrowed header-only
ExpressionInfo whose payload() interprets unallocated inline storage.
- Around line 2516-2519: In the cached branch of the surrounding constructor or
initialization logic, placement-construct the inactive union member m_decoder
with decoder instead of assigning through m_decoder. In the non-cached branch,
remove the m_decoder = nullptr write, while preserving the existing
m_cachedRecordOffset assignment only for m_isCached records.
In `@Source/JavaScriptCore/runtime/CachedTypes.h`:
- Around line 81-83: Move the canBorrowPayload() declaration into the existing
public query-method section of Decoder, placing it alongside size() rather than
adding another public/private access-specifier switch. Remove the redundant
access labels introduced around WTF_MAKE_NONCOPYABLE(Decoder), while preserving
the method’s public visibility.
In `@Source/JavaScriptCore/runtime/CachePayload.h`:
- Around line 41-43: Update CachePayload::isPersistent() to rely only on the
explicit m_isPersistent flag set by setIsPersistent(), removing automatic
persistence classification for FileSystem::MappedFileData; do not alter the
mapped-data variant itself or introduce retention changes.
In `@Source/JavaScriptCore/runtime/JSDateMath.h`:
- Around line 164-165: Guard the Bun-specific startup-snapshot contract with
USE(BUN_JSC_ADDITIONS) across all affected sites:
Source/JavaScriptCore/runtime/JSDateMath.h:164-165 and JSDateMath.cpp:567-572
for DateCache::didRestoreFromStartupSnapshot(),
Source/JavaScriptCore/runtime/IntlSegmenter.h:71-74 for breakIterator() and
m_startupSnapshotEpoch while keeping m_segmenter unconditional, and
IntlSegmenter.cpp:105-125 for epoch tracking/iterator recreation. At
IntlSegmenter.cpp:145-148, retain the direct m_segmenter clone path when
disabled, and guard the matching VM call path; preserve existing non-snapshot
behavior.
In `@Source/JavaScriptCore/runtime/OptionsList.h`:
- Line 444: Wrap the Bun-specific options, including
verboseStartupSnapshotFreeze and the entries around the referenced lines, in the
established USE(BUN_JSC_ADDITIONS) feature guard, and guard every corresponding
reference consistently so shared JavaScriptCore builds do not expose them
without Bun additions.
In `@Source/JavaScriptCore/runtime/Structure.h`:
- Around line 785-789: Guard all Bun startup-snapshot declarations, state, and
dependent behavior with USE(BUN_JSC_ADDITIONS), preserving existing non-Bun APIs
and behavior: Source/JavaScriptCore/runtime/Structure.h:785-789 guard the
snapshot lock accessor and APIs; Structure.cpp:47-63 guard side-lock storage and
preparation; MarkedSpace.h:130-132 and MarkedSpace.cpp:403-412 guard
immortal-block support; PreciseAllocation.h:90-92 and
PreciseAllocation.cpp:262-277 guard immortal allocation semantics;
SlotVisitor.h:211 and SlotVisitor.cpp:347-378 guard immortal-root traversal; and
StructureInlines.h:105-123 guard dynamic snapshot side-lock usage.
In `@Source/JavaScriptCore/runtime/VM.cpp`:
- Around line 604-647: Validate the startup snapshot lifecycle changes around
VM::completeAllJITPlansBeforeStartupSnapshot and
VM::didRestoreFromStartupSnapshot using a debug build with
ENABLE_SANITIZERS=address enabled; configure with Ninja and build the jsc target
to complete the required memory-debug validation.
In `@Source/WTF/wtf/CryptographicallyRandomNumber.h`:
- Around line 46-47: Rename
restirCryptographicallyRandomNumberGeneratorForSnapshotRestore to
resetCryptographicallyRandomNumberGeneratorForSnapshotRestore, updating its
definition and every call site while preserving the existing snapshot-restore
behavior.
In `@Source/WTF/wtf/OSRandomSource.cpp`:
- Around line 45-47: Track whether the shared random device was initialized
before restore, and only call reopenForSnapshotRestore() in
reopenOSRandomSourceForSnapshotRestore() when it already existed; a newly
constructed RandomDevice should retain the descriptor opened by
RandomDevice::RandomDevice(). Update the singleton state handling in
Source/WTF/wtf/OSRandomSource.cpp (lines 45-47) and the related initialization
behavior in Source/WTF/wtf/RandomDevice.cpp (lines 67-74) as needed.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aacb3e91-9b74-456b-a1aa-f75b6983562b
📥 Commits
Reviewing files that changed from the base of the PR and between 78d45d3 and 578ca7b9c8c8f273c06c4d5a8236eff7b73cf4e3.
📒 Files selected for processing (61)
CLAUDE.mdSource/JavaScriptCore/bytecode/BytecodeRewriter.cppSource/JavaScriptCore/bytecode/ExpressionInfo.cppSource/JavaScriptCore/bytecode/ExpressionInfo.hSource/JavaScriptCore/bytecode/InstructionStream.hSource/JavaScriptCore/bytecode/UnlinkedCodeBlock.cppSource/JavaScriptCore/bytecode/UnlinkedCodeBlock.hSource/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.cppSource/JavaScriptCore/bytecode/UnlinkedFunctionExecutable.hSource/JavaScriptCore/bytecode/Watchpoint.cppSource/JavaScriptCore/bytecode/Watchpoint.hSource/JavaScriptCore/heap/BlockDirectory.cppSource/JavaScriptCore/heap/BlockDirectory.hSource/JavaScriptCore/heap/BlockDirectoryBits.hSource/JavaScriptCore/heap/Heap.cppSource/JavaScriptCore/heap/Heap.hSource/JavaScriptCore/heap/HeapInlines.hSource/JavaScriptCore/heap/IsoSubspace.hSource/JavaScriptCore/heap/JITStubRoutineSet.cppSource/JavaScriptCore/heap/LocalAllocator.hSource/JavaScriptCore/heap/MarkedBlock.cppSource/JavaScriptCore/heap/MarkedBlock.hSource/JavaScriptCore/heap/MarkedBlockSet.hSource/JavaScriptCore/heap/MarkedSpace.cppSource/JavaScriptCore/heap/MarkedSpace.hSource/JavaScriptCore/heap/PreciseAllocation.cppSource/JavaScriptCore/heap/PreciseAllocation.hSource/JavaScriptCore/heap/SlotVisitor.cppSource/JavaScriptCore/heap/SlotVisitor.hSource/JavaScriptCore/runtime/CachePayload.hSource/JavaScriptCore/runtime/CachedBytecode.hSource/JavaScriptCore/runtime/CachedTypes.cppSource/JavaScriptCore/runtime/CachedTypes.hSource/JavaScriptCore/runtime/IntlSegmenter.cppSource/JavaScriptCore/runtime/IntlSegmenter.hSource/JavaScriptCore/runtime/JSDateMath.cppSource/JavaScriptCore/runtime/JSDateMath.hSource/JavaScriptCore/runtime/OptionsList.hSource/JavaScriptCore/runtime/Structure.cppSource/JavaScriptCore/runtime/Structure.hSource/JavaScriptCore/runtime/StructureInlines.hSource/JavaScriptCore/runtime/VM.cppSource/JavaScriptCore/runtime/VM.hSource/WTF/wtf/AutomaticThread.cppSource/WTF/wtf/AutomaticThread.hSource/WTF/wtf/CryptographicallyRandomNumber.cppSource/WTF/wtf/CryptographicallyRandomNumber.hSource/WTF/wtf/OSRandomSource.cppSource/WTF/wtf/OSRandomSource.hSource/WTF/wtf/ParkingLot.cppSource/WTF/wtf/ParkingLot.hSource/WTF/wtf/RandomDevice.cppSource/WTF/wtf/RandomDevice.hSource/WTF/wtf/StackBounds.cppSource/WTF/wtf/StackBounds.hSource/WTF/wtf/Threading.hSource/WTF/wtf/posix/OSAllocatorPOSIX.cppSource/WTF/wtf/posix/ThreadingPOSIX.cppSource/WTF/wtf/text/StringImpl.hSource/WTF/wtf/threads/Signals.cppSource/WTF/wtf/threads/Signals.h
| // snapshot restore: the generator's stream state came from the building process; every restored process would otherwise emit the same bytes. | ||
| WTF_EXPORT_PRIVATE void restirCryptographicallyRandomNumberGeneratorForSnapshotRestore(); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rename the snapshot RNG reset API.
restirCryptographicallyRandomNumberGeneratorForSnapshotRestore does not describe the reset operation in its comment. Rename it to resetCryptographicallyRandomNumberGeneratorForSnapshotRestore and update its definition and call sites.
🤖 Prompt for 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.
In `@Source/WTF/wtf/CryptographicallyRandomNumber.h` around lines 46 - 47, Rename
restirCryptographicallyRandomNumberGeneratorForSnapshotRestore to
resetCryptographicallyRandomNumberGeneratorForSnapshotRestore, updating its
definition and every call site while preserving the existing snapshot-restore
behavior.
There was a problem hiding this comment.
Kept the name — it says what happens: the generator is made to stir itself again (its own vocabulary; see forceStirOnNextUse), not reset to a fixed state. The comment above the declaration now says exactly that, which is what was missing.
There was a problem hiding this comment.
Rate Limit Exceeded
@Jarred-Sumner have exceeded the limit for the number of chat messages per hour. Please wait 8 minutes and 41 seconds before sending another message.
578ca7b to
909260d
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
ae37959 to
7266644
Compare
7266644 to
a5c9af5
Compare
a5c9af5 to
f49f4bc
Compare
f49f4bc to
b069341
Compare
b069341 to
d01bea4
Compare
d01bea4 to
92ee5e5
Compare
| if (isImmortal()) [[unlikely]] { | ||
| // Snapshot memory is never handed back to the allocator, so it cannot be realloc'd: copy header and cell into a fresh | ||
| // allocation, laid out exactly as realloc would have left them, and abandon the old one where it is. The copy is an | ||
| // ordinary allocation of this process (the flag is cleared at the end). | ||
| newSpace = subspace->alignedMemoryAllocator()->tryAllocateMemory(adjustedAlignmentAllocationSize); | ||
| if (!newSpace) | ||
| return nullptr; | ||
| WTF_ALLOW_UNSAFE_BUFFER_USAGE_BEGIN | ||
| memcpy(newSpace, oldBasePointer, oldAdjustment + PreciseAllocation::headerSize() + oldCellSize); // byte-for-byte what realloc would have produced; the adjustment logic below then applies unchanged | ||
| WTF_ALLOW_UNSAFE_BUFFER_USAGE_END | ||
| } else | ||
| #endif | ||
| { | ||
| newSpace = subspace->alignedMemoryAllocator()->tryReallocateMemory(oldBasePointer, adjustedAlignmentAllocationSize); |
There was a problem hiding this comment.
🟡 The 92ee5e5b fix (allocate-fresh + memcpy + clear m_isImmortal) resolves the leak, but because it landed inside tryReallocate() rather than in canRealloc, the caller CompleteSubspace::reallocatePreciseAllocationNonVirtual still runs its bookkeeping against the immortal allocation: (1) oldAllocation->remove() at :177 splices the immortal BasicRawSentinelNode — writing prev/next into the immortal header and its immortal neighbours; (2) m_preciseAllocations[oldIndexInSpace] = allocation at :195 replaces the immortal's stable-prefix slot with the mortal copy, so when that mortal later dies sweepPreciseAllocations() compaction shifts every higher-indexed immortal and the compare-before-write at MarkedSpace.cpp:268/348 fires into their headers. Page-cleanliness only (Auxiliary butterflies aren't in m_snapshotPreciseRoots; the new allocation is correctly mortal). Adding && !preciseAllocation().isImmortal() to canRealloc — the originally suggested fix — would bypass the whole caller path; alternatively teach the caller to skip remove() and append at the end when oldAllocation->isImmortal().
Extended reasoning...
What the bug is
The original comment on this file flagged that growing a snapshot large butterfly would (a) hand snapshot memory to realloc, (b) propagate m_isImmortal into the fresh allocation causing a leak, (c) remove() the immortal from the intrusive list, and (d) break the stable-prefix invariant. The 92ee5e5b fix addressed (a) and (b) by having PreciseAllocation::tryReallocate() allocate fresh memory, memcpy header+cell into it, and unconditionally clear m_isImmortal on the result — and the maintainer's reply says the old allocation is "abandoned in place like everything else in the snapshot". But the fix landed inside tryReallocate(), so the caller CompleteSubspace::reallocatePreciseAllocationNonVirtual (CompleteSubspace.cpp:148-201, unchanged in this PR) still runs its own bookkeeping against the immortal, and (c) and (d) remain.
The specific code path
Butterfly::reallocArrayRightIfPossible (ButterflyInlines.h:208) computes canRealloc = !propertyCapacity && !vm.heap.mutatorShouldBeFenced() && isPreciseAllocation() — no isImmortal() check — and calls reallocatePreciseAllocationNonVirtual. That function, before calling tryReallocate:
- Line 176-177:
if (oldAllocation->isOnList()) oldAllocation->remove(). Immortal precise allocations are on the subspace'sSentinelLinkedList(nothing removes them at freeze), so this runs.BasicRawSentinelNode::remove()writesprev/nextinthis— the immortal header — and in both neighbours, which since every precise allocation live at freeze was made immortal, are also immortal. So the old allocation is not abandoned in place; it's spliced out with three header writes into snapshot pages. The fix intryReallocate()is downstream of this and cannot prevent it.
Then, after tryReallocate returns the fresh mortal allocation:
- Line 195:
m_space.m_preciseAllocations[oldIndexInSpace] = allocationoverwrites the immortal's vector slot with the new mortal allocation.
Why the compare-before-write guards no longer suffice
The maintainer's earlier reply on MarkedSpace.cpp:349 ("snapshot allocations form a stable prefix of the vector … so their index never actually changes") is the invariant the compare-before-write guards at MarkedSpace.cpp:268 and :348 rely on. Line 195 breaks it: a mortal allocation now sits at index i inside the immortal prefix. When that mortal later becomes garbage, sweepPreciseAllocations() compacts the vector and every immortal at index > i shifts down by one, so allocation->indexInSpace() != dstIndex and the guard fires setIndexInSpace() into each of their headers — the exact write those guards were added to prevent.
Step-by-step proof
- Warm-up creates a large array whose butterfly exceeds
MarkedSpace::largeCutoff(~8 KB) — aPreciseAllocationinauxiliarySpace. freezeCurrentHeapAsImmortalStartupSnapshot()runs; every livePreciseAllocationgetsmakeImmortal(). The butterfly's allocation sits at indexiinm_preciseAllocations, onauxiliarySpace's intrusive list between two other immortal allocations, all in snapshot pages.- Restored process pushes past capacity.
canReallocis true (no property storage, mutator not fenced, precise allocation).reallocatePreciseAllocationNonVirtualis called. - Line 177:
oldAllocation->remove()writesm_prev/m_nextin the immortal header at indexiand in its two immortal neighbours. Three snapshot header pages dirtied. tryReallocate()(with the 92ee5e5b fix) allocates fresh memory, memcpy's header+cell, clearsm_isImmortal. Correct so far.- Line 195:
m_preciseAllocations[i] = newAllocation(mortal). Line 199 also appends it to the subspace's intrusive list. - Later the array grows again but this time acquires property storage, so
!propertyCapacityis false and the allocate-fresh-butterfly path is taken; the mortal at indexibecomes unreachable. - Next
sweepPreciseAllocations(): the mortal atiis empty and destroyed; compaction moves every entry ati+1..N-1(all immortal, all part of the frozen prefix) down by one. For each,indexInSpace() != dstIndex, sosetIndexInSpace(dstIndex)writes into its header. Every remaining immortal precise-allocation header page dirtied.
Impact
Not a correctness bug. The abandoned immortal butterfly is HeapCell::Auxiliary, so it was never added to m_snapshotPreciseRoots (only isJSCellKind allocations are), and the new allocation is correctly mortal, marked, swept, and collected. No leak, no crash, no wrong answers. The only effect is that the clean-page goal — the whole point of the immortal machinery — is defeated for every immortal precise-allocation header once any snapshot large-array is grown after restore. Narrow trigger (>8 KB array frozen in the snapshot, then pushed past capacity), but when it fires the damage is broad.
How to fix
Either of:
- Add
&& !std::bit_cast<HeapCell*>(theBase)->preciseAllocation().isImmortal()tocanReallocin ButterflyInlines.h:208, so the wholereallocatePreciseAllocationNonVirtualpath is bypassed andtryAllocateNewButterfly(allocate-fresh + memcpy in the butterfly layer) is used instead. This was the originally suggested fix and makes the immortal branch intryReallocate()unreachable (it can stay as belt-and-suspenders). - Or, teach
reallocatePreciseAllocationNonVirtualto checkoldAllocation->isImmortal(): skip theremove(), and instead of overwritingm_preciseAllocations[oldIndexInSpace], append the fresh allocation at the end of the vector with a fresh index (the immortal stays in its slot forever;isMarked()is always true so sweep never removes it).
There was a problem hiding this comment.
You were right that the earlier fix was in the wrong place, and the fix you originally suggested is simply better: in 78136ea canRealloc is false for an immortal allocation, so a snapshot butterfly grows through the ordinary allocate-and-copy path and the snapshot allocation is never touched — no header write, no list splice, and the stable prefix stays intact — and the special case in tryReallocate is gone in favor of an assertion. Less code and zero writes; the grow-after-restore test passes through the new route.
92ee5e5 to
78136ea
Compare
There was a problem hiding this comment.
I reviewed the latest revision (78136ea) and the bug-hunting pass found no new issues — all prior inline findings on this PR have been addressed. Given the scope (63 files across GC/heap internals, Structure locking, bytecode-cache decoding, and WTF signal/thread/RNG restore paths), a human sign-off is still worthwhile before merge.
What was reviewed:
- The 78136ea follow-ups: registry-lock ordering in
forgetUnderlyingThreadsForSnapshotRestore, compare-before-write inWeakSet::resetAllocator. - Candidate race on
s_sharedRandomDevicein OSRandomSource.cpp — restore runs single-threaded, ruled out. - The immortal-butterfly realloc gate in ButterflyInlines.h and the
SetForScopeleaf-recording suppression now applied to both re-decode entry points.
Extended reasoning...
Overview
This PR adds startup-snapshot support to JSC/WTF across four commits: (1) an immortal-snapshot heap where every MarkedBlock and PreciseAllocation is frozen after a full GC and never marked into, swept, or allocated from again, with side tables replacing writes into snapshot cells (barrier remembered set, watchpoint side chains, snapshot-executable code-block map, striped Structure side locks); (2) borrowed instruction streams / expression info that alias a persistent bytecode-cache mapping instead of copying, plus a per-VM shared Decoder registry and a re-decode path for jettisoned cache-generated code blocks; (3) VM::didRestoreFromStartupSnapshot() which recomputes stack bounds, forgets AutomaticThread/ParkingLot state, re-stirs the RNG and reopens /dev/urandom, drops ICU/date caches, re-homes hash tables out of snapshot pages, resets GC pacing, and re-installs kernel-side signal/mach-exception handlers; and (4) deterministic mmap placement hints keyed on MIMALLOC_DETERMINISTIC_HINT. It touches 63 files (+998/−158) spanning heap/, bytecode/, runtime/, and wtf/.
Security risks
Several security-adjacent surfaces are modified. SignalHandlers::reinstallAfterSnapshotRestore() clears g_wtfConfig.isPermanentlyFrozen and g_gigacageConfig.isPermanentlyFrozen to rewrite handler state, with the caller re-freezing afterwards — a deliberate but sensitive window. The ARM64E hardened-mach-exception path is bypassed on restore (falls back to the classic handler) and is explicitly noted by the author as reviewed-by-reading-only, not exercised on hardware. restirCryptographicallyRandomNumberGeneratorForSnapshotRestore() correctly forces a re-stir from the OS and reopens the urandom fd, which is the right call; without it every restored process would emit the same random bytes. StringImpl::ref()/deref() now unconditionally short-circuit on isStatic(), which is a hot-path behaviour change (previously TSAN-only). The snapshot-Structure side-lock striping means two distinct snapshot Structures can share a lock — contention only, but a change to the concurrency model the JIT compilers rely on. None of these look wrong, but each is the kind of change that benefits from a second pair of eyes.
Level of scrutiny
High. This is a large architectural addition to the garbage collector and its interaction with the concurrent JIT — new invariants (immortal blocks whose marks are permanent liveness, snapshot cells whose headers must never be written, a side remembered set that stands in for cellState transitions) thread through SlotVisitor, MarkedBlock, BlockDirectory, PreciseAllocation, Heap::addToRememberedSet, and Structure. The review history on this PR bears that out: over the past ~12 hours multiple correctness-class issues were found and fixed (immortal precise-allocation realloc propagating the immortal bit, m_leafExecutables growth via the shared decoder, a goto-over-initialization compile error on ARM64E, an AB/BA lock-order inversion, snapshot executables being written by clearCode()). All of those are now resolved, and this pass found nothing new — but the density of subtle findings so far argues for a human read of the final state rather than auto-approval.
Other factors
Most snapshot-specific logic is guarded by USE(BUN_JSC_ADDITIONS) and/or hasImmortalBlocks() / per-object bits, so the no-snapshot cost is well-contained (the PR description quantifies it and reports JetStream2 parity and identical JSTests/stress pass/fail sets). A few changes are unconditional across all builds: the isStatic() early-return in StringImpl::ref()/deref(), the extra Immortal word in BlockDirectoryBits, the compare-before-write in WeakSet::resetAllocator and InlineWatchpointSet::fireAll, the InstructionStream span-based ref refactor, and the CachedPtr::decode/decodeShared split. Tests exist on the Bun side (test/js/bun/startup-snapshot) rather than in this repo. Given the scope, the security-adjacent surfaces, and the design-level nature of the immortal-heap invariants, this falls squarely outside the auto-approval bar.
78136ea to
8e6b087
Compare
| UCollator* IntlCollator::collatorForThisProcess(JSGlobalObject* globalObject) const | ||
| { | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| VM& vm = globalObject->vm(); | ||
| if (m_startupSnapshotEpoch != vm.startupSnapshotEpoch()) [[unlikely]] { | ||
| (void)m_collator.release(); // the handle belongs to the process that built the snapshot; its ICU is gone | ||
| const_cast<IntlCollator*>(this)->openCollator(globalObject, m_ignorePunctuation ? TriState::True : TriState::False); | ||
| m_startupSnapshotEpoch = vm.startupSnapshotEpoch(); | ||
| } | ||
| #else | ||
| UNUSED_PARAM(globalObject); | ||
| #endif | ||
| return m_collator.get(); | ||
| } |
There was a problem hiding this comment.
🟡 Two small gaps in collatorForThisProcess()'s epoch-mismatch branch vs. the sibling revalidators added in the same commit: (1) m_canDoASCIIUCADUCETComparison is not reset to Indeterminate, so the reopened collator inherits the build-process ICU's cached fast-path verdict (derived from ucol_getRules()/UCOL_FRENCH_COLLATION on the old handle); (2) m_startupSnapshotEpoch is set unconditionally after openCollator(), whereas IntlSegmenter/IntlRelativeTimeFormat/IntlDateTimeFormat all guard the epoch update on success — and compareStrings() declares its throw scope after this call with no RETURN_IF_EXCEPTION, so the failure path passes null to ucol_strcoll*. Both are one-line fixes: reset the tri-state alongside m_collator.release(), and gate the epoch store on if (m_collator).
Extended reasoning...
What the two gaps are
IntlCollator::collatorForThisProcess() (IntlCollator.cpp:296-309) is the epoch-revalidating accessor added in 8e6b087d alongside IntlSegmenter::breakIterator(), IntlRelativeTimeFormat::ensureICUObjectsForThisProcess() and IntlDateTimeFormat::ensureICUObjectsForThisProcess(). Its epoch-mismatch branch differs from all three siblings in two ways:
(1) Stale ASCII fast-path cache. m_canDoASCIIUCADUCETComparison (IntlCollator.h:116, mutable TriState default Indeterminate) is computed once by updateCanDoASCIIUCADUCETComparison() (:440-464), whose final check canDoASCIIUCADUCETComparisonWithUCollator(*m_collator) (:413-438) reads two pieces of ICU-data-dependent state from the live handle: ucol_getAttribute(UCOL_FRENCH_COLLATION) — never explicitly set by openCollator(), so it's the locale default from CLDR — and ucol_getRules() tailoring length. Once set to True/False it is never re-evaluated (canDoASCIIUCADUCETComparison() at h:72-77 only calls the update when Indeterminate). The epoch-mismatch branch releases and reopens m_collator but leaves this tri-state as it was. The other resolved-field checks (m_usage/m_collation/m_sensitivity/m_caseFirst/m_numeric/m_ignorePunctuation) are stored in the cell and stable across restore, and the explicitly-set attributes (STRENGTH/CASE_LEVEL/CASE_FIRST/NUMERIC/ALTERNATE_HANDLING) are re-applied by openCollator() from those same fields — so the only input that can drift is the ICU probe.
(2) Unguarded epoch update. openCollator() (:236-244) does m_collator = …(ucol_open(m_icuLocale.data(), &status)); if (U_FAILURE(status)) { throwTypeError(...); return; } — so on ICU failure it throws and leaves m_collator null. collatorForThisProcess() at :303 then unconditionally sets m_startupSnapshotEpoch = vm.startupSnapshotEpoch(). All three sibling revalidators guard the epoch update on success: IntlSegmenter does if (m_segmenter) m_startupSnapshotEpoch = …, IntlRelativeTimeFormat does if (self->m_relativeDateTimeFormatter) …, IntlDateTimeFormat does if (U_SUCCESS(status)) … — so a transient ICU failure retries next call instead of latching a null handle. The caller compareStrings() (:312) declares its throw scope at :318 after the collatorForThisProcess() call at :314 and has only ASSERT(collator) between, so on the failure path debug hits the assert and release passes nullptr to ucol_strcollUTF8 (:335) / ucol_strcoll (:342).
Why it matters (and why it's narrow)
The rationale for reopening the handle at all is the one the maintainer confirmed in the IntlSegmenter thread: on macOS, ICU is the system's libicucore, whose CLDR data version is not the build machine's, so per-process ICU state cannot be carried in a snapshot. That is exactly the environment in which ucol_getRules() length and the UCOL_FRENCH_COLLATION locale default can differ between the build-process ICU and the restored-process ICU for the same m_icuLocale. On Linux, where ICU is static and its allocations are captured, the two are identical and gap (1) is unobservable. Gap (2) requires ucol_open to fail on a locale that succeeded during warm-up — extremely unlikely; the fixture the maintainer added exercises this path successfully.
Step-by-step proof (gap 1)
- Build process warm-up runs
const c = new Intl.Collator('en'); c.compare('a', 'b').compareStrings()at :322 callscanDoASCIIUCADUCETComparison(), which findsIndeterminate, callsupdateCanDoASCIIUCADUCETComparison(), which reachescanDoASCIIUCADUCETComparisonWithUCollator(*m_collator)at :460. Against the build machine's ICU,ucol_getRules(&collator, &length)returnslength == 0(root collation, no tailoring) andUCOL_FRENCH_COLLATIONdefaults toUCOL_OFF, so the tri-state is cached asTriState::True. freezeCurrentHeapAsImmortalStartupSnapshot()runs; theIntlCollatorcell is snapshot-immortal withm_canDoASCIIUCADUCETComparison == Trueandm_startupSnapshotEpoch == 0.- Restored process (macOS, system libicucore of a different CLDR version).
didRestoreFromStartupSnapshot()bumpsvm.startupSnapshotEpoch()to 1. On the nextc.compare(...),collatorForThisProcess()sees the epoch mismatch, does(void)m_collator.release(), callsopenCollator()which opens a freshUCollatoragainst the system ICU, and setsm_startupSnapshotEpoch = 1.m_canDoASCIIUCADUCETComparisonis stillTrue. - Suppose the system ICU's CLDR data has non-empty tailoring rules for this locale (or a different
UCOL_FRENCH_COLLATIONdefault) such thatcanDoASCIIUCADUCETComparisonWithUCollator()would returnfalsefor the reopened handle.compareStrings()at :322 sees the cachedTrueand, for ASCII inputs, takescompareASCIIWithUCADUCET()— the hardcoded DUCET table — while non-ASCII inputs go throughucol_strcoll()on the reopened handle. ASCII and non-ASCII comparisons on the same collator can now disagree.
Step-by-step proof (gap 2)
- Same setup through step 3, except
ucol_open(m_icuLocale.data(), &status)fails insideopenCollator()(:240-244): a TypeError is thrown into the VM andm_collatoris left null. Control returns tocollatorForThisProcess(). - Line :303 unconditionally sets
m_startupSnapshotEpoch = vm.startupSnapshotEpoch(), and :308 returnsnullptr. - Back in
compareStrings()(:314),collator == nullptr. The throw scope is declared at :318, after the call, so there is noRETURN_IF_EXCEPTIONbetween :314 and the uses. Debug hitsASSERT(collator)at :315; release proceeds to :335 or :342 and passesnullptrtoucol_strcollUTF8/ucol_strcoll. Because the epoch was already latched at step 2, subsequent calls do not retry the reopen either.
How to fix
Two one-line changes inside the if (m_startupSnapshotEpoch != vm.startupSnapshotEpoch()) branch, matching the sibling pattern:
if (m_startupSnapshotEpoch != vm.startupSnapshotEpoch()) [[unlikely]] {
(void)m_collator.release();
m_canDoASCIIUCADUCETComparison = TriState::Indeterminate; // re-probe against the reopened handle
const_cast<IntlCollator*>(this)->openCollator(globalObject, m_ignorePunctuation ? TriState::True : TriState::False);
if (m_collator)
m_startupSnapshotEpoch = vm.startupSnapshotEpoch();
}The comment at :314 ("before the ASCII probe below, which uses the handle") shows the author already considered the Indeterminate → compute case (where updateCanDoASCIIUCADUCETComparison dereferences *m_collator); resetting to Indeterminate here just extends that to the already-cached case.
There was a problem hiding this comment.
Both taken, applied locally and going out with the next push (a preview build is running and I'm not interrupting those any more): the verdict is reset alongside the release — it really is a statement about the old ICU's rules, and the restoring machine's system ICU can differ — the epoch store is gated on the handle like the siblings, and compareStrings returns a neutral result if the reopen threw so the caller's exception check reports it instead of ICU seeing a null.
There was a problem hiding this comment.
Update: 9c8a7dc5 supersedes what I described for the failure path. The embedder's ASAN lanes flagged the refactored initializers under exception-check validation (the opener can throw and the tail didn't hand off), which also applies to every revalidation entry point, so the shape is now: initializers release their scope before the tail call, and the revalidators never throw — they observe and clear the opener's exception and fail deterministically with a message, since an object that opened before and can't reopen means the ICU underneath changed, which nothing above can recover from. The verdict reset and the epoch gating from your comment are in as well.
| // m_decoder is the build process's, frozen with this cell: only read from it. The decode goes through this | ||
| // process's own decoder for the same payload, and records nothing (this code was recorded when first decoded). | ||
| Ref decoder = vm.ensureBytecodeCacheDecoder(Ref { m_decoder->cachedBytecode() }, m_decoder->provider()); | ||
| decodeFunctionCodeBlockForReDecode(decoder.get(), offset, slot, this); | ||
| } |
There was a problem hiding this comment.
🟡 The comment says "the decode goes through this process's own decoder for the same payload", but m_bytecodeCacheDecoderMap is never cleared in didRestoreFromStartupSnapshot()/evacuateTablesForStartupSnapshot(), so ensureBytecodeCacheDecoder() finds the build-process entry (kept alive by the immortal executable's m_decoder) and hands back the frozen Decoder itself. decodeFunctionCodeBlockForReDecode then SetForScope-writes m_recordsLeafExecutables and grows m_offsetToPtrMap/m_finalizers in the frozen object — page-dirtying + bounded accretion in a decoder that never destructs, defeating the 92ee5e5b "frozen decoder is only read" intent. Clearing (or rehomeOutOfSnapshotPages) m_bytecodeCacheDecoderMap in didRestoreFromStartupSnapshot() makes the first post-restore lookup create a fresh Decoder as the comment describes.
Extended reasoning...
What the bug is
The 92ee5e5b fix reworked the snapshot m_isCached branch so that instead of decoding through the immortal cell's frozen m_decoder, it calls vm.ensureBytecodeCacheDecoder(Ref{m_decoder->cachedBytecode()}, m_decoder->provider()) — with the maintainer's stated intent (in the timeline reply for 92ee5e5b) that "the frozen decoder is now only read: the branch obtains this process's own shared decoder for the same payload", and the in-code comment here says the same. But VM::m_bytecodeCacheDecoderMap is never cleared or rehomed on restore: neither VM::didRestoreFromStartupSnapshot() (VM.cpp) nor Heap::evacuateTablesForStartupSnapshot() (Heap.cpp) touches it. So the map still contains the build process's {snapshot CachedBytecode* → frozen Decoder*} entry, and ensureBytecodeCacheDecoder() returns the frozen Decoder itself rather than a fresh one.
The specific code path
- Warm-up (build process).
decodeCodeBlockImpl()(CachedTypes.cpp:2798) callsvm.ensureBytecodeCacheDecoder(...), which creates a Decoder, stores{cachedBytecode.ptr() → decoder.ptr()}inm_bytecodeCacheDecoderMap(VM.cpp:1137-1143), and callssetIsRegisteredWithVM(). A nestedUnlinkedFunctionExecutablewith at least one cached code block setsm_isCached = trueandm_decoder = &decoder(CachedTypes.cpp:2520-2521). The function is not called during warm-up, som_isCachedstays true. - Freeze.
freezeCurrentHeapAsImmortalStartupSnapshot()runs. The executable is now in an immortal block; its destructor never runs, so itsm_decoderRefPtr keeps the frozen Decoder's refcount ≥ 1 forever.~Decoder()therefore never runs, andunregisterBytecodeCacheDecoder()is never called: the map entry survives. - Restore.
didRestoreFromStartupSnapshot()runs —m_bytecodeCacheDecoderMapis not among the tables it clears or rehomes. - Post-restore call.
unlinkedCodeBlockFor()on the immortal executable takes the snapshot branch, seesm_isCached, and reaches this line:vm.ensureBytecodeCacheDecoder(Ref{m_decoder->cachedBytecode()}, ...). InensureBytecodeCacheDecoder,m_bytecodeCacheDecoderMap.add(cachedBytecode.ptr(), nullptr)finds the existing key (same frozenCachedBytecode*), so!addResult.isNewEntryand the function returns*addResult.iterator->value— the frozen Decoder, wrapped in a freshRef(a refcount write into the snapshot RefCounted header). - Writes into the frozen decoder.
decodeFunctionCodeBlockForReDecode()(CachedTypes.cpp:2833) doesSetForScope reDecoding(decoder.m_recordsLeafExecutables, false)— two stores (set + restore) into the frozen Decoder object. During the decode, anyCachedRefPtroffset not visited by the initial top-level decode (which is the point ofm_isCached: the executable was decoded but its code blocks were not) reachesdecodeShared()→decoder.cacheOffset()anddecoder.addFinalizer(), growingm_offsetToPtrMapandm_finalizersin the frozen decoder's storage.
Why nothing prevents it
The only removal path for a map entry is ~Decoder() → vm.unregisterBytecodeCacheDecoder() (CachedTypes.cpp:302-303), and the frozen Decoder never destructs because the immortal executable's m_decoder RefPtr is never released (immortal cells' destructors never run — that is the invariant this PR establishes at MarkedBlock/PreciseAllocation). Grepping confirms m_bytecodeCacheDecoderMap is referenced only in ensureBytecodeCacheDecoder/unregisterBytecodeCacheDecoder and the field declaration; nothing on the restore path.
Impact
Not a crash and not unbounded growth. The frozen decoder's cached m_offsetToPtrMap entries point at snapshot StringImpls that were made static (immortal) during freeze, so decodeShared() reuse is safe; growth is bounded by the payload's distinct offsets; and since the decoder never destructs, the accreting m_finalizers are never run on stale pointers. What it does do is (a) dirty the frozen Decoder's mimalloc pages via the refcount bump, the SetForScope stores, and container growth, and (b) leave those containers accreting in an object that never destructs — exactly the class of page-cleanliness issue the maintainer has been fixing throughout this review (72666445, 78136ea), and it directly contradicts the comment at :258-259 and the 92ee5e5b thread's stated "frozen decoder is only read" invariant.
Step-by-step proof
- Build process loads a bytecode cache with a top-level script containing function
f.decodeCodeBlockImpl→ensureBytecodeCacheDecodercreates decoder D, registers{CB → D}in the map.f'sCachedFunctionExecutablehas a cached call code block, so the constructor setsm_isCached = true,m_decoder = D. - Warm-up never calls
f. Freeze runs:f's executable cell is immortal; D's refcount is ≥ 1 held byf.m_decoder. - Restored process:
didRestoreFromStartupSnapshot()does not touchm_bytecodeCacheDecoderMap; the{CB → D}entry is still there. - Restored process calls
f.unlinkedCodeBlockFor→ snapshot branch →m_isCached→ensureBytecodeCacheDecoder(Ref{D->cachedBytecode()}, …).add(CB, nullptr).isNewEntry == false; returnsRef{*D}. decodeFunctionCodeBlockForReDecode(D, offset, …):SetForScopewritesD.m_recordsLeafExecutables = false. Decodingf's code block hitsCachedRefPtroffsets never seen in step 1 →D.m_offsetToPtrMap.add(...)andD.m_finalizers.append(...). Frozen decoder mutated.
How to fix
One line in VM::didRestoreFromStartupSnapshot():
{ Locker locker { m_bytecodeCacheDecoderMapLock }; m_bytecodeCacheDecoderMap.clear(); }(or Heap::rehomeOutOfSnapshotPages(m_bytecodeCacheDecoderMap) if you also want the table's storage out of snapshot pages). The frozen Decoder(s) stay alive via the immortal executables' m_decoder and are still readable for cachedBytecode()/provider(); the first post-restore ensureBytecodeCacheDecoder for that payload then creates a fresh, this-process Decoder as the comment already claims.
There was a problem hiding this comment.
You're right — the comment described the intent, not what happened, since the map still held the building process's decoders. Fixed (locally, next push): didRestoreFromStartupSnapshot() clears the map, so the first lookup in the restored process creates a fresh decoder and the frozen ones are genuinely left alone; the comment is now true.
8e6b087 to
b8dd65d
Compare
| UCollator* IntlCollator::collatorForThisProcess(JSGlobalObject* globalObject) const | ||
| { | ||
| #if USE(BUN_JSC_ADDITIONS) | ||
| VM& vm = globalObject->vm(); | ||
| if (m_startupSnapshotEpoch != vm.startupSnapshotEpoch()) [[unlikely]] { | ||
| (void)m_collator.release(); // the handle belongs to the process that built the snapshot; its ICU is gone | ||
| const_cast<IntlCollator*>(this)->openCollator(globalObject, m_ignorePunctuation ? TriState::True : TriState::False); | ||
| m_startupSnapshotEpoch = vm.startupSnapshotEpoch(); | ||
| } | ||
| #else | ||
| UNUSED_PARAM(globalObject); | ||
| #endif | ||
| return m_collator.get(); | ||
| } |
There was a problem hiding this comment.
🟡 collatorForThisProcess() sets m_startupSnapshotEpoch = vm.startupSnapshotEpoch() unconditionally after openCollator(), whereas every sibling revalidator in this PR (ListFormat, DisplayNames, PluralRules, RelativeTimeFormat, NumberFormat, DurationFormat, DateTimeFormat, Segmenter, Segments, SegmentIterator) gates the epoch update on the reopened handle being non-null / U_SUCCESS so a failed reopen retries next time. Also, compareStrings() calls it before declaring its throw scope with no RETURN_IF_EXCEPTION, so on the (essentially unreachable) failure path it would proceed to ucol_strcoll(nullptr, …) in release. Suggest gating on if (m_collator) to match the pattern the rest of the PR established.
Extended reasoning...
What the inconsistency is
IntlCollator::collatorForThisProcess() (IntlCollator.cpp:296-309) does:
if (m_startupSnapshotEpoch != vm.startupSnapshotEpoch()) [[unlikely]] {
(void)m_collator.release();
const_cast<IntlCollator*>(this)->openCollator(globalObject, ...);
m_startupSnapshotEpoch = vm.startupSnapshotEpoch(); // <-- unconditional
}Every other Intl revalidator added in this same PR follows a defensive pattern where the epoch is only bumped once the reopened handle actually exists — if (self->m_listFormat), if (self->m_displayNames), if (self->m_pluralRules), if (self->m_relativeDateTimeFormatter), if (U_SUCCESS(status)) (NumberFormat/DurationFormat/DateTimeFormat/Segments/SegmentIterator), if (m_segmenter). IntlCollator is the sole exception.
The specific code path
openCollator() calls ucol_open(m_icuLocale.data(), &status) and, on U_FAILURE(status), does throwTypeError(globalObject, scope, "failed to initialize Collator") and returns with m_collator still null. Back in collatorForThisProcess(), the epoch is nonetheless updated to the current VM epoch, and m_collator.get() returns nullptr.
The sole caller, compareStrings() (:311-314), does:
UCollator* collator = collatorForThisProcess(globalObject);
ASSERT(collator);
VM& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);The throw scope is declared after the revalidator call, and there is no RETURN_IF_EXCEPTION between them, so a pending TypeError from openCollator() would not be observed before ucol_strcollUTF8(collator, …) / ucol_strcoll(collator, …) is reached with collator == nullptr. In debug that hits ASSERT(collator); in release ICU dereferences the null collator.
Why nothing prevents it
Nothing in compareStrings() guards on the returned pointer, and the epoch having been updated means that on a subsequent compareStrings() call (with no pending exception this time), the epoch check passes, the reopen branch is skipped, and nullptr is again passed to ICU — the sibling classes' if (handle) gate exists precisely so a failed reopen is retried on the next call rather than latched.
Step-by-step proof
- Build process creates
const c = new Intl.Collator('de');initializeCollator()succeeds, storesm_icuLocale = "de",m_startupSnapshotEpoch = 0, and a liveUCollator*inm_collator. freezeCurrentHeapAsImmortalStartupSnapshot()freezes the cell.- Restored process:
vm.startupSnapshotEpoch()is now 1.c.compare('a', 'b')→compareStrings()→collatorForThisProcess(). Epoch mismatch:m_collator.release()leaks the stale handle,openCollator(globalObject, …)runs. - Hypothesize
ucol_open("de", &status)fails (e.g. OOM inside ICU).openCollator()throws a TypeError and returns;m_collatoris null. m_startupSnapshotEpoch = 1is written anyway.collatorForThisProcess()returnsnullptr.compareStrings()declares its throw scope after this and does not check for exceptions from step 4, then reachesucol_strcollUTF8(nullptr, …)— null-deref in release.- Even if step 6 were caught by the caller and the exception cleared, a later
c.compare(...)would findm_startupSnapshotEpoch == vm.startupSnapshotEpoch(), skip the reopen, and again returnnullptr.
Impact and why nit
The trigger — ucol_open failing on an m_icuLocale string that already succeeded during initializeCollator in the build process, against ICU data that is present (the restore path exists precisely because ICU is initialized in the restored process) — is essentially unreachable outside of OOM. This is not a concrete failure a user would hit; it's a defensive-consistency gap in one of eleven parallel implementations, worth pointing out because the maintainer explicitly stated the intent that these follow "exactly the segmenter's shape" and that "the rule is uniform" (timeline 2026-08-10T03:57:38Z).
Fix
One-line: gate on the reopened handle like the siblings do.
const_cast<IntlCollator*>(this)->openCollator(globalObject, m_ignorePunctuation ? TriState::True : TriState::False);
if (m_collator)
m_startupSnapshotEpoch = vm.startupSnapshotEpoch();Optionally also add RETURN_IF_EXCEPTION(scope, {}) after collatorForThisProcess() in compareStrings() (moving the DECLARE_THROW_SCOPE above it), though with the epoch gate in place the retry-on-next-call behavior already matches the siblings.
There was a problem hiding this comment.
In the current head the store is unreachable on failure — reopening either succeeds or fails deterministically inside the revalidator (see the update on the thread just above) — so unconditional is now correct by construction there; the sibling shape you describe is what the others use because their openers can leave a null handle.
b8dd65d to
9c8a7dc
Compare
…ep snapshot cells' pages clean Heap::freezeCurrentHeapAsImmortalStartupSnapshot() runs after a synchronous full collection and marks every MarkedBlock and precise allocation immortal: the mark bits it has at that point become its permanent liveness, and the collector never marks into, sweeps, allocates from or ages such a block again (BlockDirectory keeps them out of the empty/canAllocate/unswept sets; areMarksStale() answers "not stale" for them, so the stock mark fast paths see live snapshot cells as already marked and never write). The write barrier records snapshot cells that get written in a side set instead of touching their cell state; those cells, plus the precise allocations that existed at the freeze, are the full-collection roots for everything reachable from the snapshot, so a restored process's collections stay proportional to what it allocated itself. Things that would otherwise write into snapshot cells during ordinary execution move out of them: snapshot Structures are flagged with a spare bitfield bit and lock through a striped side lock; snapshot UnlinkedCodeBlocks are not aged; watchpoint sets that live in the snapshot chain their new watchpoints through a side table; every string in the snapshot is made static, and static strings no longer write their refcount; a few identity writes (setIndexInSpace, resetAge, JITStubRoutine flags) are skipped when the value is unchanged. Unlinked code blocks decoded for snapshot executables after the freeze are kept in a heap-side table. Costs on paths every process runs: one byte test in areMarksStale() per mark and the isStatic() test in StringImpl::ref()/deref(); everything else is behind hasImmortalBlocks() or a per-object bit. Options: verboseStartupSnapshotFreeze, unlinkedCodeBlockJettisonAge and useUnlinkedCodeBlockJettisoningForBytecodeCache (defaults preserve current behaviour).
…low re-decoding jettisoned code When a CachedBytecode payload is persistent (mapped from a file or embedded in the executable), the instruction streams and expression info decoded from it borrow the payload's bytes instead of copying them (useBorrowedBytecodeFromCache). InstructionStream refs read through a span owned by the stream — the writer refreshes it after every mutation — so owned and borrowed streams share one access path. UnlinkedFunctionExecutables generated from a cache remember where their record lives, so a code block that was jettisoned (useUnlinkedCodeBlockJettisoningForBytecodeCache) can be decoded again on demand; for executables that are part of a snapshot the decoded block goes to the heap's side table rather than into the never-written cell.
…artup snapshot has to recompute Stack bounds and the cached stack top belong to the process that took the snapshot; so do the threads behind AutomaticThreads (GC helpers, JIT worklists — they are restarted on the next notify), ParkingLot entries, the random number generator's state, ICU objects held by the Intl and date caches, and the regexp JIT code. Per-heap tables that take inserts every run are re-homed so their growth does not dirty snapshot pages, and allocation pacing starts over from an empty heap: the limits in the snapshot were computed while its cells were ordinary live cells, whereas now they are immortal and uncounted. Signal handling is kernel state: the Mach exception port and handler thread (or the sigaction table elsewhere) that WTF installed belong to the process that took the snapshot, while WTF's own record says they are in place, so a wasm out-of-bounds access in the resumed process was a crash rather than a trap. SignalHandlers::reinstallAfterSnapshotRestore() installs them again for the resumed process, after which the config is frozen again (it is copied in writable, since this process never froze it). On the way in, completeAllJITPlansBeforeStartupSnapshot() also cancels and joins wasm compilation plans: BBQ/OMG tier-ups still running after a warm-up were installing code into the executable pool while the snapshot was being written, so the snapshot held the module's pointers to code it did not contain. The OS random source is reopened before the generator is stirred: on Linux it holds a /dev/urandom descriptor, and the number restored with it belongs to the process that took the snapshot, so the stir read from whatever the resumed process happened to have there and could block forever. Likewise the thread suspend/resume signal handler (SIGPWR on Linux), installed once by initializePlatformThreading(): the resumed process was killed by the signal the first time something suspended a thread. StackBounds (Linux) caches the process's original environ pointer in a static to keep the environment vector out of the main thread's bounds; resumed from a snapshot, that is an address on the build process's stack, and whenever ASLR put the new stack over that range the origin was clamped below the live frames (sanitizeStackForVM's assertion, ~1% of launches). Adopting the thread forgets the cached value so it is re-read from this process's environ.
…s being taken or restored With MIMALLOC_DETERMINISTIC_HINT set (the allocator reads the same variable), non-executable reservations are placed by a bump cursor in a fixed window, so the memory captured into a snapshot comes back at the same addresses. One getenv per process; no effect otherwise.
9c8a7dc to
4c0ca85
Compare
There was a problem hiding this comment.
This automated pass found no new issues. Given the scope — new GC invariants for immortal snapshot blocks, Structure side-locking, bytecode-cache borrowing, and process-restore hooks across threading/signals/ICU — a human review is still warranted.
Checked the immortal mark/sweep/allocate exclusion and the side-remembered-set barrier against the concurrent collector's read paths.
Checked the b8dd65df Intl epoch revalidation across all eleven Intl classes — each releases-and-reopens its ICU handle on epoch mismatch, and SegmentIterator re-establishes position via ubrk_isBoundary.
Checked the shared-decoder leaf-executable suppression (now SetForScoped per re-decode call) and the borrowed InstructionStream/ExpressionInfo payload paths.
Ruled out: intermediate pthread keys created in adoptCurrentThreadForStartupSnapshot misfiring destructTLS — nothing sets a value on them, so the destructor is never invoked.
Extended reasoning...
Overview
83 files, ~+1000/−160, across four layered commits: (1) an immortal-heap freeze that makes every MarkedBlock/PreciseAllocation permanently live with frozen mark bits, redirects write barriers on snapshot cells to a side remembered set, and moves post-freeze mutations (Structure locks, WatchpointSet chains, UnlinkedCodeBlock aging, StringImpl refcounts) out of snapshot pages; (2) bytecode-cache payload borrowing with a span-based InstructionStream and a per-VM shared Decoder map plus re-decode of jettisoned code blocks; (3) VM::didRestoreFromStartupSnapshot() recomputing stack bounds, RNG, ParkingLot, AutomaticThreads, ICU handles (all Intl classes now epoch-revalidate), signal handlers, and pacing; (4) deterministic mmap placement hints. Everything Bun-specific is behind USE(BUN_JSC_ADDITIONS); the non-guarded changes are compare-before-write guards, the StringImpl::isStatic() check in ref()/deref(), the InstructionStream span refactor, and the extra immortal bit in BlockDirectoryBits.
Security risks
The most sensitive area is decodeFunctionCodeBlockFromExecutableRecord, which reinterprets bytes at a stored offset inside a provider's cache as a CachedFunctionExecutable — mitigated by bounds checks and a start-offset/line-count/parameter-count identity guard against the live executable, and the offset is only recorded from a decode of the same payload. SignalHandlers::reinstallAfterSnapshotRestore unfreezes g_wtfConfig/g_gigacageConfig, but only from the single-threaded restore path and the caller re-freezes immediately. OSAllocatorPOSIX reads getenv("MIMALLOC_DETERMINISTIC_HINT") once as a boolean gate — no injection surface. RNG restir is correct (would otherwise replay the build process's stream). No auth/permissions code touched.
Level of scrutiny
High. This changes core GC invariants (mark staleness, sweep eligibility, barrier semantics, cell-state transitions) that every allocation and collection depends on, and threads new state through SlotVisitor, MarkedBlock, BlockDirectory, and Structure. The prior review cycle on this PR surfaced and fixed roughly a dozen real issues (immortal-butterfly realloc leaking the flag, disk-cache leaf-executable regression, ARM64E goto-over-init, lock-order inversion, stale-ICU-handle crashes on macOS, several page-dirtying identity writes) — that history alone argues against auto-approval. Correctness here is validated by the embedder's snapshot fixtures and a differential JSTests/stress run, but neither is exercised in this repo's CI.
Other factors
All prior inline findings from this reviewer are marked resolved, with fix SHAs cited. JetStream2 parity and identical stress-suite pass/fail were reported by the author. The one candidate raised and refuted this run (intermediate pthread keys in adoptCurrentThreadForStartupSnapshot) is benign because pthread only invokes a key's destructor for non-null values, and nothing writes to the fill keys. Deferring rather than approving: this is exactly the class of change (large, GC-core, cross-cutting) where a human maintainer should sign off.
1 participant
JavaScriptCore side of startup snapshots in Bun: a process is frozen once it is idle, and later launches of the same executable map the frozen heap and continue in the event loop instead of booting. Four commits on top of the current pin, each building on its own; 54 files, +998/−158. Companion: oven-sh/mimalloc#13 (allocator side); the Bun side is oven-sh/bun#37225.
Heap::freezeCurrentHeapAsImmortalStartupSnapshot()runs after a full collection and makes every block and precise allocation immortal: its marks become its permanent liveness and the collector never marks into, sweeps, allocates from or ages it. Barriers on snapshot cells record the cell in a side set instead of writing its state; written cells plus the precise allocations that existed at the freeze are the full-GC roots for the snapshot. Writes that ordinary use would make into snapshot cells are moved out or skipped: snapshot Structures (one spare bitfield bit) lock through a striped side lock, snapshot UnlinkedCodeBlocks are not aged, snapshot watchpoint sets chain new watchpoints through a side table, snapshot strings are made static and static strings no longer write their refcount, and a few identity writes compare first.areMarksStale()per mark, andisStatic()inStringImpl::ref()/deref(); everything else is behindhasImmortalBlocks()or a per-object bit. New options keep current defaults.InstructionStreamrefs read through a span the stream owns, so owned and borrowed streams share one branch-free path. Cache-generated executables remember their record so a jettisoned code block can be decoded again (opt-in option).VM::didRestoreFromStartupSnapshot()/dev/urandomdescriptorRandomDeviceholds — the restored number was the build process's), ICU-backed Intl/date caches, regexp code, per-heap tables that grow every run, allocation pacing (the snapshot's limits were computed while its cells still counted as live), and the kernel side of signal handling —SignalHandlers::reinstallAfterSnapshotRestore()re-creates the Mach exception port/thread or sigaction entries, after which the config is frozen again. On Linux it also re-installs the thread suspend/resume signal handler (Thread::reinstallSuspendResumeSignalHandlerForStartupSnapshotRestore()), and adopting the main thread forgetsStackBounds' cached originalenviron, which is a build-process stack address that clamped the origin below the live frames whenever the new stack happened to land over that range. On the way in,completeAllJITPlansBeforeStartupSnapshot()completes JS plans and cancels+joins wasm plans, which were otherwise still installing code into the pool while it was being written.getenvper processMeasurements, this branch vs. stock WebKit at the same commit, both built the same way (external mimalloc), 5 interleaved JetStream2 pairs on an M-series laptop: parity (+0.05 % on medians; per-benchmark spread is the usual ±5 %). Peak RSS of an allocation-churn script equal or lower in 7 of 8 pairs; hello-world +0.1 MB.
JSTests/stressrun differentially against the stock shell: identical pass/fail sets over the full suite (5,599 tests). Exercised further bytest/js/bun/startup-snapshoton the Bun branch and by an application that starts from a snapshot.Earlier iterations of this branch kept executables' link state and structures' transition tables out of line as well; those turned out to add loads on shared paths (and the link-state arena was never freed) without a measurable memory benefit, so they are gone. The
UnlinkedMetadataTablecompaction that used to be here is unrelated to snapshots and will be proposed separately.