Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

src: add snapshot support for embedder API #45888

Closed
wants to merge 7 commits into from

Conversation

addaleax
Copy link
Member

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.

@addaleax addaleax added c++ Issues and PRs that require attention from people who are familiar with C++. semver-minor PRs that contain new features and should be released in the next minor version. experimental Issues and PRs related to experimental features. embedding Issues and PRs related to embedding Node.js in another project. snapshot Issues and PRs related to the startup snapshot labels Dec 16, 2022
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Dec 16, 2022
@joyeecheung
Copy link
Member

joyeecheung commented Dec 16, 2022

👍 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
Copy link
Member Author

@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
Copy link
Member

joyeecheung commented Dec 16, 2022

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
Copy link
Member Author

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
Copy link
Member

joyeecheung commented Dec 21, 2022

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
Copy link
Member Author

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

// 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 added a commit to addaleax/node that referenced this pull request Dec 22, 2022
As discussed in nodejs#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.
@addaleax
Copy link
Member Author

@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
Copy link
Member

joyeecheung commented Dec 22, 2022

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 addaleax added the blocked PRs that are blocked by other issues or PRs. label Jan 6, 2023
addaleax added a commit to addaleax/node that referenced this pull request Jan 6, 2023
As discussed in nodejs#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.
addaleax added a commit to addaleax/node that referenced this pull request Jan 10, 2023
As discussed in nodejs#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.
addaleax added a commit to addaleax/node that referenced this pull request Jan 17, 2023
As discussed in nodejs#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.
nodejs-github-bot pushed a commit that referenced this pull request Jan 18, 2023
As discussed in #45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: #45942
Reviewed-By: Joyee Cheung <[email protected]>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
As discussed in #45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: #45942
Reviewed-By: Joyee Cheung <[email protected]>
@addaleax
Copy link
Member Author

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?).

@addaleax addaleax marked this pull request as draft January 24, 2023 20:48
@joyeecheung
Copy link
Member

@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).

nodejs-github-bot pushed a commit that referenced this pull request Feb 3, 2023
PR-URL: #45888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
@addaleax addaleax deleted the embedder-snapshot-api branch February 3, 2023 21:43
nodejs-github-bot pushed a commit that referenced this pull request Feb 6, 2023
This is a shared follow-up to 06bb6b4 and a466fea
now that both have been merged.

PR-URL: #46491
Refs: #45888
Refs: #46463
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 6, 2023
nodejs#45888 took the environment
creation code out of the scope covered by the v8::TryCatch
that we use to print early failures during environment creation.
So e.g. when adding something that would fail in node.js, we get

```
node:internal/options:554: Uncaught Error: Should not query options before bootstrapping is done
```

This patch restores that by adding another v8::TryCatch for it:

```
node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:45:19)
    at node:internal/bootstrap/node:433:29
```
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
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.

PR-URL: #45888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
PR-URL: #45888
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
This is a shared follow-up to 06bb6b4 and a466fea
now that both have been merged.

PR-URL: #46491
Refs: #45888
Refs: #46463
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
MylesBorins added a commit that referenced this pull request Feb 19, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: TODO
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
MylesBorins added a commit that referenced this pull request Feb 20, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit to MylesBorins/node that referenced this pull request Feb 20, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) nodejs#46673
  * add ada as a dependency (Yagiz Nizipli) nodejs#46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) nodejs#46716
  * add deokjinkim to collaborators (Deokjin Kim) nodejs#46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) nodejs#46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) nodejs#46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) nodejs#45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) nodejs#46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) nodejs#46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) nodejs#45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) nodejs#45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) nodejs#45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) nodejs#46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) nodejs#46273
test_runner:
  * add initial code coverage support (Colin Ihrig) nodejs#46017
url:
  * replace url-parser with ada (Yagiz Nizipli) nodejs#46410

PR-URL: nodejs#46725
MylesBorins added a commit that referenced this pull request Feb 20, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit that referenced this pull request Feb 20, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit that referenced this pull request Feb 20, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
MylesBorins added a commit that referenced this pull request Feb 21, 2023
Notable changes:

deps:
  * upgrade npm to 9.5.0 (npm team) #46673
  * add ada as a dependency (Yagiz Nizipli) #46410
doc:
  * add debadree25 to collaborators (Debadree Chatterjee) #46716
  * add deokjinkim to collaborators (Deokjin Kim) #46444
doc,lib,src,test:
  * rename --test-coverage (Colin Ihrig) #46017
lib:
  * (SEMVER-MINOR) add aborted() utility function (Debadree Chatterjee) #46494
src:
  * (SEMVER-MINOR) add initial support for single executable applications (Darshan Sen) #45038
  * (SEMVER-MINOR) allow optional Isolate termination in node::Stop() (Shelley Vohr) #46583
  * (SEMVER-MINOR) allow blobs in addition to `FILE*`s in embedder snapshot API (Anna Henningsen) #46491
  * (SEMVER-MINOR) allow snapshotting from the embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) make build_snapshot a per-Isolate option, rather than a global one (Anna Henningsen) #45888
  * (SEMVER-MINOR) add snapshot support for embedder API (Anna Henningsen) #45888
  * (SEMVER-MINOR) allow embedder control of code generation policy (Shelley Vohr) #46368
stream:
  * (SEMVER-MINOR) add abort signal for ReadableStream and WritableStream (Debadree Chatterjee) #46273
test_runner:
  * add initial code coverage support (Colin Ihrig) #46017
url:
  * replace url-parser with ada (Yagiz Nizipli) #46410

PR-URL: #46725
legendecas pushed a commit that referenced this pull request Feb 28, 2023
#45888 took the environment
creation code out of the scope covered by the v8::TryCatch
that we use to print early failures during environment creation.
So e.g. when adding something that would fail in node.js, we get

```
node:internal/options:554: Uncaught Error: Should not query options before bootstrapping is done
```

This patch restores that by adding another v8::TryCatch for it:

```
node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:45:19)
    at node:internal/bootstrap/node:433:29
```

PR-URL: #46533
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Mar 13, 2023
#45888 took the environment
creation code out of the scope covered by the v8::TryCatch
that we use to print early failures during environment creation.
So e.g. when adding something that would fail in node.js, we get

```
node:internal/options:554: Uncaught Error: Should not query options before bootstrapping is done
```

This patch restores that by adding another v8::TryCatch for it:

```
node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:45:19)
    at node:internal/bootstrap/node:433:29
```

PR-URL: #46533
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Mar 14, 2023
#45888 took the environment
creation code out of the scope covered by the v8::TryCatch
that we use to print early failures during environment creation.
So e.g. when adding something that would fail in node.js, we get

```
node:internal/options:554: Uncaught Error: Should not query options before bootstrapping is done
```

This patch restores that by adding another v8::TryCatch for it:

```
node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:45:19)
    at node:internal/bootstrap/node:433:29
```

PR-URL: #46533
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@danielleadams danielleadams added the backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. label Apr 2, 2023
@danielleadams
Copy link
Contributor

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

danielleadams pushed a commit that referenced this pull request Apr 11, 2023
#45888 took the environment
creation code out of the scope covered by the v8::TryCatch
that we use to print early failures during environment creation.
So e.g. when adding something that would fail in node.js, we get

```
node:internal/options:554: Uncaught Error: Should not query options before bootstrapping is done
```

This patch restores that by adding another v8::TryCatch for it:

```
node:internal/options:20
    ({ options: optionsMap } = getCLIOptions());
                               ^

Error: Should not query options before bootstrapping is done
    at getCLIOptionsFromBinding (node:internal/options:20:32)
    at getOptionValue (node:internal/options:45:19)
    at node:internal/bootstrap/node:433:29
```

PR-URL: #46533
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
targos pushed a commit that referenced this pull request Nov 10, 2023
As discussed in #45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: #45942
Reviewed-By: Joyee Cheung <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
As discussed in nodejs/node#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: nodejs/node#45942
Reviewed-By: Joyee Cheung <[email protected]>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
As discussed in nodejs/node#45888, using a
global `BuiltinLoader` instance is probably undesirable in a world
in which embedders are able to create Node.js Environments with
different sources and therefore mutually incompatible code
caching properties.

This PR makes it so that `BuiltinLoader` is no longer a global
singleton and instead only shared between `Environment`s that
have a direct relation to each other, and addresses a few
thread safety issues along with that.

PR-URL: nodejs/node#45942
Reviewed-By: Joyee Cheung <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v18.x PRs awaiting manual backport to the v18.x-staging branch. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. embedding Issues and PRs related to embedding Node.js in another project. experimental Issues and PRs related to experimental features. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. semver-minor PRs that contain new features and should be released in the next minor version. snapshot Issues and PRs related to the startup snapshot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants