vm: reject in importModuleDynamically without --experimental-vm-modules

[joyeecheung]

vm: unify host-defined option generation in vm.compileFunction

Set a default host-defined option for vm.compileFunction so that
it's consistent with vm.Script.

vm: use internal versions of compileFunction and Script

Instead of using the public versions of the vm APIs internally,
use the internal versions so that we can skip unnecessary
argument validation.

The public versions would need special care to the generation
of host-defined options to hit the isolate compilation cache
when imporModuleDynamically isn't used, while internally it's
almost always used, so this allows us to handle the host-defined
options separately.

vm: reject in importModuleDynamically without --experimental-vm-modules

Users cannot access any API that can be used to return a module or
module namespace in this callback without --experimental-vm-modules
anyway, so this would eventually lead to a rejection. This patch
rejects in this case with our own error message and use a constant
host-defined option for the rejection, so that scripts with the
same source can still be compiled using the compilation cache
if no import() is actually called in the script.

Refs: #35375

with-dynamic-import-callback is Jest's current behavior which blocks users from upgrading from v16.x (see #49950 (comment)).

                                                                                   confidence improvement accuracy (*)     (**)    (***)
vm/compile-script-in-isolate-cache.js n=100 type='with-dynamic-import-callback'           ***   9880.21 %     ±166.83% ±237.30% ±343.44%
vm/compile-script-in-isolate-cache.js n=100 type='without-dynamic-import-callback'                 1.87 %       ±4.21%   ±5.83%   ±8.06%

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[joyeecheung]

This is a follow up of #49950 so that older versions of Jest (and any other packages that are doing a similar custom error in the importModuleDynamically callback) don't have to be punished by compilation cache misses. (Also, more background in jestjs/jest#12205 (comment)).

cc @SimenB

[SimenB]

This is awesome, thank you so much for your time and efforts here!

[joyeecheung]

Local numbers of the compile-script-in-isolate-cache.js benchmark - similar to #49950 hitting the cache results in a ~100x speedup (note that in the benchmark the import callback isn't doing anything useful because the script does not actually invoke import() and --experimental-vm-modules isn't set, which is the case that this PR wants to optimize here)

                                                                                   confidence improvement accuracy (*)     (**)    (***)
vm/compile-script-in-isolate-cache.js n=100 type='with-dynamic-import-callback'           ***   9880.21 %     ±166.83% ±237.30% ±343.44%
vm/compile-script-in-isolate-cache.js n=100 type='without-dynamic-import-callback'                 1.87 %       ±4.21%   ±5.83%   ±8.06%

[nodejs-github-bot]

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

[joyeecheung]

Fixed linter errors

[nodejs-github-bot]

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

[GeoffreyBooth]

Thank you for doing this! I’ve been working with some of these files lately and this is a great improvement!

[JakobJingleheimer]

No idea what's happening here, but the code certainly looks much much cleaner, and the code comments are great!

[reaktivo]

Hi @joyeecheung Thank you and great work on the fix!

I think these can be backported, although it was just released in v21, according to the LTS protocol we still need to let the fixes sit in the current release for two weeks before backporting them to LTS (also, juust in case there are regressions).

Do you know if there is somewhere I can track the backport for v20?

[joyeecheung]

There would be some conflicts if we backport this before #50183 is backported. But I am not sure if it's worth blocking this backport for that because that one seems to need more investigation. The conflicts are very superficial (changing internal variable names from "assertions" to "attributes").

[joyeecheung]

#50183 just landed so this should land cleanly on v20.x now (I think @targos is doing that)

[targos]

I'm running tests locally. This will be pushed shortly.

[github-actions]

The notable-change PRs with changes that should be highlighted in changelogs. label has been added by @joyeecheung.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[joyeecheung]

Add a post-mortem notable-change label so that we don't miss sending the words out in v18.x again. Copying text from v21.1.0:

### vm: fix V8 compilation cache support for vm.Script

Previously repeated compilation of the same source code using `vm.Script`
stopped hitting the V8 compilation cache after v16.x when support for
`importModuleDynamically` was added to `vm.Script`, resulting in a performance
regression that blocked users (in particular Jest users) from upgrading from
v16.x.

The recent fixes allow the compilation cache to be hit again
for `vm.Script` when `--experimental-vm-modules` is not used even in the
presence of the `importModuleDynamically` option, so that users affected by the
performance regression can now upgrade. Ongoing work is also being done to
enable compilation cache support for `vm.CompileFunction`.

Contributed by Joyee Cheung in [#49950](https://github.com/nodejs/node/pull/49950)
and [#50137](https://github.com/nodejs/node/pull/50137).

[billoneil]

@joyeecheung I see the backport to 20.x was merged. Will this also be backported to 18.x?

[joyeecheung]

IIUC the plan is to backport this to 18.x after #50669 (which just landed) to reduce conflict. Maybe @targos can confirm.

[targos]

I have no plan to backport it as it's out of my competence, but I would like to see it backported :)

[joyeecheung]

Oh I thought it should land cleanly after that. If not I can backport it myself.

[joyeecheung]

hmm I just realized that to backport the whole thing, we will need to backport Symbols as WeakMap keys to v18.x (it might be possible to just backport the cache fix, but then that's probably not enough to unblock upgrades because without Symbols as WeakMap keys we won't be able to backport the memory fixes, so people are still going to run into seg faults and leaks). I'm not sure about our policy regarding backporting language features from V8 to maintenance LTS though. Setting policies aside, it's not actually that big of a change so it shouldn't be too hard to backport it to v18.x.

[joyeecheung]

So I got some confirmations that it's okay to backport Symbols as WeakMap keys to v18.x, I have the Node.js part backport done in https://github.com/joyeecheung/node/tree/backport-50137-to-18, currenly working on the V8 part