Move ESM loaders off-thread

[JakobJingleheimer]

Resolves https://github.com/nodejs/node/issues/43658

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/modules

[cspotcode]

Does each worker thread get its own loader thread? E.g. if an app launches 2x worker threads, there will be 6x threads total: one main, 2x worker, 3x loader?

[JakobJingleheimer]

The current design puts all loaders in the same worker thread (eg there are a total of 2 threads: the main and the worker). Internal things are handled via the main thread; userland things are handled by the worker thread.

[cspotcode]

I think that's slightly different than what I'm asking about. If a node app uses the worker threads API to launch worker threads, does each get its own loader thread? (where each loader thread may have one or more loaders running within it)

Not does each loader get a thread, but does each user thread get a loader thread.

[GeoffreyBooth]

If a node app uses the worker threads API to launch worker threads, does each get its own loader thread?

I think the current state (before this PR) is that loaders always execute in the main thread, even if they’re “for” user code that is in a worker thread. So I would think that after this PR, loaders would run inside their worker thread, regardless of whether they’re customizing main or worker thread user code.

So in other words, for an app using custom loaders where user code that spawns three workers, there are five threads total: main, loaders, and the three workers spawned by the user code.

This is just my guess, others can please correct me if I’m mistaken.

[targos]

I don't know how it's implemented but I would expect that each Node.js thread (main and workers) has its own separate loaders thread, at least for two reasons:

• Worker threads are supposed to be as isolated as possible from the main thread and from each other
• You can spawn a worker thread with a different set of --loader flags.

[bmeck]

First pass this seems fine but likely should investigate a thread per loader URL/some key. Having it per thread they instrument would be more costly if you spawn/tear down threads.

On Tue, Sep 27, 2022, 1:24 PM Michaël Zasso @.***> wrote:

I don't know how it's implemented but I would expect that each Node.js thread (main and workers) has its own separate loaders thread, at least for two reasons:

• Worker threads are supposed to be as isolated as possible from the main thread and from each other
• You can spawn a worker thread with a different set of --loader flags.

— Reply to this email directly, view it on GitHub https://github.com/nodejs/node/pull/44710#issuecomment-1259889616, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZJI7YII3ZMWWAGIMOC6LWAM3WJANCNFSM6AAAAAAQPNEM7E . You are receiving this because you are on a team that was mentioned.Message ID: @.***>

[targos]

What remains to be done?

[JakobJingleheimer]

Something in this implementation is causing node to hang on startup. We suspect it's some kind of circular dependency, and the subsequent dependency smacks into the Atomics lock (preventing the rest of the flow to complete, which would release the initial lock). I think this circular dep is between ESMLoader and Worker (it's rather difficult to troubleshoot as output is swallowed).

There is a working PoC, so we know this works in principle (and that the Atomics otherwise behave appropriately, so the problem lies with integrating it into node).

This was on hold whilst I was on holiday. I'm working on it today and tomorrow (but will be away on a business trip next week).

[JakobJingleheimer]

I found the circular dep:

process/esm_loader.js:12 ← modules/esm/public_loader_proxy.js:3 ← worker_thread.js:29/36 ← process/pre_execution.js:562 ← process/esm_loader.js

[JakobJingleheimer]

Gah, it wasn't a circular dependency 🤦‍♂️ it was setting url to null here:

https://github.com/nodejs/node/blob/9836c67198b2d01350f283e10b872849e81abefa/lib/internal/worker.js#L141-L149

which caused the worker to spawn with nothing in it.

(I shouldn't have copied it over from the previous attempt at off-threading, which was later hard-coding the url).

I'm not sure if the internal worker should go through the classic path, or merely get require()ed (I tried both, and it doesn't seem to make a difference).

There is (now was) a circular dependency issue after the empty worker issue is addressed in

https://github.com/nodejs/node/blob/9836c67198b2d01350f283e10b872849e81abefa/lib/internal/main/worker_thread.js#L142

that's now fixed too.

NODE_DEBUG=esm,worker ./out/Release/node -p 1 still doesn't print 1, BUT it does now print debugging output, and we can see it getting fairly far along now, so progress:

ESM 13018: hooking up public ESMLoader
ESM 13018: creating worker for publicESMLoader
[…]
WORKER 15812: instantiating Worker. url: file:///…/nodejs/node/lib/internal/modules/esm/worker.js doEval: classic
WORKER 15812: [0] created Worker with ID 1

esm/worker.js's debug (worker for public ESM running) is not printing, so that's likely why things have stalled (as soon as the worker is running, it releases the lock).

[JakobJingleheimer]

I'm currently investigating the V8 / C++ error thrown when the Worker is instantiated:

node:internal/worker:202
    this[kHandle] = new WorkerImpl(url,
                    ^
This Environment was initialized without a V8::Inspector
Thrown at:
    at Worker (node:internal/worker:202:21)
    at InternalWorker (node:internal/worker:446:5)
    at node:internal/modules/esm/public_loader_proxy:39:16
    at compileForInternalLoader (node:internal/bootstrap/loaders:331:7)
    at nativeModuleRequire (node:internal/bootstrap/loaders:362:14)
    at node:internal/process/esm_loader:49:45
    at compileForInternalLoader (node:internal/bootstrap/loaders:331:7)
    at nativeModuleRequire (node:internal/bootstrap/loaders:362:14)
    at node:internal/modules/cjs/loader:141:18
    at compileForInternalLoader (node:internal/bootstrap/loaders:331:7)

This error is also occurring in https://github.com/nodejs/node/pull/44732#issuecomment-1253914637.

[JakobJingleheimer]

Adding a “request changes” here to see https://github.com/nodejs/node/issues/43658#issuecomment-1288170130 resolved.

It might be a little premature to drop a block: The loaders team (what're we called now?) also want the numbers you're concerned about, and I'm not gonna be the guy to tank node's performance 😉

Feel free to leave the block if you'd rather, but I fear leaving it might give passersby the wrong takeaway on the status of the PR.

[aduh95]

Now node --input-type=module -e 'console.log(1)' is creating the following error:

Error: [object Module] could not be cloned.
    at serialize (node:v8:371:7)
    at node:internal/modules/esm/worker:81:29
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)

We would need a way to pass those to one thread to another.