Repository navigation
Extend startup snapshot support - #8405
dbezhetskov wants to merge 4 commits into
Conversation
…napshot A Worker on the new module registry (`new_module_registry`) now qualifies for the startup snapshot pipeline. The zygote shares the Worker's `jsg::modules::ModuleRegistry`: the registry itself holds no V8 state, every V8 handle lives in the per-context `IsolateModuleRegistry`, so the Worker started from the snapshot re-resolves the baked entries in that same registry.
The startup snapshot no longer requires the TypeScript streams implementation. What kept C++ streams out was a stream whose pull() or start() is still pending when top-level evaluation ends: its C++ promise continuation is a v8::Function whose callback is a template instantiation the zygote's external reference table does not hold, so CreateBlob() aborted the process on the unknown address. The jsg::Promise::thenImpl now registers the two callbacks of every continuation created while a snapshot is being prepared; the serializer then reaches the continuation's opaque callback data, which has no snapshot transfer, and the zygote fails with the usual unclonable report (ReadableStreamDefaultController, OpaqueWrappable, ReadableStream), so the Worker starts without a snapshot instead of taking the process down. A C++ stream whose top level leaves no pending pull transfers like any other object.
The `performance` global is reachable from the context of any Worker whose top level touches it (reading performance.now() or importing node:perf_hooks is enough), and Performance had no snapshot transfer, so such Workers started without a snapshot. An instance that has recorded no entries is rebuilt from the isolate's limit enforcer; one with entries holds jsg::Refs to PerformanceEntry objects, which a clone cannot carry, and stays unclonable.
|
This pull request's reviews are posted as pull request reviews: see the review of |
|
clanky review |
There was a problem hiding this comment.
💬 Approved with comments — One WARNING: Performance::snapshotClone() silently drops top-level EventTarget listeners on performance. Plus an INFO about missing tests for the new Performance clone and the pending-C++-continuation fallback.
Findings: 1 warning · 1 info (2 are inline comments on the diff)
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
🔧 Fix with your agent
Paste the following into your coding agent to investigate and fix these issues.
Fix the following code review issues found in pull request #8405.
Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order
## WARNING (1)
### [Code Quality] `Performance::snapshotClone()` checks only `entries`, so listeners added to `performance` at the top level are silently dropped when the Worker starts from the snapshot.
- **File:** src/workerd/api/performance.c++:16
## INFO (1)
### [Code Quality] No test covers the pending-C++-continuation fallback registered here, or `Performance::snapshotClone()`.
- **File:** src/workerd/jsg/promise.h:457
📋 Code Quality: ⚠️ 1 warning · 1 info
Performance clone loses EventTarget listeners; the new Performance clone and pending-continuation fallback have no tests.
Recommendation: 📝 Minor Comments
- [WARNING]
Performance::snapshotClone()checks onlyentries, so listeners added toperformanceat the top level are silently dropped when the Worker starts from the snapshot. (src/workerd/api/performance.c++:16) - [INFO] No test covers the pending-C++-continuation fallback registered here, or
Performance::snapshotClone(). (src/workerd/jsg/promise.h:457)
View full review
src/workerd/api/performance.c++:16:PerformanceextendsEventTarget. Whenenable_global_performance_classesor the perf_hooks flag is on, it inheritsaddEventListenerandon*handlers (performance.h:603-604), and those handlers live inEventTarget::typeMap.snapshotClone()only checksentriesand returns a freshPerformancewith an empty handler map. The snapshot restores listeners in C++ only for the global scope (EventTarget::restoreSnapshotListener()). A listener registered onperformanceat the top level is therefore lost on load, and the Worker does not fall back to starting without a snapshot.src/workerd/jsg/promise.h:457: nothing tests the newisPreparingSnapshot()registration inthenImpl. Thecpp-streamscase deliberately has no pendingpull(). Nothing testsPerformance::snapshotClone()either, neither the clonable case nor the fallback once entries are recorded.
✅ All other checks passed
🔒 Security: ✅ LGTM
No security issues found in this change.
📖 AGENTS.md: ✅ LGTM
This pull request does not change any AGENTS.md files.
⚡ Performance: ✅ LGTM
No performance issues found.
📝 Documentation: ✅ LGTM
The updated docs/startup-snapshot.md sections match the changed code.
Reviewed commit: 2e74745f
| namespace workerd::api { | ||
|
|
||
| kj::Maybe<kj::Own<jsg::Wrappable>> Performance::snapshotClone() const { | ||
| if (entries.size() != 0) return kj::none; |
There was a problem hiding this comment.
Performance::snapshotClone() checks only entries, so listeners added to performance at the top level are silently dropped when the Worker starts from the snapshot.
Performance is an EventTarget. Under the global-performance-classes or perf_hooks flags, a Worker can call performance.addEventListener(...) or set an on* handler at the top level, and those handlers live in EventTarget::typeMap. This clone only refuses when entries is non-empty, so the restored Worker gets a Performance with no listeners. A later performance.dispatchEvent(...) then does nothing, and nothing falls back to a plain start. Return kj::none when the EventTarget has any registered handlers too, which probably needs a small accessor on EventTarget.
| return js.withinHandleScope([&] { | ||
| auto context = js.v8Context(); | ||
|
|
||
| if (js.isPreparingSnapshot()) { |
There was a problem hiding this comment.
ℹ️ INFO · Code Quality
No test covers the pending-C++-continuation fallback registered here, or Performance::snapshotClone().
This registration is now the only thing that turns a pending C++ continuation (such as a stream whose pull() is still pending at the end of top-level evaluation) into a failed zygote instead of a V8 abort. The worker factory no longer blocks this configuration up front. A .wd-test case with such a stream should assert that the snapshot is not used and that the Worker still serves requests. Cases for performance.mark() at the top level and for a listener on performance would also cover the new Performance clone.
Support for the snapshot: