bootstrap: reorganize the bootstrap process #27539

joyeecheung · 2019-05-02T16:49:53Z

This patch reorganizes the bootstrap process so it's easier
to snapshot it incrementally.

src: inline ProcessCliArgs in the Environment constructor

Inline ProcessCliArgs() in the Environment constructor, and
emit the Environment creation trace events with the arguments
earlier. Remove the unused arguments passed to CreateProcessObject()
since these are now attached to process in PatchProcessObject()
during pre-execution instead.

src: create Environment properties in Environment::CreateProperties()

Move creation of env->as_callback_data(), env->primordials()
and env->process() into Environment::CreateProperties() and
call it in the Environment constructor - this can be replaced with
deserialization when we snapshot the per-environment properties
after the instantiation of Environment.

src: reorganize inspector and diagnostics initialization

Split the initialization of the inspector and other diagnostics
into Environment::InitializeInspector() and
Environment::InitializeDiagnostics() - these need to be
reinitialized separately after snapshot deserialization.
Do not store worker url alongside the inspector parent handle,
instead just get it from the handle.
Rename Worker::profiler_idle_notifier_started_ to
Worker::start_profiler_idle_notifier_ because it stores
the state inherited from the parent env to use for initializing
itself.

src: Split `RunBootstrapping()`

Split RunBootstrapping() into BootstrapInternalLoaders()
and BootstrapNode() from so the two can be snapshotted
incrementally.

Checklist

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

nodejs-github-bot · 2019-05-02T23:10:12Z

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

joyeecheung · 2019-05-03T06:39:22Z

cc @nodejs/process

joyeecheung · 2019-05-07T07:05:32Z

Based on the GitHub reviewer suggestions...@addaleax @bnoordhuis @jasnell can I have some review please?

joyeecheung · 2019-05-09T13:43:57Z

Ping...I would like to have this landed first before moving to snapshot more of the bootstrap process (it won't be possible without changing the order of inspector and event loop initialization, really)

Fishrock123 · 2019-05-10T19:47:27Z

Can this be broken down into multiple commits? That might make review easier...

Fishrock123 · 2019-05-14T18:06:08Z

Ok, so I try not to mention this because it's what we (NodeSource) have signed up for in maintaining our own node fork/runtime (N|Solid)... however for Node 12 so much of the startup and related internals have been moved around so much that the rebase pain is quite immense. 😓

Are we really gaining much from doing these changes...? (I mean, this PR is particular..)

joyeecheung · 2019-05-14T19:20:17Z

@Fishrock123 This is required for including these parts in the v8 snapshot, see the design doc on the constraints the Environment needs to follow in order for it to be snapshottable.

I think I've laid out the reasons for doing these in the PR message clearly enough, but to explain further:

We need to deserialize the env->props() properties from the snapshot instead of creating them from scratch. It would be easier if the creation of them is separated in a method and then you'll be able to control when these properties are created all together.
The snapshot creator discards inspector states as well as a bunch of other per-isolate callbacks. We need to reinstall them after the environment is deserialized. So these are put into separate methods.
Similarly it would be easier to snapshot loaders.js and node.js incrementally if we split the invocation of them into separate methods.

On how this refactoring is used, see this for a prototype. The alternative to refactoring is..well, copy pasting all these code for the snapshot creator, and adding a bunch of deserialization in the middle, like what was done in the initial prototype of the RFC. That would not be maintainable, and may not really help with rebasing as there would still be conflicts with deserialization code.

joyeecheung · 2019-05-14T19:28:42Z

By the way, from my experience maintaining a similar fork in my previous job, one way to make the rebasing easier is instead of rebasing, squash the changes into commits that make sense, and cherry-picking them onto the upstream branches. This is especially important for paths like the bootstrap, because it has been quite messy - most things were done in a certain order for no particularly good reason but simply because it worked. The downstream patches likely follow the same kind of reasoning when they add stuff in the middle, so once someone tries to make the upstream more organized, there would be a lot of conflicts since the patches still relied on that disorganized state of things.

It would be easier to bring the two together if the downstream patches encapsulate what they need to do in a few method invocations (with implementation details written in separate files to reduce conflicts), and when being cherry-picked onto the upstream, just insert the invocation to the new places where it makes more sense for these to happen. The worst that could happen is when you implement things right inside the upstream files in the middle of the code that's being refactored, and the downstream commits are mixed in the history with the upstream commits. Then it would very difficult to rebase once a refactored branch comes out (I had worked with that kind of git states before so I know how painful it could be). Although once the upstream starts to become organized, it tend to be easier to merge the two from then on because the patches are then inserted into logical places that tend to stabalize.

Inline `ProcessCliArgs()` in the `Environment` constructor, and emit the `Environment` creation trace events with the arguments earlier. Remove the unused arguments passed to `CreateProcessObject()` since these are now attached to process in `PatchProcessObject()` during pre-execution instead.

Move creation of `env->as_callback_data()`, `env->primordials()` and `env->process()` into `Environment::CreateProperties()` and call it in the `Environment` constructor - this can be replaced with deserialization when we snapshot the per-environment properties after the instantiation of `Environment`.

- Split the initialization of the inspector and other diagnostics into `Environment::InitializeInspector()` and `Environment::InitializeDiagnostics()` - these need to be reinitialized separately after snapshot deserialization. - Do not store worker url alongside the inspector parent handle, instead just get it from the handle. - Rename `Worker::profiler_idle_notifier_started_` to `Worker::start_profiler_idle_notifier_` because it stores the state inherited from the parent env to use for initializing itself.

Split `RunBootstrapping()` into `BootstrapInternalLoaders()` and `BootstrapNode()` from so the two can be snapshotted incrementally.

joyeecheung · 2019-05-20T13:18:27Z

@Fishrock123 @addaleax @bnoordhuis @jasnell I split the PR into multiple commits, the descriptions are updated in the OP. PTAL, thanks!

nodejs-github-bot · 2019-05-25T07:10:56Z

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

joyeecheung · 2019-05-27T10:52:43Z

Pinging for reviews again...this has been open for over three weeks without any reviews

src/env.cc

src/node_worker.cc

…rties()

nodejs-github-bot · 2019-05-31T15:33:42Z

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

addaleax · 2019-05-31T22:13:32Z

src/node.cc

-  Isolate* isolate = env->isolate();
-  Local<Context> context = env->context();
+  isolate_->GetHeapProfiler()->AddBuildEmbedderGraphCallback(
+      Environment::BuildEmbedderGraph, this);


This is dependent on #if HAVE_INSPECTOR now, but you can do heap dumps without the inspector, so should we maybe not do this here? (Sorry that I’m bringing it up again 😄)

An right, maybe it also makes sense to move it back to InitializeDiagnostics and then we invoke InitializeDiagnostics before InitializeInspector. The other part of InitializeDiagnostics is adding GC callbacks for dtrace, I think it also make sense to do that before starting the inspector agent.

nodejs-github-bot · 2019-06-02T13:04:23Z

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

nodejs-github-bot · 2019-06-02T16:33:12Z

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

joyeecheung · 2019-06-03T15:33:30Z

Landed in f0018a5...0778599

Inline `ProcessCliArgs()` in the `Environment` constructor, and emit the `Environment` creation trace events with the arguments earlier. Remove the unused arguments passed to `CreateProcessObject()` since these are now attached to process in `PatchProcessObject()` during pre-execution instead. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Move creation of `env->as_callback_data()`, `env->primordials()` and `env->process()` into `Environment::CreateProperties()` and call it in the `Environment` constructor - this can be replaced with deserialization when we snapshot the per-environment properties after the instantiation of `Environment`. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

- Split the initialization of the inspector and other diagnostics into `Environment::InitializeInspector()` and `Environment::InitializeDiagnostics()` - these need to be reinitialized separately after snapshot deserialization. - Do not store worker url alongside the inspector parent handle, instead just get it from the handle. - Rename `Worker::profiler_idle_notifier_started_` to `Worker::start_profiler_idle_notifier_` because it stores the state inherited from the parent env to use for initializing itself. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Split `RunBootstrapping()` into `BootstrapInternalLoaders()` and `BootstrapNode()` from so the two can be snapshotted incrementally. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Inline `ProcessCliArgs()` in the `Environment` constructor, and emit the `Environment` creation trace events with the arguments earlier. Remove the unused arguments passed to `CreateProcessObject()` since these are now attached to process in `PatchProcessObject()` during pre-execution instead. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Move creation of `env->as_callback_data()`, `env->primordials()` and `env->process()` into `Environment::CreateProperties()` and call it in the `Environment` constructor - this can be replaced with deserialization when we snapshot the per-environment properties after the instantiation of `Environment`. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

- Split the initialization of the inspector and other diagnostics into `Environment::InitializeInspector()` and `Environment::InitializeDiagnostics()` - these need to be reinitialized separately after snapshot deserialization. - Do not store worker url alongside the inspector parent handle, instead just get it from the handle. - Rename `Worker::profiler_idle_notifier_started_` to `Worker::start_profiler_idle_notifier_` because it stores the state inherited from the parent env to use for initializing itself. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

Split `RunBootstrapping()` into `BootstrapInternalLoaders()` and `BootstrapNode()` from so the two can be snapshotted incrementally. PR-URL: #27539 Reviewed-By: Anna Henningsen <anna@addaleax.net>

This comment has been minimized.

Sign in to view

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels May 2, 2019

This comment has been minimized.

Sign in to view

joyeecheung added the review wanted PRs that need reviews. label May 9, 2019

joyeecheung added 4 commits May 20, 2019 13:06

src: Split RunBootstrapping()

8d400df

Split `RunBootstrapping()` into `BootstrapInternalLoaders()` and `BootstrapNode()` from so the two can be snapshotted incrementally.

joyeecheung force-pushed the refactor-bootstrap-cpp branch from ca713c3 to 8d400df Compare May 20, 2019 13:16

addaleax reviewed May 31, 2019

View reviewed changes

src/env.cc Outdated Show resolved Hide resolved

src/env.cc Show resolved Hide resolved

src/node_worker.cc Show resolved Hide resolved

fixup! src: create Environment properties in Environment::CreatePrope…

1de0c1a

…rties()

addaleax approved these changes May 31, 2019

View reviewed changes

fixup! src: reorganize inspector and diagnostics initialization

bd13830

joyeecheung force-pushed the refactor-bootstrap-cpp branch from 8b92cc7 to bd13830 Compare June 1, 2019 23:26

joyeecheung closed this Jun 3, 2019

BridgeAR mentioned this pull request Jun 17, 2019

v12.5.0 proposal #28268

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bootstrap: reorganize the bootstrap process #27539

bootstrap: reorganize the bootstrap process #27539

joyeecheung commented May 2, 2019 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 2, 2019

joyeecheung commented May 3, 2019

joyeecheung commented May 7, 2019

joyeecheung commented May 9, 2019

Fishrock123 commented May 10, 2019

Fishrock123 commented May 14, 2019

joyeecheung commented May 14, 2019 •

edited

Loading

joyeecheung commented May 14, 2019 •

edited

Loading

joyeecheung commented May 20, 2019

nodejs-github-bot commented May 25, 2019

joyeecheung commented May 27, 2019 •

edited

Loading

nodejs-github-bot commented May 31, 2019

addaleax May 31, 2019

joyeecheung Jun 1, 2019

nodejs-github-bot commented Jun 2, 2019

nodejs-github-bot commented Jun 2, 2019

joyeecheung commented Jun 3, 2019 •

edited

Loading

bootstrap: reorganize the bootstrap process #27539

bootstrap: reorganize the bootstrap process #27539

Conversation

joyeecheung commented May 2, 2019 • edited Loading

src: inline ProcessCliArgs in the Environment constructor

src: create Environment properties in Environment::CreateProperties()

src: reorganize inspector and diagnostics initialization

src: Split RunBootstrapping()

Checklist

This comment has been minimized.

This comment has been minimized.

nodejs-github-bot commented May 2, 2019

joyeecheung commented May 3, 2019

joyeecheung commented May 7, 2019

joyeecheung commented May 9, 2019

Fishrock123 commented May 10, 2019

Fishrock123 commented May 14, 2019

joyeecheung commented May 14, 2019 • edited Loading

joyeecheung commented May 14, 2019 • edited Loading

joyeecheung commented May 20, 2019

nodejs-github-bot commented May 25, 2019

joyeecheung commented May 27, 2019 • edited Loading

nodejs-github-bot commented May 31, 2019

addaleax May 31, 2019

Choose a reason for hiding this comment

joyeecheung Jun 1, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Jun 2, 2019

nodejs-github-bot commented Jun 2, 2019

joyeecheung commented Jun 3, 2019 • edited Loading

joyeecheung commented May 2, 2019 •

edited

Loading

src: Split `RunBootstrapping()`

joyeecheung commented May 14, 2019 •

edited

Loading

joyeecheung commented May 14, 2019 •

edited

Loading

joyeecheung commented May 27, 2019 •

edited

Loading

joyeecheung commented Jun 3, 2019 •

edited

Loading