process: run RunBootstrapping in CreateEnvironment by joyeecheung · Pull Request #26788 · nodejs/node (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation15 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Issues and PRs that require attention from people who are familiar with C++.
label
| // TODO(addaleax): This should load a real per-Isolate option, currently |
| // this is still effectively per-process. |
| if (isolate_data->options()->track_heap_objects) { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My thinking was when we implement snapshot, this could be where the context is deserialized from the snapshot (as opposed to using NewContext). Note that to deserialize the snapshot properly we need to reinstall per-isolate callbacks, including the ones installed in SetIsolateUpForNode, so we may need to stop using NewIsolate at that point. We also need to restore inspector states after deserialization as those are discarded when the snapshot is generated, so it makes sense to me to encapsulate all this together.
Cross referencing: #26334 (comment)
This detaches bootstraping and pre-execution in our C++ API, so CreateEnvironment actually provides an incomplete Node.js context. I am not sure if that's desirable, maybe we should also run a script that runs through parts of pre-exeuction for CreateEnvironment() instead? If so what should be run?
| return nullptr; |
|---|
| } |
| std::vector<Local> parameters = { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are splitting RunBootstrapping and StartExecution, this requires the user not to pass any arguments that result in an async operation done in pre-execution at the moment. This is not ideal, but we may be able to split what's done in environment.js in the future to make sure nothing async is done during that...then the bootstrap will be divided into three phases:
- Up to node.js: no async operation is allowed, no access to run time states
- environment.js: no async operation is allowed, but access to run time states is allowed (warnings emitted here must be done synchronously)
- pre_execution.js: both are allowed, but the embedder would have to call this themselves somehow and make sure it plays with how they run the event loops.
Also creates CreateMainEnvironment to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.
PRs that have at least one approval, no pending requests for changes, and a CI started.
label
If there are no more reviews I am going to land this later today.
cjihrig pushed a commit to cjihrig/node that referenced this pull request
Also creates CreateMainEnvironment to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.
PR-URL: nodejs#26788 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Refael Ackermann refack@gmail.com
BethGriggs pushed a commit that referenced this pull request
Also creates CreateMainEnvironment to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.
PR-URL: #26788 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Refael Ackermann refack@gmail.com
BethGriggs pushed a commit that referenced this pull request
Also creates CreateMainEnvironment to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.
PR-URL: #26788 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Refael Ackermann refack@gmail.com Signed-off-by: Beth Griggs Bethany.Griggs@uk.ibm.com
BethGriggs pushed a commit that referenced this pull request
Also creates CreateMainEnvironment to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.
PR-URL: #26788 Reviewed-By: Anna Henningsen anna@addaleax.net Reviewed-By: Refael Ackermann refack@gmail.com Signed-off-by: Beth Griggs Bethany.Griggs@uk.ibm.com
This was referenced
Feb 22, 2020
Labels
PRs that have at least one approval, no pending requests for changes, and a CI started.
Issues and PRs that require attention from people who are familiar with C++.