Repository navigation
Conversation
293d8e9 to
878e241
Compare
|
Since last review: 0 resolved, 0 still open, 0 new. Not re-run: docs, design-simplicity (no author changes in their files since the last review) Reviewed commit: 04aeb430 · github run |
erikcorry
left a comment
There was a problem hiding this comment.
Review of the store/consume/produce design, with the edgeworker MR (!15320) read alongside. Nothing blocking; inline comments cover the specifics. Summary:
- Startup limit: the SHA-256 and store lookup run inside
enterStartupJson both hit and miss, which the validator (no store) never pays. Deterministic and small, but it is the "passes upload, exceeds the limit in prod" direction. Move the key computation/lookup outside the limit scope. - Request-time compiles (module first imported during a request) are serialized synchronously in the request on the elected isolate, and that cache is the weak eager-functions-only kind.
onCompileCacheFoundandonCompileCacheRejectedboth fire for one compile when V8 refuses the data during deserialization.- The production window is per isolate but opened/closed per
Script/Worker; pin the one-Script-per-isolate assumption with an assert. - Small items:
boolparameter,nullptrasArrayPtr, stalecachedDatacomment,jsg/AGENTS.mdKEY FILES row forcode-cache.h,docs/reference/detail/new-module-registry.mdcompile-cache sections still describe the per-module slot as the only path.
Measurements that would help judge the limits above and maxEntryBytes: blob size and deserialize time for the Hono bundle (and blob/source ratio); CreateCodeCache wall time on the producing isolate; and a kEagerCompile production row as a data point (expected to deserialize more than the validator measured, so not a recommendation, but it would show the bound).
Verified and fine: key completeness (origin fields are reapplied from ScriptDetails, not serialized; flags all set pre-init in V8System; decoders split by Unit); cross-isolate-group validity (RO refs are page index + offset, checksummed; no raw pointers); no-store behaviour unchanged; v8::Global release before Dispose on every failure path; lookup.hit Arc outlives compileSource.
Possible follow-up, not for this PR: a two-level key, rapidhash(source) with a per-process seed as the table key and the SHA-256 stored in the entry and verified only on a hit. Misses then cost ~50 µs/MB instead of ~0.5 ms/MB, hits are unchanged, and the SHA-256 remains the integrity control.
These comments were drafted with an AI assistant and may contain mistakes.
878e241 to
751f141
Compare
|
The generated output of |
|
The recovered follow-up is pushed; current head is
The direct-API comparison uses an 815,423-byte Hono/Zod bundle, five interleaved rounds, V8 15.6.75, and optimized V8 in the default CI build. After-top-level production gives a 315,432-byte blob (0.39x source), median serialization 2.508 ms, and median deserialization 0.912 ms. Explicit eager production gives 997,288 bytes (1.22x), 6.266 ms and 2.396 ms respectively. The full table and CI link are in the description. V8 deserialization and rejection housekeeping remain inside startup CPU accounting. The implementation does not claim exact validator/production CPU parity. Startup-only production and preparing hashes/lookups before the limit are the current scope; extra observability is deferred. Windows CI reproduced nine unsafe-module fixture timeouts near the end of the 16-handler suite. Both affected fixtures use real-time waits and a 30-second eviction deadline, exceeding the default 15-second test budget. @erikcorry, please re-review the current head, including the remaining deserialization accounting and sharing documentation. |
1f70292 to
676373e
Compare
jasnell
left a comment
There was a problem hiding this comment.
Nice to see the more robust approach.
7995ec9 to
5eeadd6
Compare
An embedder can now give an isolate a jsg::CodeCacheStore, a process-wide store of V8 code caches for worker bundle modules and service-worker main scripts. Without one, nothing changes. With a store, both module registries and NonModuleScript::compileWorkerScript() compile through one per-isolate helper, jsg::IsolateCodeCache. It looks the source up in the store, pre-checks a hit with CachedData::CompatibilityCheck(), compiles with kConsumeCodeCache, and reports refused data, whether V8 refuses it in the check or while deserializing, to the store and to the CompilationObserver hooks. On a miss the store may elect the isolate to produce the entry, so concurrent isolates do not all serialize the same code. V8 checks a cache only against its version, flags, read-only snapshot and the source length, not the source content, so entries are keyed by a SHA-256 of the exact UTF-8 source and the way it is compiled (CodeCacheKey). Worker::Script opens a production window and Worker closes it after the top level has run, so the caches hold the functions V8 compiled lazily while running it, not only the eagerly compiled ones. Code compiled after startup, such as a module first imported during a request, is serialized right after compilation. Modules whose evaluation failed, and scripts whose startup failed, are not cached. Production runs outside the startup CPU limit. A Script that no Worker runs closes the window when it is destroyed, so its elections do not keep other isolates from producing those caches. Pending entries hold v8::Globals, so IsolateBase::dropWrappers() releases them under the lock. The new registry keeps its per-module cache slot for builtins and for isolates without a store. For bundle modules in isolates with a store, the content key replaces the slot, so registries created per replica share caches too. The key is computed once per module, from the UTF-8 bytes, in the same lazy initializer as the source encoding. The legacy bundle ModuleInfo constructor loses its compileCache parameter, which every caller passed as null; the store is the cache for bundle modules. Builtin constructors keep theirs. code-cache-test covers both registries and service-worker scripts: a second isolate consuming the cache, production after evaluation capturing lazily compiled functions, distinct content of equal length missing, refused data (flags hash, truncation, and a cache for source of another length), failed evaluation, and releasing the window on discard and on isolate teardown. worker-code-cache-test covers a Worker producing its Script's caches after the top level, and a Script without a Worker releasing its elections when destroyed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Prepare source hashes and store lookups before startup CPU scopes. Keep prepared results through the window so identical modules share a lookup and transfer the producer election only once. Request-time imports consume existing entries but never produce or replace them. Count accepted hits separately from rejected caches, enforce store/window invariants, and cover classic-worker lifecycle, prepared lookups, and rejection outside startup. Document startup imports and process-local sharing. Deserialization remains inside CPU accounting; no standard Worker API changes.
KJ heterogeneous map lookups preserve the argument type. Cast source addresses to const void* so lookup hashes the address and does not instantiate the forbidden char-pointer string hash.
Let TestFixture accept a module registry matching its feature flags. Construct Workers with fresh CODE_CACHE_KEYS registries and verify that preparation performs the only store lookup outside startup CPU accounting. The second Worker must consume the produced cache without serializing another entry.
Per-isolate bootstrap also reports cache hits through the observer. Scope the regression counter to bundle ESM compilation so it checks exactly the user module hit with every autogate configuration.
These fixtures exercise eviction with a 30-second deadline, and the alarm-name fixture also waits for a real-time alarm. The default medium-test budget is only 15 seconds. Windows CI timed out while the unsafe-module suite continued passing its handlers. A focused five-run job reproduced nine timeouts near the end of that suite. Classify both fixtures as large so their 60-second budget covers their bounded waits and process startup. Preserve every assertion and require five passes without retries.
5eeadd6 to
b84b20a
Compare
|
💬 Approved with comments — No blocking or production-impact problems found. Two documentation WARNINGs (store use on the new registry depends on Findings: 2 warnings · 5 info The decision is advisory: this comment never approves, blocks or requests changes on the pull request. 🔧 Fix with your agentPaste the following into your coding agent to investigate and fix these issues. 📋 Code Quality: 💬 4 infoThe cache-window lifecycle, the cache keys and both registry compile paths look correct and are well tested. Four low-impact edge cases. Recommendation: 💬 Optional Nits
View full reviewThe production-window lifecycle, the SHA-256 source keys, the address-keyed prepared lookups, and the legacy and new registry compile paths all look correct, and the tests cover them well. Four low-impact notes: A store failure can fail worker startup (
A rejected prepared hit is served again (
Unrelated BUILD changes (
📝 Documentation:
|
Keep the existing ModuleInfo constructor that accepts an embedded compile cache. The UTF-8 cache regression on main calls this overload and verifies cache consumption with invalid source of matching length. Prefer that explicit cache when present; otherwise use the isolate's code cache store. Keep the four-argument constructor for user modules.
V8 15.7 stores a 256-bit source hash in the serialized code header. The flags hash now follows that digest at offset 40. Corrupting byte 12 changes the source hash and is rejected during compilation, rather than by the flags compatibility check that the regression exercises. Update the corruption offset while retaining the expected flags mismatch, replacement generation and successful replacement reuse.
An embedder can now give an isolate a
jsg::CodeCacheStore, a process-wide store of V8 code caches for worker bundle modules and service-worker main scripts. Without one, nothing changes.With a store, both module registries and
NonModuleScript::compileWorkerScript()compile through one per-isolate helper,jsg::IsolateCodeCache. It looks the source up in the store, pre-checks a hit withCachedData::CompatibilityCheck(), compiles withkConsumeCodeCache, and reports refused data to the store and to theCompilationObserverhooks. On a miss the store may elect the isolate to produce the entry, so concurrent isolates do not all serialize the same code.V8 checks a cache only against its version, flags, read-only snapshot and the source length, not the content, so entries are keyed by a SHA-256 of the exact UTF-8 source and the compilation unit (
CodeCacheKey).Worker::Scriptopens a production window andWorkercloses it after the top level has run, so caches hold the functions V8 compiled lazily while running it, not only the eagerly compiled ones (on a Hono bundle: 336 functions instead of 12, first response 8.5 ms instead of 11.1 ms in d8). Code first compiled during a request may consume an existing entry but never produces or replaces one. Import modules during startup to populate their caches. Modules whose evaluation failed and scripts whose startup failed are not cached. AScriptnoWorkerruns closes the window on destruction. Pending entries holdv8::Globals, released inIsolateBase::dropWrappers().Source hashes and initial store lookups are prepared before startup CPU accounting. Identical modules share a prepared lookup and transfer the producer election only once. V8 deserialization remains CPU-counted. New-registry builders opt into
CODE_CACHE_KEYS; builtins and isolates without a store keep their per-module cache slot.Tests cover both registries through Worker construction (including lookup timing and the accepted-cache hook), module and classic Worker lifecycle, prepared lookups (including duplicate and unused sources), hits versus rejection, startup-only production, request-time rejection without replacement, and store/window invariants.
Verification and landing
Rebased onto workerd main
587bab1c2; current head75990bad5. Conflict resolution retains main's replacement I/O task test and JSG helper table. The companion !15320 pins this exact head. Formatting and whitespace checks pass. Fresh full upstream and downstream CI are running.Earlier full CI passed all upstream platforms, coverage and clang-tidy. Strict focused CI passed 160 first-attempt result events, with explicit completion for every Worker cache case: combined verification, targeted Windows cases. Diagnostic workflow and benchmark commits are excluded from this PR.
Erik's addressed comments have replies and are resolved. Hashing and initial lookups run before startup CPU accounting; V8 deserialization and rejection housekeeping remain counted. The process-local store's cross-account sharing and accepted whole-module timing existence oracle are documented in the companion embedder header. Formal human approval remains a landing requirement.
Merge this PR first, then update the companion runtime pin if the merge changes the SHA.
Direct eager-compilation comparison
CI benchmark, five interleaved rounds of the same 815,423-byte Hono/Zod bundle, fresh isolates, V8 15.6.75. Default CI build with V8 compiled at
-O3; these are wall times. Values are medians. This useskEagerCompiledirectly and is not directly comparable to the older d8/edgeworker V8 15.5 measurements on a different host.The eager blob is 3.16 times the after-top-level blob, and its deserialization cost is 2.63 times as large. The startup-only after-top-level policy remains unchanged. The benchmark commit is isolated from this PR/MR.