esm: refactor hooks – add loader instances #49315

izaakschroeder · 2023-08-24T20:50:37Z

Previously we had one array for each kind of chain in the variable #chains; this is probably the most efficient, but it decouples the connection between the original register call and the hook functions themselves.

By adding #loaderInstances we can keep track of each time register is called. This allows, for example, trivially removing the loader later on, or associating shared context with a particular register call. The tradeoff here if we just did this would be that during hook traversal we may iterate over a slightly larger number of objects. Instead we keep #chains but it is now a derived property; every time #loaderInstances changes we simply rebuild #chains.

This sets the stage for doing other things, like adding deregister: #49159 or adding the ability to attach local state to individual loader instances.

N.B. I should probably add back the documentation about the entries in #chains somehow.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

nodejs-github-bot · 2023-08-24T20:50:43Z

Review requested:

@nodejs/loaders
@nodejs/modules

lib/internal/modules/esm/hooks.js

GeoffreyBooth · 2023-08-24T22:02:01Z

lib/internal/modules/esm/default_loader.js

+const { defaultResolve } = require('internal/modules/esm/resolve');
+
+exports.resolve = defaultResolve;
+exports.load = defaultLoad;


If we create a new file we might need to add it to the startup snapshot (see #45849). Alternatively is there a way to stick this in an existing file?

The reason I put this in its own file is so that the url property actually led to a valid file that represented the base "loader".

It can live in literally any file that can be treated as a loader (exports load, resolve, etc.) This code is the relevant block:

const defaultLoader = 'internal/modules/esm/default_loader'; this.addCustomLoader(`node:${defaultLoader}`, require(defaultLoader));

So that if we ever expose a list loaders or anything like that the url property is what you see above.

GeoffreyBooth · 2023-08-24T22:03:19Z

lib/internal/modules/esm/hooks.js

+  }
+
+  #rebuildChains() {
+    this.#rebuildChain('globalPreload');


Let’s wait to land this after #49144.

Works for me 🎉

Previously we had one array for each kind of chain in the variable `#chains`; this is probably the most efficient, but it decouples the connection between the original `register` call and the hook functions themselves. By adding `#loaderInstances` we can keep track of each time `register` is called. This allows, for example, trivially removing the loader later on, or associating shared context with a particular `register` call. The tradeoff here if we just did this would be that during hook traversal we may iterate over a slightly larger number of objects. Instead we keep `#hooks` but it is now a derived property; every time `#loaderInstances` changes we simply rebuild `#hooks`.

JakobJingleheimer

Hmm, the code here is less readable and more convoluted in execution than it was before. I think my proposals will help, but I'd need to see them all together in the diff.

Note that you basically cannot use for…of or native object methods at all due to the risk from prototype pollution. See https://github.com/nodejs/node/blob/main/doc/contributing/primordials.md

JakobJingleheimer · 2023-08-25T08:18:52Z

lib/internal/modules/esm/hooks.js

+  #rebuildChain(name) {
+    const chain = this.#chains[name] = [];
+    let i = 0;
+    for (const instance of this.#loaderInstances) {


Can't use for…of :( gotta use a plain ol' for

💯 😢 😆

JakobJingleheimer · 2023-08-25T08:19:24Z

lib/internal/modules/esm/hooks.js

@@ -87,45 +86,40 @@ let importMetaInitializer;
 // [2] `validate...()`s throw the wrong error

 class Hooks {
-  #chains = {


Why remove these? If you need them to start out empty, you can still leave the fields (resolve etc) and then also the code docs

So that the built-in hooks are not treated as any kind of "special" object; they now work just like any other loader – you can see them being added in the constructor. This ensures that if we attach any data to a loader instance that the default node loader gets it too – right now it's just url.

Ah, yes. I figured that :) I think you can still leave everything else here and just initialise the lists as empty 🙂

Ah yes true 😄 I can do that. Part of me considered that hard-coding the hook names in a bunch of places might be less clean… but it's more clear and that works for me 🎉

JakobJingleheimer · 2023-08-25T08:24:39Z

lib/internal/modules/esm/hooks.js

  const { hookName } = meta;
-  const {
-    fn: hook,
-    url: hookFilePath,
-    next,
-  } = current;


Did anything change here? It looks like it's now just less readable

Yes. The previous structure of chain:

type Fn = (arg0: any, context: any, next: Fn) => any; interface ChainItem { fn: Fn url: string; } type Chain = ChainItem[];

The new structure of chain:

type Fn = (arg0: any, context: any, next: Fn) => any; interface LoaderInstance { load?: Fn; resolve?: Fn; initialize?: Fn; url: string; } interface ChainItem { fn: Fn loader: LoaderInstance; } type Chain = ChainItem[];

JakobJingleheimer · 2023-08-25T08:27:44Z

lib/internal/modules/esm/hooks.js

+      if (typeof instance[name] !== 'function') {
+        continue;
+      }
+      chain.push({


Gotta use primordials for these sorts of things ArrayPrototypePush.

Derp thanks 😄

Suggested change

chain.push({

ArrayPrototypePush(chain, {

__proto__: null,

JakobJingleheimer · 2023-08-25T08:52:51Z

lib/internal/modules/esm/hooks.js

+    }
+  }
+
+  #rebuildChains() {


This is a performance loss. I think it would be better for there to be only rebuildChains and for it to accept a list of chain names to rebuild, like

#rebuildChains(...names) { if (!names.length) { names = new Array('initialize','load','resolve'); // [1] } const shouldRebuild = { __proto__: null }; for (let n = names.length - 1; n > -1; n--) { const name = names[n]; shouldRebuild[name]: true; this.#chains[name].length = 0; // [2] Might need a primordial? } for ( let i = 0, l = this.#loaderInstances.length - 1; i < l; i++ ) { const { initialize, load, resolve, } = this.#loaderInstances[i]; if (shouldRebuild.initialize) { /* … */ } if (shouldRebuild.load) { /* … */ } if (shouldRebuild.resolve) { /* … */ } } }

this.#rebuildChains('resolve'); // Rebuild only `resolve` chain this.#rebuildChains('load','resolve'); // Rebuild `load` and `resolve` this.#rebuildChains(); // Rebuild all chains

[1] I believe this (new Array(…) instead of […]) will invoke an optimisation in V8 to choose the most appropriate specialised C++ array from the start and also to pre-allocation only exactly the space for these 3 items. If it doesn't, no need for the otherwise uncommon use of new Array() instead of [].

[2] We don't need to re-create the array, only empty it. For that, setting its length to 0 is far better.

I think we can make this happen 😄 Probably a bit of a µ-optimization considering how often #rebuildChains is called and the size of N… but it's not a problem to make it a little bit faster. If the actual chain execution was affected I would be more concerned, considering that is called on every import/require.

Mm, it likely won't break the performance bank. But it's easy to avoid the de-op, so why not 🙂

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Aug 24, 2023

izaakschroeder commented Aug 24, 2023

View reviewed changes

lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved

izaakschroeder commented Aug 24, 2023

View reviewed changes

lib/internal/modules/esm/hooks.js Outdated Show resolved Hide resolved

GeoffreyBooth reviewed Aug 24, 2023

View reviewed changes

izaakschroeder force-pushed the esm-loader-instances branch from a989338 to a1aef89 Compare August 25, 2023 00:21

JakobJingleheimer reviewed Aug 25, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: refactor hooks – add loader instances #49315

esm: refactor hooks – add loader instances #49315

izaakschroeder commented Aug 24, 2023 •

edited

Loading

nodejs-github-bot commented Aug 24, 2023

GeoffreyBooth Aug 24, 2023

izaakschroeder Aug 24, 2023

GeoffreyBooth Aug 24, 2023

izaakschroeder Aug 24, 2023

JakobJingleheimer left a comment

JakobJingleheimer Aug 25, 2023

JakobJingleheimer Aug 25, 2023 •

edited

Loading

JakobJingleheimer Aug 25, 2023

izaakschroeder Aug 25, 2023

JakobJingleheimer Aug 25, 2023

izaakschroeder Aug 25, 2023

JakobJingleheimer Aug 25, 2023

izaakschroeder Aug 25, 2023

JakobJingleheimer Aug 25, 2023

izaakschroeder Aug 25, 2023

ljharb Aug 25, 2023

JakobJingleheimer Aug 25, 2023 •

edited

Loading

izaakschroeder Aug 25, 2023

JakobJingleheimer Aug 25, 2023

esm: refactor hooks – add loader instances #49315

Are you sure you want to change the base?

esm: refactor hooks – add loader instances #49315

Conversation

izaakschroeder commented Aug 24, 2023 • edited Loading

nodejs-github-bot commented Aug 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JakobJingleheimer Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

izaakschroeder commented Aug 24, 2023 •

edited

Loading

JakobJingleheimer Aug 25, 2023 •

edited

Loading

JakobJingleheimer Aug 25, 2023 •

edited

Loading