src: let embedders supply a builtin code cache without a snapshot - #65352
src: let embedders supply a builtin code cache without a snapshot#65352codebytere wants to merge 1 commit into
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
1b99e86 to
136d2ad
Compare
github-actions
Bot
removed
the
request-ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65352 +/- ##
==========================================
+ Coverage 90.05% 90.07% +0.01%
==========================================
Files 751 751
Lines 254420 254829 +409
Branches 47975 48101 +126
==========================================
+ Hits 229121 229540 +419
+ Misses 16483 16481 -2
+ Partials 8816 8808 -8
🚀 New features to boost your workflow:
|
136d2ad to
d33c3df
Compare
legendecas
added
semver-minor
github-actions
Bot
removed
the
request-ci
d33c3df to
e512f86
Compare
github-actions
Bot
removed
the
request-ci
| // run by NewContext(), start with these entries. Entries a snapshot provides | ||
| // still apply. Call before creating contexts/Environments; may be called | ||
| // again to replace the set for later ones. | ||
| NODE_EXTERN void SetBuiltinCodeCache( |
There was a problem hiding this comment.
I am a bit hesitant of making this a process-wide method, this potentially makes it difficult for us to reorganize the hierarchy in the future. Can we make the list per-Environment on the API level? We can probably make the wrappers thin enough so that it's possible to share underlying cache across different Environments.
Also I think on the API level, it would be better to reuse/nest the v8::ScriptCompiler::CachedData struct to pass things around instead of adding an ad-hoc structure.
Another thing to safe guard: code cache must be generated from the same isolate as the snapshot data (or lack thereof) or otherwise it would crash/corrupt the memory due to readonly space mismatches. We should probably call v8::ScriptCompiler::CachedData::CompatibilityCheck somewhere to ensure that they matches or surface the error otherwise.
There was a problem hiding this comment.
@joyeecheung reworked along those lines in 189fbb1: it's now a node::EmbedderBuiltinCodeCache whose entries are {id, std::unique_ptr<v8::ScriptCompiler::CachedData>}, passed per Environment as a trailing CreateEnvironment() parameter (so CommonEnvironmentSetup::Create() forwards it) with one instance shareable across Environments, and CreateEnvironment() runs CachedData::CompatibilityCheck() over the entries before using them and returns nullptr on a mismatch. the process-wide setter is gone; the one thing that loses is the internal/per_context/* scripts NewContext() compiles outside any Environment, which i've left compiling from source rather than keep a global for them. does the CreateEnvironment() parameter work for you, or would you rather it hang off IsolateData next to the snapshot's cache?
Environments created from the built-in snapshot get the builtins' code cache from that snapshot. An embedder that bootstraps an Environment from scratch (its own isolate and context, no EmbedderSnapshotData) has no way to provide one: every builtin the bootstrap touches is compiled from source in every such process, and each of them then serializes a fresh cache (SaveCodeCache) that only a later worker thread would ever consume. Add node::EmbedderBuiltinCodeCache for that case. Its entries pair a builtin id with a v8::ScriptCompiler::CachedData; Generate(context) compiles every builtin in a context of the right kind of isolate and returns them for a build step to embed, and an instance passed to CreateEnvironment() (new trailing parameter, forwarded by CommonEnvironmentSetup::Create()) seeds that Environment's loader. CreateEnvironment() runs CachedData::CompatibilityCheck() on the entries first and returns nullptr for a cache made with another V8 version, flag set or read-only snapshot. One instance can be passed to any number of Environments; entries from a snapshot still merge with it (RefreshCodeCache() now merges instead of assuming a single call). ProcessInitializationFlags::kNoHarvestBuiltinCodeCache stops serializing caches for builtins compiled without one, for embedders that supply their own or never create workers. The default is unchanged because worker threads copy the harvested cache. embedtest gains --builtin-code-cache-create, --builtin-code-cache and --no-harvest-builtin-code-cache, and a test that generates a cache in one process, checks that another Environment's bootstrap compiles with it, and that a worker does or does not find a harvested cache depending on the flag. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
e512f86 to
189fbb1
Compare
5 participants
An embedder that creates its Environments without Node's snapshot (its own isolate, no
EmbedderSnapshotData) compiles every builtin the bootstrap touches from source in each such process, and then serializes a fresh code cache for each of them that only a later worker thread ever reads. This adds a way to handCreateEnvironment()a cache built ahead of time, plus a flag to skip the runtime serialization;nodeitself is unchanged.embedtest -- 0, process start to exit, n=30 interleavedEmbedderBuiltinCodeCachepassed toCreateEnvironment()kNoHarvestBuiltinCodeCachenode::EmbedderBuiltinCodeCacheholds entries pairing a builtin id with av8::ScriptCompiler::CachedData.EmbedderBuiltinCodeCache::Generate(context)compiles every builtin in a context made withnode::NewContext()and returns them for a build step to embed; an instance passed toCreateEnvironment()(new trailing parameter, forwarded byCommonEnvironmentSetup::Create()) seeds that Environment's loader, and one instance can serve any number of Environments, which share its buffers.CreateEnvironment()runsCachedData::CompatibilityCheck()on the entries first and returnsnullptrfor a cache made with another V8 version, flag set or read-only snapshot. A snapshot's entries still merge with it, soRefreshCodeCache()merges withinsert_or_assigninstead of asserting a single call.ProcessInitializationFlags::kNoHarvestBuiltinCodeCachestopsLookupAndCompile()from serializing a cache for builtins compiled without one. The default stays as it is because worker threads start from that harvested cache.The per-context scripts
NewContext()runs (internal/per_context/*) are outside an Environment and keep compiling from source.embedtestgets--builtin-code-cache-create <file>,--builtin-code-cache <file>and--no-harvest-builtin-code-cacheso the test drives this through an embedder binary: it generates a cache in one process, checks that another Environment's bootstrap compiles from it, and that a worker started with and without the flag does and doesn't find a harvested cache.Tests: the new embedding test and a cctest for the
RefreshCodeCachemerge; embedding, cctest and the default suite pass. A cache with a corrupted header makesCreateEnvironment()returnnullptr(EmbedderBuiltinCodeCache rejected: 5underNODE_DEBUG_NATIVE=CODE_CACHE).Disclosure: the code, tests, measurements and this description were written by Claude Code, directed and reviewed by @codebytere.