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

process: run RunBootstrapping in CreateEnvironment #26788

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Mar 19, 2019

Also creates CreateMainEnvironment to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.

This should be easier to review with https://github.com/nodejs/node/pull/26788/files?w=1

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Mar 19, 2019

// TODO(addaleax): This should load a real per-Isolate option, currently
// this is still effectively per-process.
if (isolate_data->options()->track_heap_objects) {
Copy link
Member Author

@joyeecheung joyeecheung Mar 19, 2019

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.

@joyeecheung
Copy link
Member Author

@joyeecheung
Copy link
Member Author

Resume: https://ci.nodejs.org/job/node-test-pull-request/21692/

cc @addaleax @nodejs/process

@joyeecheung
Copy link
Member Author

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?

@joyeecheung joyeecheung force-pushed the create-env branch 2 times, most recently from 04d2c6e to cb956fc Compare March 22, 2019 02:49
@joyeecheung
Copy link
Member Author

joyeecheung commented Mar 22, 2019

Rebased and added a environment.js bootstrap script for CreateEnvironment to run pre-exeuction. @addaleax Can you take another look?

CI: https://ci.nodejs.org/job/node-test-pull-request/21753/ https://ci.nodejs.org/job/node-test-pull-request/21774/

return nullptr;
}

std::vector<Local<String>> parameters = {
Copy link
Member Author

@joyeecheung joyeecheung Mar 22, 2019

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:

  1. Up to node.js: no async operation is allowed, no access to run time states
  2. environment.js: no async operation is allowed, but access to run time states is allowed (warnings emitted here must be done synchronously)
  3. 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.

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

https://ci.nodejs.org/job/node-test-pull-request/21934/

@addaleax this changed quite a bit since last reviews (mostly, CreateEnvironment now runs prepareMainThreadExecution). Can you take a look again? Thanks!

@BridgeAR
Copy link
Member

@joyeecheung seems like this needs another rebase :/

Also creates `CreateMainEnvironment` to encapsulate the code
creating the main environment from the provided Isolate data
and arguments.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 1, 2019
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

If there are no more reviews I am going to land this later today.

@joyeecheung
Copy link
Member Author

Landed in 1944265

@joyeecheung joyeecheung closed this Apr 3, 2019
cjihrig pushed a commit to cjihrig/node that referenced this pull request Apr 3, 2019
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 4, 2019
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
BethGriggs pushed a commit that referenced this pull request Apr 9, 2019
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Signed-off-by: Beth Griggs <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Apr 9, 2019
BethGriggs pushed a commit that referenced this pull request Apr 10, 2019
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Signed-off-by: Beth Griggs <[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. c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants