src: add snapshot support for embedder API

[addaleax]

The first commits here are #45885 + #45886 + #45887.

src: add snapshot support for embedder API

Add experimental support for loading snapshots in the embedder API
by adding a public opaque wrapper for our SnapshotData struct and
allowing embedders to pass it to the relevant setup functions.
Where applicable, use these helpers to deduplicate existing code
in Node.js’s startup path.

This has shown a 40 % startup performance increase for a real-world
application, even with the somewhat limited current support for
built-in modules.

The documentation includes a note about no guarantees for API or
ABI stability for this feature while it is experimental.

[nodejs-github-bot]

Review requested:

• @nodejs/gyp
• @nodejs/startup

[joyeecheung]

👍 to see this happening!

Before I dive deeper into the code: the snapshot blob we currently have is really a per-process thing, it contains a template for the vm contexts, a template for creating worker instances, and a template for creating the main instance. For now it probably makes more sense to make it a parameter to something like InitializeOncePerProcess() and then add some flags to CommonEnvironmentSetup to instruct whether Node.js should use this blob internally to create the new Node.js environments (ditto for NewContext or maybe also NewIsolate). It probably makes more sense to reserve isolate-specific snapshot data for embeeder API support of user land snapshots.

[addaleax]

@joyeecheung Aren't snapshots really a per-Isolate concept, though? What would be the downside of embedders potentially passing two different snapshots to different Isolates, for example?

It probably makes more sense to reserve isolate-specific snapshot data for embeeder API support of user land snapshots.

Just to be clear, this PR does allow embedders to consume snapshots built with node --build-snapshot -- that is userland snapshots, right?

[joyeecheung]

Aren't snapshots really a per-Isolate concept, though?

Yes, and our SnapshotData is...currently not. That's a TODO.

What would be the downside of embedders potentially passing two different snapshots to different Isolates, for example?

Because they'd be passing two snapshots blobs that are actually not of the same structure. Currently they are passing a pointer to a per-process snapshot blob to a per-isolate API, so if we add per-isolate snapshots later (which is probably a building block of the per-process snapshot), we need to figure out which type the blob actually is, it's also somewhat confusing the there is one EmbedderSnapshotData type that can both be for a per-isolate snapshot, or for a per-process snapshot that contains multiple per-isolate snapshots, it's better to make that clearer with just two different types (and for now the per-isolate one isn't a thing yet, so just a per-process one for InitializeOncePerProcess() makes sense).

that is userland snapshots, right?

Yes and...no. It's user-land snapshot but it's not per-isolate, so we build it as a user-land snapshot, and consume it as a built-in snapshot, which is fine by now because we technically only support one isolate (the one of the main instance) in the snapshot and there is currently no way for users to pass any more snapshots into any other isolate that might be in the process.

this PR does allow embedders to consume snapshots built with node --build-snapshot

That's actually where the problem is because technically you'd want to pass a snapshot data blob to ComomnEvnrionmentSetup that's produced by something like node::SnapshotFromEnvironment(isolate). The blob produced by node --build-snapshot is per-process, and therefore it should be consumed by a per-process API.

[addaleax]

Aren't snapshots really a per-Isolate concept, though?

Yes, and our SnapshotData is...currently not. That's a TODO.

So ... let me rephrase things a bit: Currently, the Node.js snapshots, both builtin and userland, are per-Node.js-Isolate-tree, where "Node.js Isolate tree" includes a main Isolate and (at least potentially) the Isolates of worker threads spawned from that main Isolate. Embedders don't really know about those "child" Isolates, so from their perspective, the snapshot is per-Isolate -- that's what I mean here. It probably makes sense to be more specific with the wording here. But as there is no true per-process data associated with snapshots right now (and I don't think there could ever really be, given their nature), I don't think it makes sense to say they are per-process.

it's better to make that clearer with just two different types (and for now the per-isolate one isn't a thing yet, so just a per-process one for InitializeOncePerProcess() makes sense).

It's always easy enough to distinguish different types of snapshots from this API starting point -- there could just be two different subclasses of EmbedderSnapshotData, for example. I don't really think that practically speaking, they would be passed and used differently, though.

InitializeOncePerProcess() is really for setting up attributes of the process so that it is usable for creating Node.js environments though. That does not seem to be the right place to set up something like this at all.

that is userland snapshots, right?

Yes and...no. It's user-land snapshot but it's not per-isolate, so we build it as a user-land snapshot, and consume it as a built-in snapshot, which is fine by now because we technically only support one isolate (the one of the main instance) in the snapshot and there is currently no way for users to pass any more snapshots into any other isolate that might be in the process.

Then maybe it could help to clarify, what is the eventual plan for user-land snapshots (from a non-embedder point of view) here? In a non-embedder context, users can only create new Isolates through the Worker API or addons, but it doesn't seem to me like those would lead to a deviation in APIs for embedders.

this PR does allow embedders to consume snapshots built with node --build-snapshot

That's actually where the problem is because technically you'd want to pass a snapshot data blob to ComomnEvnrionmentSetup that's produced by something like node::SnapshotFromEnvironment(isolate).

I think that might be useful in the future, yes.

The blob produced by node --build-snapshot is per-process, and therefore it should be consumed by a per-process API.

As mentioned above -- it does seem to be that both of these things are really per-Node.js-Isolate-tree, not per-process.

[joyeecheung]

So ... let me rephrase things a bit: Currently, the Node.js snapshots, both builtin and userland, are per-Node.js-Isolate-tree, where "Node.js Isolate tree" includes a main Isolate and (at least potentially) the Isolates of worker threads spawned from that main Isolate. Embedders don't really know about those "child" Isolates, so from their perspective, the snapshot is per-Isolate -- that's what I mean here. It probably makes sense to be more specific with the wording here. But as there is no true per-process data associated with snapshots right now (and I don't think there could ever really be, given their nature), I don't think it makes sense to say they are per-process.

OK, I think it makes a bit more sense if we conceptualize the snapshot as per-isolate-tree. Are we supposed to support multiple isolate trees though? I don't think the snapshot is really ready for that yet...

BTW, --build-snapshot and --snapshot-blob are still in PerProcessOptions, they should probably be moved to EnvironmentOptions, if the plan is to make them the way to get snapshots in/out. Another unsafe area is the BuiltinLoader because it expects that there is only one snapshot used in the process, and uses the code cache that's compatible with that snapshot. If there are multiple snapshots used in the process, the BuiltinLoader for these different isolate trees need to be isolated (right now it's shared across the entire process) as the code cache from different snapshot might be incompatible and lead to crashes (unless shared-RO-heap is turned on).

Then maybe it could help to clarify, what is the eventual plan for user-land snapshots (from a non-embedder point of view) here? In a non-embedder context, users can only create new Isolates through the Worker API or addons, but it doesn't seem to me like those would lead to a deviation in APIs for embedders.

Is there any plan to support creating workers in the embedder API? Or are the embedders only supposed to build workers on top of existing APIs?

I think the snapshot support in the JS-land worker API should be blob-based, not file-based, as that's programmable unlike the startup of the main instance, and technically, the embedder API should be blob-based too, as that's also programmable. The intermediate file seems to be a weird distraction to me, we only do that for the main instance because when you start Node.js from the command line there's not really a better alternative to specify the blob.

[addaleax]

Are we supposed to support multiple isolate trees though? I don't think the snapshot is really ready for that yet...

I guess the question would be, in what ways are those actually interdependent? As long as there is no shared global state that is affected, this should be fine.

BTW, --build-snapshot and --snapshot-blob are still in PerProcessOptions, they should probably be moved to EnvironmentOptions, if the plan is to make them the way to get snapshots in/out.

That’s not necessarily my plan… we can do it, and I don’t really see anything speak against it. At the same time, for regular users, I think you already brought up better alternatives: For Workers, there should likely be a more programmatic approach than CLI flags, and for embedders, creating a snapshot through an API method may be convenient.

For the application that this patch was evaluated against, it seems like a perfectly fine approach to build a snapshot with a regular Node.js binary (using the same version and configure flags of course), then compile Node.js again with the generated snapshot embedded.

Another unsafe area is the BuiltinLoader because it expects that there is only one snapshot used in the process, and uses the code cache that's compatible with that snapshot. If there are multiple snapshots used in the process, the BuiltinLoader for these different isolate trees need to be isolated (right now it's shared across the entire process) as the code cache from different snapshot might be incompatible and lead to crashes (unless shared-RO-heap is turned on).

Great point – I’m happy to provide a PR for making the BuiltinLoader a non-global object and instead just something that is shared across an Environment and its Workers, if that sounds good to you? That shouldn’t really have any impact on non-embedder users that way. (That would also finally make it easier to resolve

node/src/api/environment.cc

Line 460 in b66ae39

// TODO(addaleax): Avoid having a global table for all scripts.

, which I guess is already a little unsafe in this regard…)

Is there any plan to support creating workers in the embedder API? Or are the embedders only supposed to build workers on top of existing APIs?

Well, Workers are already kind of embedded Node.js instances… :) I don’t see any reason to change the embedder API in this regard.

The intermediate file seems to be a weird distraction to me, we only do that for the main instance because when you start Node.js from the command line there's not really a better alternative to specify the blob.

Totally agree – the main reason I kept using FILE* here is that a) this is perfectly fine for testing since fmemopen() exists (and can at least be kind-of-polyfilled on Windows) and b) I didn’t want to build too many changes on top of each other, but it would be nice if this was eventually something that didn’t require a FILE* object.

[addaleax]

@joyeecheung Fwiw, I’ve opened #45942 to take us one further step there regarding the BuiltinLoader class. I’ll update this PR if/when that is merged, assuming that we’ll want to do that one first.

[joyeecheung]

I’m happy to provide a PR for making the BuiltinLoader a non-global object and instead just something that is shared across an Environment and its Workers, if that sounds good to you?

SGTM, I don't think that's necessarily a blocker for this PR though if the APIs here assert that the embedders cannot try to create isolates from multiple Node.js snapshots in the same process (otherwise, that's unsafe without isolation of the builtin code cache).

I feel more strongly about the file based API though....it just seems weird to me that the users would use a per-process CLI flag to generate the blob and them consume it from the API with a File*, it might've made a bit more sense even if the blob generation is also done with a File* instead of a flag? If it's not too much work, I'd prefer that we at least introduce an API that allows users to create an environment with the intent to snapshot it, and an API to EmbedderSnapshotData blob to write it a File* (which is probably just a wrapper of SnapshotData::ToBlob which already takes a File*). Then the fact that the output of the per-process --build-snapshot works with EmbedderSnapshotData::FromFile would be more like just an accident, but not anything that we actually guarantee. And also that seems less weird since the since as you mentioned File* isn't necessarily backed by an actual file :)

[addaleax]

If it's not too much work, I'd prefer that we at least introduce an API that allows users to create an environment with the intent to snapshot it, and an API to EmbedderSnapshotData blob to write it a File* (which is probably just a wrapper of SnapshotData::ToBlob which already takes a File*). Then the fact that the output of the per-process --build-snapshot works with EmbedderSnapshotData::FromFile would be more like just an accident, but not anything that we actually guarantee. And also that seems less weird since the since as you mentioned File* isn't necessarily backed by an actual file :)

@joyeecheung Sooo … I did just rebase this PR with what is essentially what you are suggesting, in a way that I don’t think requires adding too much new code, see d2746af. This doesn’t currently work, because compiling the main script in the embedder test uses the code path that compiles it as a builtin, rather than what the internal/main/mksnapshot module currently does (and consequently fails serialization because of that) … I think one possible way to work around this would be to require the embedder to compile and run scripts directly, but I wanted to check with you that a) this approach still sounds good to you in general and b) to what degree we should expose the functionality of the internal/main/mksnapshot here (e.g. should all code in it always run when snapshotting, with the possibility of using a C++ function instead of serializeMainFunction?).

[joyeecheung]

@addaleax IIUC the issue is essentially that the snapshot preparation process is done in LoadEnvironment() instead of CreateEnvironment()? In that case can we just make that also part of the embedder API? i.e. if you want to build a snapshot, it should eventually come out of LoadEnvironment(), or some function that can only be called after the load, which conceptually makes sense, because the user might very well want to run the event loop when building the snapshot (e.g. if the snapshot building process involves asynchronous operations that can actually be finished before the snapshot is taken).

[danielleadams]

@addaleax do you mind opening a backport PR for this to land in v18? Thanks!