vm: use default host-defined options when importModuleDynamically is not set

[joyeecheung]

This makes it possile to hit the in-isolate compilation cache when host-defined options are not necessary.

Local benchmark numbers: huge variance because when the cache is missed (in the main branch) it is just toooooo slow, I think we can keep n fairly small for now until we no longer need to compare the cache-miss cases, but the speed up is very visible if you just run the benchmark locally - with cache hit it finishes in a blink of an eye and without cache it gets stuck for a few seconds.

                                                                                   confidence improvement accuracy (*)     (**)    (***)
vm/compile-script-in-isolate-cache.js n=100 type='with-dynamic-import-callback'                   -0.33 %       ±0.81%   ±1.08%   ±1.41%
vm/compile-script-in-isolate-cache.js n=100 type='without-dynamic-import-callback'        ***   9132.25 %     ±319.74% ±433.30% ±580.24%

Refs: #35375

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[rluvaton]

oh Damn!

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[benjamingr]

@SimenB any chance you can check and verify this fixes the v16->v18 performance regression on Jest's side?

[SimenB]

@benjamingr I did here: #35375 (comment)

It works 👍 (the thing I mention below my results)

@joyeecheung is it possible to only attach this callback if import() is actually used in the source? I see we currently get the perf hit whether or not the callback will be used or not.
If that's possible to check, I don't have to change anything in Jest to take advantage of this PR.

[joyeecheung]

is it possible to only attach this callback if import() is actually used in the source?

I think it would be difficult to check that just from Node.js. For example the code can also eval() or new Function() that does import() from a string. Maybe the userland can do that check itself having a bit more knowledge about exactly how the API is going to be used. Ultimately this probably still comes back to V8.

[SimenB]

The docs for the option says "This option is part of the experimental modules API", so I'd expect if the experimental API is not active that there's no impact, even if the callback is present in the options.

https://nodejs.org/api/vm.html#new-vmscriptcode-options

But again - if this is difficult I can add that check myself in Jest (i.e. only attach the callback if SourceTextModule is available)

[rluvaton]

I did here: #35375 (comment)

It works 👍 (the thing I mention below my results)

@SimenB it look like you tried for the minimal reproduction, does it fix it for jest as a whole? meaning actual jest project

[SimenB]

Yeah, it's how I discovered I had to remove the importModuleDynamically callback for the changes in this PR to take effect.

[SimenB]

@joyeecheung I see this:

node/lib/vm.js

Lines 339 to 341 in 1b96975

	// The vm module is patched to include vm.Module, vm.SourceTextModule
	// and vm.SyntheticModule in the pre-execution phase when
	// --experimental-vm-modules is on.

Would it be possible to add the same "only if flag is active" logic to passing importModuleDynamically on to the C++ layer? Trivial change on Jest's side, but would be nice if the improvement applied to older versions of Jest as well via "just" a Node.js upgrade 🙂

[joyeecheung]

Would it be possible to add the same "only if flag is active" logic to passing importModuleDynamically on to the C++ layer?

Ignoring the callback all together when the flag isn't active sounds like a plausible idea, though I am not sure if the API can be / is used in a way that does not need the experimental vm.Module classes so that doing this would break them. @nodejs/loaders @nodejs/vm any idea?

I think we can get this landed first before investigating that idea though. If that turns out to be breaking, we can probably still do it in semver-major. If not, semver-patch.

[SimenB]

AFAIK the callback is only usable if the flag is active - there's no non-flagged thing that can be returned from the callback, so it'll end up throwing anyways.

Doing it in separate PRs is of course very much feasible just to get this one in first 🙂 But it won't help people on older versions of Jest (unless you go way back), just future versions. Still super valuable of course!

[aaldao]

Hello @joyeecheung!
There is any way to track the progress on backporting to v18?
Thanks!

[joyeecheung]

I will do it together with #50137 when that one reaches enough time (2 weeks) for the backport

[bmeck]

This wont affect https://github.com/nodejs/node/issues/49726#issuecomment-1761234932 will it?

[targos]

@joyeecheung The 2 weeks is not a minimum wait time for the backport; it's for the release. I'm preparing the next v18.x release and I confirm that it will happen at least two weeks after v21.1.0.

[joyeecheung]

This wont affect #49726 (comment) will it?

No, that's an orthogonal issue.