src: bootstrap internals loaders before bootstrapping Node.js #19112

joyeecheung · 2018-03-03T22:00:00Z

Moves the creation of process.binding(), process._linkedBinding()
internalBinding() and NativeModule into a separate file
lib/internal/bootstrap_loaders.js, and documents them there.
This file will be compiled and run before bootstrap_node.js, which
means we now bootstrap the internal module & binding system before
actually bootstrapping Node.js.
Rename the special ID that can be used to require NativeModule
as internal/bootstrap_loaders since it is setup there. Also put
internalBinding in the object exported by NativeModule.require
instead of putting it inside the NativeModule.wrapper
Names the bootstrapping functions so their names show up
in the stack trace.

Checklist

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

Affected core subsystem(s)

process, modules

joyeecheung · 2018-03-03T22:05:52Z

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

joyeecheung · 2018-03-03T22:14:18Z

lib/internal/native_module.js

+    };
+  }
+
+  // Set up internalBinding in the closure


Come to think of it, we may also put internalBinding in the returned object and rename this file inernal_loaders which seem to be more accurate..so it will become const { NativeModule, internalBinding } = require('internal/internal_loaders') (somewhat clashes with internal/loader/* though..)

joyeecheung · 2018-03-03T22:38:36Z

src/node.cc

+
+static bool ExecuteFactory(Environment* env, Local<Function> factory, int argc,
+                           Local<Value> argv[], Local<Value>* out) {
+  bool ret = factory->Call(


This now calls into JS from C++ twice during bootstrapping..maybe passing the loader bootstrapper into the core bootstrapper and call JS from JS will lower the performance impact..are there any existing benchmarks for measuring startup time?

I’d expect this to be fine, tbh. It might be nice to have a single call into JS again at some point, but that doesn’t have to be a strict rule.

addaleax

LGTM, great work!

addaleax · 2018-03-03T22:43:34Z

lib/internal/bootstrap_node.js


 'use strict';

-(function(process) {
-  let internalBinding;
+(function bootstrapNodeJSCore(process, NativeModule) {


I’m wondering whether this function name showing up is more helpful or more confusing when showing up ni embedder code…

addaleax · 2018-03-03T22:45:50Z

lib/internal/native_module.js

+    const bindingObj = Object.create(null);
+
+    const getInternalBinding = process._internalBinding;
+    delete process._internalBinding;


Since you’re at it – how about passing this to bootstrapInternalLoaders as an argument, too?

Good idea, also getLinkedBinding and getBinding can be passed in as well so we don’t need to override the original property, which kind of hard to understand at first glance

addaleax · 2018-03-03T22:47:15Z

src/node.cc

+
+static bool ExecuteFactory(Environment* env, Local<Function> factory, int argc,
+                           Local<Value> argv[], Local<Value>* out) {
+  bool ret = factory->Call(


I’d expect this to be fine, tbh. It might be nice to have a single call into JS again at some point, but that doesn’t have to be a strict rule.

apapirovski

LGTM. I agree with @addaleax that passing in internalBinding into native_module.js would probably be a good idea, instead of declaring it on the process and then deleting it.

joyeecheung · 2018-03-04T12:34:55Z

Updates:

Passing getBinding(), getLinkedBinding() and getInternalBinding() to the loader bootstrapper instead of putting them on process and override latter (suggestion by @addaleax )
Add documentation about process._linkedBinding, this fixes What are Node.js "Linked" modules? #12516
Rename native_module -> bootstrap_loaders, ExecuteFactory/GetFactory -> ExecuteBootstrapper/GetBootstrapper, so it's consistent with bootstrap_node.js and it's easier to figure out what it does just by looking at the names
Expose internalBinding through the object returned by NativeModule.require instead of putting it into the NativeModule.wrapper .

apapirovski

Still LGTM with a tiny nit.

apapirovski · 2018-03-04T12:39:35Z

lib/internal/bootstrap_loaders.js

@@ -109,9 +109,12 @@

  const config = process.binding('config');

+  // Think of this as module.exports in this file even though it is not
+  // written in CommonJS style.
+  const exports_ = { internalBinding, NativeModule };


Not generally a huge fan of trailing _ variables in JS, esp since it looks similar enough to exports. Could this be like loaderExports or something?

joyeecheung · 2018-03-04T13:02:47Z

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

devsnek · 2018-03-04T15:10:38Z

Expose internalBinding through the object returned by NativeModule.require instead of putting it into the NativeModule.wrapper .

i'm feeling -1ish on this with this change, is there any reason it can't be kept in the module wrapper?

also we could move these two files to internal/bootstrap or internal/startup or something and remove the bootstrap_ prefix

joyeecheung · 2018-03-04T15:47:25Z

i'm feeling -1ish on this with this change, is there any reason it can't be kept in the module wrapper?

Doesn't have to move it out, but this way we have less globals in core modules and this can now be used in bootstrap_node.js. (Also, there is not any reason it has to be in the wrapper either...right?)

apapirovski · 2018-03-04T15:51:00Z

I'm +1 on having it be an export rather than a wrapper-introduced global. Given how few modules actually use it, I don't think there's a great argument for having it be a part of the wrapper.

devsnek · 2018-03-04T15:52:04Z

(Also, there is not any reason it has to be in the wrapper either...right?)

this makes it so that it can't be used even if --expose-internals is used, i think that was something @addaleax explicitly did/wanted with her pr, and personally i agree with that change.

apapirovski · 2018-03-04T16:08:01Z

this makes it so that it can't be used even if --expose-internals is used, i think that was something @addaleax explicitly did/wanted with her pr, and personally i agree with that change.

Given that NativeModule was already exposed, I'm almost certain it was possible — if a bit awkward — to get internalBinding before.

(Edit: Or was it not because of the different module id?)

joyeecheung · 2018-03-04T16:10:36Z

this makes it so that it can't be used even if --expose-internals is used, i think that was something @addaleax explicitly did/wanted with her pr, and i also like how isolated it is currently

That can be fixed by changing NativeModule.isInternal though (which reminds me this PR leaks NativeModule by prefixing the file internal/). The same could be said about NativeModule - I think we should isolate them the same way, no matter through a wrapper or by tweaking NativeModule.isInternal.

addaleax · 2018-03-04T16:18:35Z

I'm +1 on having it be an export rather than a wrapper-introduced global. Given how few modules actually use it, I don't think there's a great argument for having it be a part of the wrapper.

I don’t really care how it is exposed, but I think the argument that few modules use it shouldn’t have a strong weight, because we really should be and are moving towards using it in place of process.binding() (slowly).

Given that NativeModule was already exposed, I'm almost certain it was possible — if a bit awkward — to get internalBinding before.

At least I couldn’t figure out how (and thought that that was a good thing). ¯\_(ツ)_/¯

joyeecheung · 2018-03-04T16:23:36Z

also we could move these two files to internal/bootstrap or internal/startup or something and remove the bootstrap_ prefix

I like the idea of having a file named node.js again ;) but probably should do that later since that would involve a lot of stack traces in test/message and comments to fix.

apapirovski · 2018-03-04T16:23:42Z

I don’t really care how it is exposed, but I think the argument that few modules use it shouldn’t have a strong weight, because we really should be and are moving towards using it in place of process.binding() (slowly).

I just don't love globals, so the fewer, the better in my mind and the exceptions should be things that we can't really run a module without (or are just used that often). (Needless to say, I would obviously prefer if process.binding wasn't exposed either...)

But anyway, don't have a strong opinion so I'm fine with whatever direction this ends up going in.

devsnek · 2018-03-04T16:24:41Z

actually its worse than i thought, internalBinding can be gotten without any flags on current release/master, so maybe this pr actually makes it harder to get? 😓

$ cat leeks.js
process.binding('natives').owo = 'module.exports = internalBinding';
console.log(require('owo'));

$ node leeks.js
[Function: internalBinding]

addaleax · 2018-03-04T16:27:15Z

@devsnek Yes, that sounds pretty bad :/ I guess we could and should freeze process.binding('natives'), for a start?

devsnek · 2018-03-04T16:29:46Z

ideally it shouldn't be exposed at all, perhaps we can deprecate and remove it? i'm worried that packages might be using this, mostly for reading, not writing (and it also seems that v8_prof_processor modifies several scripts by adding newlines to the ends of them, i'm not quite sure what to make of that)

devsnek · 2018-03-04T16:35:58Z

it should be noted that even with that last commit by joyee, native module can still be gotten with the same tomfoolery i posted above

addaleax · 2018-03-04T16:37:49Z

ideally it shouldn't be exposed at all, perhaps we can deprecate and remove it?

I’m on board with doing that, but it might be a while before we can do it – right now, the latest verions of gulp are famously still using graceful-fs@3, which reads those bindings…

(and it also seems that v8_prof_processor modifies several scripts by adding newlines to the ends of them)

I think that’s still just reading, it basically concatenates them with newlines?

- Moves the creation of `process.binding()`, `process._linkedBinding()` `internalBinding()` and `NativeModule` into a separate file `lib/internal/bootstrap_loaders.js`, and documents them there. This file will be compiled and run before `bootstrap_node.js`, which means we now bootstrap the internal module & binding system before actually bootstrapping Node.js. - Rename the special ID that can be used to require `NativeModule` as `internal/bootstrap_loaders` since it is setup there. Also put `internalBinding` in the object exported by `NativeModule.require` instead of putting it inside the `NativeModule.wrapper` - Use the original `getBinding()` to get the source code of native modules instead of getting it from `process.binding('native')` so that users cannot fake native modules by modifying the binding object. - Names the bootstrapping functions so their names show up in the stack trace. PR-URL: nodejs#19112 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/bootstrap/` and put bootstrappers there: Before: ``` lib/internal ├── ... ├── bootstrap_loaders.js └── bootstrap_node.js ``` After: ``` lib/internal ├── ... └── bootstrap ├── loaders.js └── node.js ``` PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/modules` and restructure the module loaders to make the purpose of those files clearer. Also make it clear in the code that the object exported by `lib/internal/modules/cjs/loader.js` is `CJSModule` instead of the ambiguous `Module`. Before: ``` lib ├── ... ├── internal │ ├── loaders │ │ ├── CreateDynamicModule.js │ │ ├── DefaultResolve.js │ │ ├── Loader.js │ │ ├── ModuleJob.js │ │ ├── ModuleMap.js │ │ ├── ModuleWrap.js │ │ └── Translators.js │ └── module.js └── module.js ``` After: ``` lib ├── ... ├── internal │ ├── ... │ └── modules │ ├── cjs │ │ ├── helpers.js │ │ └── loader.js │ └── esm │ ├── CreateDynamicModule.js │ ├── DefaultResolve.js │ ├── Loader.js │ ├── ModuleJob.js │ ├── ModuleMap.js │ └── Translators.js └── module.js # deleted in this commit to work with git file mode ``` PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The previous commit deleted lib/module.js so that git recognize the file move `lib/module.js` -> `lib/internal/modules/cjs/loader.js`. This commit add the redirection back. PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

- Moves the creation of `process.binding()`, `process._linkedBinding()` `internalBinding()` and `NativeModule` into a separate file `lib/internal/bootstrap_loaders.js`, and documents them there. This file will be compiled and run before `bootstrap_node.js`, which means we now bootstrap the internal module & binding system before actually bootstrapping Node.js. - Rename the special ID that can be used to require `NativeModule` as `internal/bootstrap_loaders` since it is setup there. Also put `internalBinding` in the object exported by `NativeModule.require` instead of putting it inside the `NativeModule.wrapper` - Use the original `getBinding()` to get the source code of native modules instead of getting it from `process.binding('native')` so that users cannot fake native modules by modifying the binding object. - Names the bootstrapping functions so their names show up in the stack trace. Backport-PR-URL: #19374 PR-URL: #19112 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Create `lib/internal/bootstrap/` and put bootstrappers there: Before: ``` lib/internal ├── ... ├── bootstrap_loaders.js └── bootstrap_node.js ``` After: ``` lib/internal ├── ... └── bootstrap ├── loaders.js └── node.js ``` Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/modules` and restructure the module loaders to make the purpose of those files clearer. Also make it clear in the code that the object exported by `lib/internal/modules/cjs/loader.js` is `CJSModule` instead of the ambiguous `Module`. Before: ``` lib ├── ... ├── internal │ ├── loaders │ │ ├── CreateDynamicModule.js │ │ ├── DefaultResolve.js │ │ ├── Loader.js │ │ ├── ModuleJob.js │ │ ├── ModuleMap.js │ │ ├── ModuleWrap.js │ │ └── Translators.js │ └── module.js └── module.js ``` After: ``` lib ├── ... ├── internal │ ├── ... │ └── modules │ ├── cjs │ │ ├── helpers.js │ │ └── loader.js │ └── esm │ ├── CreateDynamicModule.js │ ├── DefaultResolve.js │ ├── Loader.js │ ├── ModuleJob.js │ ├── ModuleMap.js │ └── Translators.js └── module.js # deleted in this commit to work with git file mode ``` Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

The previous commit deleted lib/module.js so that git recognize the file move `lib/module.js` -> `lib/internal/modules/cjs/loader.js`. This commit add the redirection back. Backport-PR-URL: #19374 PR-URL: #19177 Refs: #19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

- Moves the creation of `process.binding()`, `process._linkedBinding()` `internalBinding()` and `NativeModule` into a separate file `lib/internal/bootstrap_loaders.js`, and documents them there. This file will be compiled and run before `bootstrap_node.js`, which means we now bootstrap the internal module & binding system before actually bootstrapping Node.js. - Rename the special ID that can be used to require `NativeModule` as `internal/bootstrap_loaders` since it is setup there. Also put `internalBinding` in the object exported by `NativeModule.require` instead of putting it inside the `NativeModule.wrapper` - Use the original `getBinding()` to get the source code of native modules instead of getting it from `process.binding('native')` so that users cannot fake native modules by modifying the binding object. - Names the bootstrapping functions so their names show up in the stack trace. PR-URL: nodejs#19112 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

MylesBorins · 2018-05-23T18:01:23Z

Would very much like to see this backported to v8.x along with #18365

- Moves the creation of `process.binding()`, `process._linkedBinding()` `internalBinding()` and `NativeModule` into a separate file `lib/internal/bootstrap_loaders.js`, and documents them there. This file will be compiled and run before `bootstrap_node.js`, which means we now bootstrap the internal module & binding system before actually bootstrapping Node.js. - Rename the special ID that can be used to require `NativeModule` as `internal/bootstrap_loaders` since it is setup there. Also put `internalBinding` in the object exported by `NativeModule.require` instead of putting it inside the `NativeModule.wrapper` - Use the original `getBinding()` to get the source code of native modules instead of getting it from `process.binding('native')` so that users cannot fake native modules by modifying the binding object. - Names the bootstrapping functions so their names show up in the stack trace. PR-URL: nodejs#19112 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Gus Caplan <[email protected]>

Otherwise the debug log output might be mixed up with the expected errors and the assertion matching the error message would fail. PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Create `lib/internal/bootstrap/` and put bootstrappers there: Before: ``` lib/internal ├── ... ├── bootstrap_loaders.js └── bootstrap_node.js ``` After: ``` lib/internal ├── ... └── bootstrap ├── loaders.js └── node.js ``` PR-URL: nodejs#19177 Refs: nodejs#19112 Reviewed-By: Gus Caplan <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Mar 3, 2018

joyeecheung added module Issues and PRs related to the module subsystem. process Issues and PRs related to the process subsystem. labels Mar 3, 2018

joyeecheung commented Mar 3, 2018

View reviewed changes

addaleax approved these changes Mar 3, 2018

View reviewed changes

apapirovski approved these changes Mar 4, 2018

View reviewed changes

joyeecheung mentioned this pull request Mar 4, 2018

What are Node.js "Linked" modules? #12516

Closed

joyeecheung force-pushed the split-native-module branch from 0240c6f to 9239ca3 Compare March 4, 2018 12:30

apapirovski approved these changes Mar 4, 2018

View reviewed changes

joyeecheung changed the title ~~src: move internal loaders out of bootstrap_node.js~~ src: bootstrap loaders before bootstrapping Node.js Mar 4, 2018

joyeecheung changed the title ~~src: bootstrap loaders before bootstrapping Node.js~~ src: bootstrap internals loaders before bootstrapping Node.js Mar 4, 2018

targos added backported-to-v9.x and removed backport-requested-v9.x labels Apr 4, 2018

targos mentioned this pull request Apr 4, 2018

v9.11.0 proposal #19799

Merged

MylesBorins added the backport-requested-v8.x label May 23, 2018

joyeecheung self-assigned this May 26, 2018

addaleax mentioned this pull request Jun 7, 2018

tools: add option to use custom template with js2c.py #21187

Closed

2 tasks

joyeecheung mentioned this pull request Mar 29, 2023

module: do less CJS module loader initialization at run time #47194

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: bootstrap internals loaders before bootstrapping Node.js #19112

src: bootstrap internals loaders before bootstrapping Node.js #19112

joyeecheung commented Mar 3, 2018 •

edited

Loading

joyeecheung commented Mar 3, 2018

joyeecheung Mar 3, 2018

joyeecheung Mar 3, 2018

addaleax Mar 3, 2018

addaleax left a comment

addaleax Mar 3, 2018

addaleax Mar 3, 2018 •

edited

Loading

joyeecheung Mar 4, 2018

addaleax Mar 3, 2018

apapirovski left a comment

joyeecheung commented Mar 4, 2018 •

edited

Loading

apapirovski left a comment

apapirovski Mar 4, 2018

joyeecheung commented Mar 4, 2018

devsnek commented Mar 4, 2018 •

edited

Loading

joyeecheung commented Mar 4, 2018

apapirovski commented Mar 4, 2018

devsnek commented Mar 4, 2018 •

edited

Loading

apapirovski commented Mar 4, 2018 •

edited

Loading

joyeecheung commented Mar 4, 2018

addaleax commented Mar 4, 2018

joyeecheung commented Mar 4, 2018

apapirovski commented Mar 4, 2018

devsnek commented Mar 4, 2018 •

edited

Loading

addaleax commented Mar 4, 2018

devsnek commented Mar 4, 2018 •

edited

Loading

devsnek commented Mar 4, 2018

addaleax commented Mar 4, 2018

MylesBorins commented May 23, 2018

src: bootstrap internals loaders before bootstrapping Node.js #19112

src: bootstrap internals loaders before bootstrapping Node.js #19112

Conversation

joyeecheung commented Mar 3, 2018 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung commented Mar 3, 2018

joyeecheung Mar 3, 2018

Choose a reason for hiding this comment

joyeecheung Mar 3, 2018

Choose a reason for hiding this comment

addaleax Mar 3, 2018

Choose a reason for hiding this comment

addaleax left a comment

Choose a reason for hiding this comment

addaleax Mar 3, 2018

Choose a reason for hiding this comment

addaleax Mar 3, 2018 • edited Loading

Choose a reason for hiding this comment

joyeecheung Mar 4, 2018

Choose a reason for hiding this comment

addaleax Mar 3, 2018

Choose a reason for hiding this comment

apapirovski left a comment

Choose a reason for hiding this comment

joyeecheung commented Mar 4, 2018 • edited Loading

apapirovski left a comment

Choose a reason for hiding this comment

apapirovski Mar 4, 2018

Choose a reason for hiding this comment

joyeecheung commented Mar 4, 2018

devsnek commented Mar 4, 2018 • edited Loading

joyeecheung commented Mar 4, 2018

apapirovski commented Mar 4, 2018

devsnek commented Mar 4, 2018 • edited Loading

apapirovski commented Mar 4, 2018 • edited Loading

joyeecheung commented Mar 4, 2018

addaleax commented Mar 4, 2018

joyeecheung commented Mar 4, 2018

apapirovski commented Mar 4, 2018

devsnek commented Mar 4, 2018 • edited Loading

addaleax commented Mar 4, 2018

devsnek commented Mar 4, 2018 • edited Loading

devsnek commented Mar 4, 2018

addaleax commented Mar 4, 2018

MylesBorins commented May 23, 2018

joyeecheung commented Mar 3, 2018 •

edited

Loading

addaleax Mar 3, 2018 •

edited

Loading

joyeecheung commented Mar 4, 2018 •

edited

Loading

devsnek commented Mar 4, 2018 •

edited

Loading

devsnek commented Mar 4, 2018 •

edited

Loading

apapirovski commented Mar 4, 2018 •

edited

Loading

devsnek commented Mar 4, 2018 •

edited

Loading

devsnek commented Mar 4, 2018 •

edited

Loading