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.06% 90.05% -0.01%
==========================================
Files 754 754
Lines 255742 255803 +61
Branches 48314 48332 +18
==========================================
+ Hits 230341 230373 +32
- Misses 16534 16556 +22
- Partials 8867 8874 +7
🚀 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?
There was a problem hiding this comment.
Good question - I think IsolateData makes a bit more sense than Environment, for when multiple Environment is created from the same IsolateData (not that we really support it, still) the cache should be shared.
There was a problem hiding this comment.
@joyeecheung done in d1c9826: node::SetBuiltinCodeCache(isolate_data, cache) attaches it to the IsolateData (it runs CompatibilityCheck() there and returns the result), Environment's constructor picks it up right after the snapshot's cache, and CreateEnvironment() is back to its old signature.
e512f86 to
189fbb1
Compare
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 SetBuiltinCodeCache(isolate_data, cache) attaches them to an IsolateData, next to where a snapshot's code cache lives, so that every Environment created from it afterwards starts with them. The setter runs CachedData::CompatibilityCheck() against the isolate first and returns the result, leaving the IsolateData untouched for a cache made with another V8 version, flag set or read-only snapshot. Entries from a snapshot still merge with it (RefreshCodeCache() now merges instead of assuming a single call). CreateEnvironment() plus LoadEnvironment() of an empty script goes from about 38 ms to 13 ms with a cache attached. 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. A cctest generates a cache, attaches it, checks that a new Environment's bootstrap compiles from it and that a corrupted cache is refused; embedtest gains --no-harvest-builtin-code-cache for a test that a worker does or does not find a harvested cache depending on the flag. Signed-off-by: Shelley Vohr <shelley.vohr@gmail.com>
189fbb1 to
d1c9826
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 attach a cache built ahead of time to anIsolateData, plus a flag to skip the runtime serialization;nodeitself is unchanged.CreateEnvironment()+LoadEnvironment(env, "0"), fresh context, linux x64, n=6EmbedderBuiltinCodeCacheattached to theIsolateDatanode::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;node::SetBuiltinCodeCache(isolate_data, cache)attaches them next to where a snapshot's code cache lives, so every Environment created from thatIsolateDataafterwards starts with them (they share the buffers; the cache object can be freed after the call). The setter runsCachedData::CompatibilityCheck()against the isolate and returns the result, leaving theIsolateDatauntouched for 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.Tests: a cctest generates a cache, attaches it, checks that a new Environment compiles
internal/bootstrap/nodeand everything outsideinternal/per_context/*from it, and that a corrupted cache is refused and anullptrclears it; a cctest for theRefreshCodeCachemerge;embedtestgains--no-harvest-builtin-code-cachefor an embedding test that a worker started with and without the flag does and doesn't find a harvested cache. embedding, cctest and the default suite pass.Disclosure: the code, tests, measurements and this description were written by Claude Code, directed and reviewed by @codebytere.