Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

v20.8.0 proposal #49932

Merged
merged 151 commits into from
Sep 29, 2023
Merged

v20.8.0 proposal #49932

merged 151 commits into from
Sep 29, 2023

Conversation

ruyadorno
Copy link
Member

@ruyadorno ruyadorno commented Sep 28, 2023

2023-09-28, Version 20.8.0 (Current), @ruyadorno

Notable Changes

Stream performance improvements

Performance improvements to writable and readable streams, improving the creation and destruction by ±15% and reducing the memory overhead each stream takes in Node.js

Contributed by Benjamin Gruenbaum in #49745 and Raz Luvaton in #49834.

Performance improvements for readable webstream, improving readable stream async iterator consumption by ±140% and improving readable stream pipeTo consumption by ±60%

Contributed by Raz Luvaton in #49662 and #49690.

Rework of memory management in vm APIs with the importModuleDynamically option

This rework addressed a series of long-standing memory leaks and use-after-free issues in the following APIs that support importModuleDynamically:

  • vm.Script
  • vm.compileFunction
  • vm.SyntheticModule
  • vm.SourceTextModule

This should enable affected users to upgrade from older versions of Node.js.

Contributed by Joyee Cheung in #48510.

Other notable changes

  • [32d4d29d02] - deps: add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) #49874
  • [0e686d096b] - doc: deprecate fs.F_OK, fs.R_OK, fs.W_OK, fs.X_OK (Livia Medeiros) #49683
  • [a5dd057540] - doc: deprecate util.toUSVString (Yagiz Nizipli) #49725
  • [7b6a73172f] - doc: deprecate calling promisify on a function that returns a promise (Antoine du Hamel) #49647
  • [1beefd5f16] - esm: set all hooks as release candidate (Geoffrey Booth) #49597
  • [b0ce78a75b] - module: fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) #48510
  • [4e578f8ab1] - module: fix leak of vm.SyntheticModule (Joyee Cheung) #48510
  • [69e4218772] - module: use symbol in WeakMap to manage host defined options (Joyee Cheung) #48510
  • [14ece0aa76] - (SEMVER-MINOR) src: allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) #49279
  • [9fd67fbff0] - stream: use bitmap in writable state (Raz Luvaton) #49834
  • [0ccd4638ac] - stream: use bitmap in readable state (Benjamin Gruenbaum) #49745
  • [7c5e322346] - stream: improve webstream readable async iterator performance (Raz Luvaton) #49662
  • [80b342cc38] - (SEMVER-MINOR) test_runner: accept testOnly in run (Moshe Atlow) #49753
  • [17a05b141d] - (SEMVER-MINOR) test_runner: add junit reporter (Moshe Atlow) #49614

Commits

joyeecheung and others added 30 commits September 27, 2023 22:17
We do not actually need to deserialize the context and the whole
environment to compile the code cache, since code cache are not
context-dependent anyway, deserializing just the isolate snapshot is
enough.

PR-URL: #49288
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
This makes it easier to locate indeterminism in the snapshot, with
the following command:

$ ./configure --write-snapshot-as-array-literals
$ make V=
$ mv out/Release/obj/gen/node_snapshot.cc ./node_snapshot.cc
$ make V=
$ diff out/Release/obj/gen/node_snapshot.cc ./node_snapshot.cc

PR-URL: #49312
Refs: nodejs/build#3043
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
PR-URL: #49406
Fixes: #49398
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Use a JS workload instead of repeating FS operations and use a
timer to make it less flaky on machines with little resources.

PR-URL: #49274
Refs: #26401
Refs: nodejs/reliability#640
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #49247
Refs: #49028
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Guy Bedford <guybedford@gmail.com>
PR-URL: #49464
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #49465
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Updated to rehype-raw@7.0.0 rehype-stringify@10.0.0 unified@11.0.2

PR-URL: #49467
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
PR-URL: #49476
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
PR-URL: #49427
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Ruy Adorno <ruyadorno@google.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #49471
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Signed-off-by: Erick Wendel <erick.workspace@gmail.com>
PR-URL: #49477
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
PR-URL: #47854
Refs: #47842
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Erick Wendel <erick.workspace@gmail.com>
PR-URL: #49456
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
PR-URL: #49482
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
The script was missing necessary imports for the `run`
function and the `path` module, causing it to fail.
This commit adds the missing imports and resolves the issue.

- Import `run` from the appropriate module.
- Import `path` to resolve file paths.

The script should now run without errors.

PR-URL: #49489
Fixes: #49488
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #49481
Fixes: #49417
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
PR-URL: #49493
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
PR-URL: #49493
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Bumps [rtCamp/action-slack-notify](https://github.com/rtcamp/action-slack-notify) from 2.2.0 to 2.2.1.
- [Release notes](https://github.com/rtcamp/action-slack-notify/releases)
- [Commits](rtCamp/action-slack-notify@12e36fc...b24d75f)

---
updated-dependencies:
- dependency-name: rtCamp/action-slack-notify
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: #49437
Refs: rtCamp/action-slack-notify@b24d75f
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Bumps [github/codeql-action](https://github.com/github/codeql-action) from 2.21.2 to 2.21.5.
- [Release notes](https://github.com/github/codeql-action/releases)
- [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md)
- [Commits](github/codeql-action@0ba4244...00e563e)

---
updated-dependencies:
- dependency-name: github/codeql-action
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>
PR-URL: #49438
Refs: github/codeql-action@00e563e
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Feature requests are issues, not PRs.

PR-URL: #49498
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
For the breaking change in
https://chromium-review.googlesource.com/c/v8/v8/+/4707972

PR-URL: #49439
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #49463
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #49470
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
It's an undocumented V8 behavior that is subject to change. Instead
just check if the internal field is set to a promise. There is also
no need to check IsEmpty() since the object is guaranteed to be
constructed by the FileHandle constructor with enough internal
fields.

PR-URL: #49413
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/4707972/comment/be9285cc_a49aad88/
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: #49193
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: #49162
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
PR-URL: #49525
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Locally this speeds up running test-crypto-dh* from 7s to 2s. This was
previously timing out in CI (took more than 2 minutes) so should see
a bigger gap in the CI.

PR-URL: #49492
Refs: #49202
Refs: nodejs/reliability#655
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
Reviewed-By: Filip Skokan <panva.ip@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Notable changes:

deps:
  * add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) #49874
doc:
  * deprecate `fs.F_OK`, `fs.R_OK`, `fs.W_OK`, `fs.X_OK` (Livia Medeiros) #49683
  * deprecate `util.toUSVString` (Yagiz Nizipli) #49725
  * deprecate calling `promisify` on a function that returns a promise (Antoine du Hamel) #49647
esm:
  * set all hooks as release candidate (Geoffrey Booth) #49597
module:
  * fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) #48510
  * fix leak of vm.SyntheticModule (Joyee Cheung) #48510
  * use symbol in WeakMap to manage host defined options (Joyee Cheung) #48510
src:
  * (SEMVER-MINOR) allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) #49279
stream:
  * use bitmap in writable state (Raz Luvaton) #49834
  * use bitmap in readable state (Benjamin Gruenbaum) #49745
  * improve webstream readable async iterator performance (Raz Luvaton) #49662
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614

PR-URL: #49932
@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member

joyeecheung commented Sep 29, 2023

I am a bit confused by the text of the stream performance changes...isn't ± supposed to mean positive AND negative? So that would mean the performance is wiggling between a e.g. +15% improvement and also a -15% hit? I am pretty sure that's not what it was trying to say, and it was probably trying to say that the improvement is "around" 15%...so it is supposed to use something like ~15% or (if you want to be less lazy) ≈15%?

@ruyadorno
Copy link
Member Author

ruyadorno commented Sep 29, 2023

Sorry @joyeecheung I see your message here but I'm actually already running the steps to promote the release 😬 I believe it's too late to update the changelog for this release but please do follow up with the conversation so that in the future we're sure to be using the most clear way to communicate these.

@ruyadorno ruyadorno merged commit a86c2ca into v20.x Sep 29, 2023
44 of 50 checks passed
@ruyadorno ruyadorno deleted the v20.8.0-proposal branch September 29, 2023 03:06
ruyadorno added a commit that referenced this pull request Sep 29, 2023
ruyadorno added a commit that referenced this pull request Sep 29, 2023
Notable changes:

deps:
  * add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) #49874
doc:
  * deprecate `fs.F_OK`, `fs.R_OK`, `fs.W_OK`, `fs.X_OK` (Livia Medeiros) #49683
  * deprecate `util.toUSVString` (Yagiz Nizipli) #49725
  * deprecate calling `promisify` on a function that returns a promise (Antoine du Hamel) #49647
esm:
  * set all hooks as release candidate (Geoffrey Booth) #49597
module:
  * fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) #48510
  * fix leak of vm.SyntheticModule (Joyee Cheung) #48510
  * use symbol in WeakMap to manage host defined options (Joyee Cheung) #48510
src:
  * (SEMVER-MINOR) allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) #49279
stream:
  * use bitmap in writable state (Raz Luvaton) #49834
  * use bitmap in readable state (Benjamin Gruenbaum) #49745
  * improve webstream readable async iterator performance (Raz Luvaton) #49662
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) #49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) #49614

PR-URL: #49932
ruyadorno added a commit to ruyadorno/nodejs.org that referenced this pull request Sep 29, 2023
ruyadorno added a commit to nodejs/nodejs.org that referenced this pull request Sep 29, 2023
@rluvaton
Copy link
Member

Can't we just update the change log manually?

@SimenB
Copy link
Member

SimenB commented Sep 29, 2023

Notable change text for the vm memory management rework

#### Rework of memory management in `vm` APIs with the `importModuleDynamically` option

This rework addressed a series of long-standing memory leaks and use-after-free issues in the following APIs that support `importModuleDynamically`:

- `vm.Script`
- `vm.compileFunction`
- `vm.SyntheticModule`
- `vm.SourceTextModule`

This should enable affected users (in particular Jest users) to upgrade from older versions of Node.js.

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

Hiya! 👋 Maintainer of Jest here. While I'm super happy to see the use-after-free issues resolved in this release of Node, I'd just like to point out that these release notes might give people a false sense of what was accomplished.

#48510 helps people who want to use native ESM in their Jest tests (i.e. --experimental-vm-modules) avoid segfaults. However, people are still stuck on an older version of Node.js (specifically 16.10) due to #35375 (comment), which is orthogonal to ESM APIs. If I run the reproduction in the OP of that issue report, I get these results (the interesting part is using Script, not the difference with compileFunction, which was the original purpose of that bug report):

$ nvm run 16.10 index.js
Running node v16.10.0 (npm v7.24.0)
Running with Script took 19 ms
Running with compileFunction took 1799 ms
Running with compileFunction and cached data took 145 ms

$ nvm run 16.11 index.js
Running node v16.11.1 (npm v8.0.0)
Running with Script took 1790 ms
Running with compileFunction took 1798 ms
Running with compileFunction and cached data took 147 ms

$ nvm run 20 index.js
Running node v20.8.0 (npm v10.1.0)
Running with Script took 1804 ms
Running with compileFunction took 1826 ms
Running with compileFunction and cached data took 141 ms

It's essentially an 100x regression in calling new Script (albeit with a large source file), which Jest does for every singe file loaded, for every single test file. And no cache, means a lot of wasted memory.

So while I, again, greatly appreciate the changes made in Node 20.8 to alleviate the segmentation faults when using ESM APIs, I think the language in the release post will give people a false hope that they can upgrade from an old (now EOL) version of Node.

❤️

@joyeecheung
Copy link
Member

joyeecheung commented Sep 29, 2023

@SimenB Thanks for explaining that, I think we can remove the "(in particular Jest users)" part, and just keep "affected users", as from what I know there are other projects getting blocked by the memory issues too (and is actually why I started looking into the issue).

@joyeecheung
Copy link
Member

joyeecheung commented Sep 29, 2023

Actually, I remember someone told me they had to use vm.Script to avoid the memory leak in vm.compileFunction, so once that gets fixed, they can switch back to vm.compileFunction (or is it the other way around with the segfault fixed? I don't quite remember). Not sure if that applies to Jest or not, though.

@SimenB
Copy link
Member

SimenB commented Sep 29, 2023

Yeah, that might be! We used compileFunction up until we encountered the regression in #35375 - seeing as code caching is broken in both cases now (😅), we might be able to go back to compileFunction. 🙂

FWIW, I attempted to use script.createCachedData() and sticking the caches in a Map - from my checks there's no difference doing so or not. But the caches are only per vm context, so if Node itself cached between contexts before, my approach is flawed.

@Uzlopak
Copy link
Contributor

Uzlopak commented Sep 29, 2023

Is it possible to replace my nickname with my realname?

From "Uzlopak" to "Aras Abbasi"?

@benjamingr
Copy link
Member

@Uzlopak where? In the release notes?

You can edit the blog post at pages/en/blog/release/v20.8.0.md in the website repo ( https://github.com/nodejs/nodejs.org ) and you can edit the changelog here: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V20.md

@benjamingr
Copy link
Member

(But to be clear, people in the project happily refer to you however you choose, I suspect in this case they just defaulted to your alias)

@ruyadorno
Copy link
Member Author

@Uzlopak I believe it's also possible to add a .mailmap entry to ensure you're always referred by that preferred name in the future.

alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
Notable changes:

deps:
  * add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) nodejs#49874
doc:
  * deprecate `fs.F_OK`, `fs.R_OK`, `fs.W_OK`, `fs.X_OK` (Livia Medeiros) nodejs#49683
  * deprecate `util.toUSVString` (Yagiz Nizipli) nodejs#49725
  * deprecate calling `promisify` on a function that returns a promise (Antoine du Hamel) nodejs#49647
esm:
  * set all hooks as release candidate (Geoffrey Booth) nodejs#49597
module:
  * fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) nodejs#48510
  * fix leak of vm.SyntheticModule (Joyee Cheung) nodejs#48510
  * use symbol in WeakMap to manage host defined options (Joyee Cheung) nodejs#48510
src:
  * (SEMVER-MINOR) allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) nodejs#49279
stream:
  * use bitmap in writable state (Raz Luvaton) nodejs#49834
  * use bitmap in readable state (Benjamin Gruenbaum) nodejs#49745
  * improve webstream readable async iterator performance (Raz Luvaton) nodejs#49662
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs#49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs#49614

PR-URL: nodejs#49932
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
Notable changes:

deps:
  * add v8::Object::SetInternalFieldForNodeCore() (Joyee Cheung) nodejs#49874
doc:
  * deprecate `fs.F_OK`, `fs.R_OK`, `fs.W_OK`, `fs.X_OK` (Livia Medeiros) nodejs#49683
  * deprecate `util.toUSVString` (Yagiz Nizipli) nodejs#49725
  * deprecate calling `promisify` on a function that returns a promise (Antoine du Hamel) nodejs#49647
esm:
  * set all hooks as release candidate (Geoffrey Booth) nodejs#49597
module:
  * fix the leak in SourceTextModule and ContextifySript (Joyee Cheung) nodejs#48510
  * fix leak of vm.SyntheticModule (Joyee Cheung) nodejs#48510
  * use symbol in WeakMap to manage host defined options (Joyee Cheung) nodejs#48510
src:
  * (SEMVER-MINOR) allow embedders to override NODE_MODULE_VERSION (Cheng Zhao) nodejs#49279
stream:
  * use bitmap in writable state (Raz Luvaton) nodejs#49834
  * use bitmap in readable state (Benjamin Gruenbaum) nodejs#49745
  * improve webstream readable async iterator performance (Raz Luvaton) nodejs#49662
test_runner:
  * (SEMVER-MINOR) accept `testOnly` in `run` (Moshe Atlow) nodejs#49753
  * (SEMVER-MINOR) add junit reporter (Moshe Atlow) nodejs#49614

PR-URL: nodejs#49932
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. dependencies Pull requests that update a dependency file. doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run. v20.x v20.x Issues that can be reproduced on v20.x or PRs targeting the v20.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.