vm: reject in importModuleDynamically without --experimental-vm-modules by joyeecheung · Pull Request #50137 · nodejs/node

joyeecheung · 2023-10-11T11:56:20Z

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 · 2023-10-11T11:56:26Z

Review requested:

@nodejs/loaders

joyeecheung · 2023-10-11T12:57:19Z

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 · 2023-10-11T12:59:46Z

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

joyeecheung · 2023-10-11T13:11:12Z

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 · 2023-10-11T16:32:41Z

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

joyeecheung · 2023-10-12T11:50:34Z

Fixed linter errors

nodejs-github-bot · 2023-10-12T11:56:29Z

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!

targos · 2023-11-11T19:45:43Z

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

Set a default host-defined option for vm.compileFunction so that it's consistent with vm.Script. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

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. PR-URL: #50137 Refs: #35375 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

github-actions · 2023-11-12T10:02:04Z

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 · 2023-11-12T10:03:07Z

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 · 2023-11-22T18:44:04Z

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

joyeecheung · 2023-11-23T13:44:57Z

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

targos · 2023-11-23T13:58:01Z

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

joyeecheung · 2023-11-24T20:28:00Z

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

joyeecheung · 2023-11-25T22:59:35Z

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 · 2023-12-01T17:01:20Z

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

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 11, 2023

joyeecheung force-pushed the vm-flag-callback-2 branch from 3ab08c0 to def4010 Compare October 11, 2023 12:53

SimenB mentioned this pull request Oct 11, 2023

[Bug]: Memory consumption issues on Node JS 16.11.0+ jestjs/jest#11956

Closed

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 11, 2023

joyeecheung force-pushed the vm-flag-callback-2 branch from def4010 to 96bd70f Compare October 12, 2023 11:50

joyeecheung added module Issues and PRs related to the module subsystem. vm Issues and PRs related to the vm subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 12, 2023

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2023

joyeecheung mentioned this pull request Oct 12, 2023

typings: lib/internal/vm.js #50112

Merged

GeoffreyBooth approved these changes Oct 12, 2023

View reviewed changes

JakobJingleheimer reviewed Oct 12, 2023

View reviewed changes

Comment thread lib/internal/modules/cjs/loader.js

anonrig approved these changes Oct 12, 2023

View reviewed changes

GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 12, 2023

legendecas reviewed Oct 13, 2023

View reviewed changes

Comment thread src/env_properties.h Outdated

legendecas reviewed Oct 13, 2023

View reviewed changes

Comment thread doc/api/errors.md Outdated

legendecas removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2023

GeoffreyBooth reviewed Oct 13, 2023

View reviewed changes

Comment thread src/node_contextify.cc Outdated

aduh95 reviewed Oct 13, 2023

View reviewed changes

GeoffreyBooth mentioned this pull request Oct 14, 2023

esm: improve check for ESM syntax #50127

Merged

targos mentioned this pull request Nov 12, 2023

v20.10.0 release proposal #50682

Merged

joyeecheung added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 12, 2023

targos added backported-to-v20.x and removed lts-watch-v20.x labels Nov 12, 2023

targos added the backport-requested-v18.x label Nov 23, 2023

joyeecheung mentioned this pull request Dec 1, 2023

[18.x] vm: backport vm-related fixes #51004

Closed

juanarbol mentioned this pull request Dec 6, 2023

2023-12-06, N|Solid v5.0.1 Iron Release nodesource/nsolid#41

Merged

richardlau mentioned this pull request Mar 20, 2024

v18.20.0 proposal #52165

Merged

aduh95 added backported-to-v18.x and removed backport-requested-v18.x labels Nov 2, 2024

Uh oh!

Uh oh!

Conversation

joyeecheung commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

vm: use internal versions of compileFunction and Script

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

Uh oh!

nodejs-github-bot commented Oct 11, 2023

Uh oh!

joyeecheung commented Oct 11, 2023

Uh oh!

SimenB commented Oct 11, 2023

Uh oh!

joyeecheung commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 11, 2023

Uh oh!

joyeecheung commented Oct 12, 2023

Uh oh!

nodejs-github-bot commented Oct 12, 2023

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

targos commented Nov 11, 2023

Uh oh!

github-actions Bot commented Nov 12, 2023

Uh oh!

joyeecheung commented Nov 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

billoneil commented Nov 22, 2023

Uh oh!

joyeecheung commented Nov 23, 2023

Uh oh!

targos commented Nov 23, 2023

Uh oh!

joyeecheung commented Nov 24, 2023

Uh oh!

joyeecheung commented Nov 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joyeecheung commented Dec 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

15 participants

joyeecheung commented Oct 11, 2023 •

edited

Loading

joyeecheung commented Oct 11, 2023 •

edited

Loading

joyeecheung commented Nov 12, 2023 •

edited

Loading

joyeecheung commented Nov 25, 2023 •

edited

Loading