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

[v18.x] backport startup performance patches #46425

Closed
wants to merge 13 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 30, 2023

This is a backport of #45659 and #45849, the first two commits are specifically added for v18.x with the second one taken from #44483.

Refs: #46425

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/net
  • @nodejs/startup

@nodejs-github-bot 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. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch. labels Jan 30, 2023
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Looks good, there’s just one typo in the extra commit messages (gloablThis) 🙂

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

@panva panva added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 31, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung
Copy link
Member Author

I cannot reproduce the snapshot test failures locally on similar platforms. It could be a flake, or maybe something wasn't backported properly for v18.x. Will need to wait until the CI is unlocked to do a bit more runs to find out.

@juanarbol
Copy link
Member

I would love to land this in the v18.15.0 release <3, I was going to kick a CI for this but it needs a rebase first.

@danielleadams
Copy link
Contributor

@joyeecheung It looks like this needs a rebase

@krk
Copy link
Contributor

krk commented Apr 26, 2023

Hi! What is the expected performance gain in startup with this PR?

@ruyadorno
Copy link
Member

hi @joyeecheung this PR needs rebasing in case we want to land it in v18.x

So that we can add multiple initialization scripts in the test runner.

Refs: nodejs#44483
It turns out that even with startup snapshots, there is a non-trivial
overhead for loading internal modules. This patch makes the loading
of the non-essential modules lazy again.

Caveat: we have to make some of the globals lazily-loaded too,
so the WPT runner is updated to test what the state of the global
scope is after the globals are accessed (and replaced with the
loaded value).

PR-URL: nodejs#45659
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
targos pushed a commit that referenced this pull request Nov 10, 2023
This patch makes the top-level access to runtime states in the
CJS loader lazy, and move the side-effects into a
initializeCJS() function that gets called during pre-execution.
As a result the CJS loader can be included into the built-in
snapshot.

PR-URL: #45849
Backport-PR-URL: #46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this pull request Nov 10, 2023
Preload essential modules and lazy-load non-essential ones.
After this patch, all modules listed by running this snippet:

```
const list = process.moduleLoadList.join('\n');
require('fs').writeSync(1, list, 'utf-8');
```

(which is roughly the same list as the one in
test-bootstrap-module.js for the main thread)
are loaded from the snapshot so no additional compilation cost
is incurred.

PR-URL: #45849
Backport-PR-URL: #46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos added a commit that referenced this pull request Nov 10, 2023
Original commit message:

    [snapshot] Dont defer ByteArray when serializing

    JSTypedArray needs the base_pointer ByteArray immediately
    if it's on heap. JSTypedArray's base_pointer was initialized
    to Smi::uninitialized_deserialization_value at first when
    deserializing, and if base_pointer was deferred, we will
    mistakenly check JSTypedArray not on heap.

    Bug: v8:13149
    Change-Id: I104c83ff9a2017de1c8071a9e116baa602f6977d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3813068
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Commit-Queue: 王澳 <wangao.james@bytedance.com>
    Cr-Commit-Position: refs/heads/main@{#82254}

Refs: v8/v8@d69c793
PR-URL: #46425
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#46425
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
So that we can add multiple initialization scripts in the test runner.

Refs: nodejs/node#44483
PR-URL: nodejs/node#46425
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
It turns out that even with startup snapshots, there is a non-trivial
overhead for loading internal modules. This patch makes the loading
of the non-essential modules lazy again.

Caveat: we have to make some of the globals lazily-loaded too,
so the WPT runner is updated to test what the state of the global
scope is after the globals are accessed (and replaced with the
loaded value).

PR-URL: nodejs/node#45659
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Since the module has to be loaded during bootstrap anyway.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
The helpers are actually shared by the two loaders, so move them
under modules/ directly.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This patch adds a getLazy() method to facilitate initialize-once
lazy loading in the internals.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
So that the file can be snapshotted.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
So that the file can be snapshotted

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This moves the following utils into modules/esm/utils.js:

- Code related to default conditions
- The callbackMap (which is now created in the module instead of
  hanging off the module_wrap binding, since the C++ land
  does not need it).
- Per-isolate module callbacks

These are self-contained code that can be included into the
built-in snapshot.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This patch makes the top-level access to runtime states in the
CJS loader lazy, and move the side-effects into a
initializeCJS() function that gets called during pre-execution.
As a result the CJS loader can be included into the built-in
snapshot.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Preload essential modules and lazy-load non-essential ones.
After this patch, all modules listed by running this snippet:

```
const list = process.moduleLoadList.join('\n');
require('fs').writeSync(1, list, 'utf-8');
```

(which is roughly the same list as the one in
test-bootstrap-module.js for the main thread)
are loaded from the snapshot so no additional compilation cost
is incurred.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Original commit message:

    [snapshot] Dont defer ByteArray when serializing

    JSTypedArray needs the base_pointer ByteArray immediately
    if it's on heap. JSTypedArray's base_pointer was initialized
    to Smi::uninitialized_deserialization_value at first when
    deserializing, and if base_pointer was deferred, we will
    mistakenly check JSTypedArray not on heap.

    Bug: v8:13149
    Change-Id: I104c83ff9a2017de1c8071a9e116baa602f6977d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3813068
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Commit-Queue: 王澳 <wangao.james@bytedance.com>
    Cr-Commit-Position: refs/heads/main@{#82254}

Refs: v8/v8@d69c793
PR-URL: nodejs/node#46425
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#46425
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
So that we can add multiple initialization scripts in the test runner.

Refs: nodejs/node#44483
PR-URL: nodejs/node#46425
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
It turns out that even with startup snapshots, there is a non-trivial
overhead for loading internal modules. This patch makes the loading
of the non-essential modules lazy again.

Caveat: we have to make some of the globals lazily-loaded too,
so the WPT runner is updated to test what the state of the global
scope is after the globals are accessed (and replaced with the
loaded value).

PR-URL: nodejs/node#45659
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Daeyeon Jeong <daeyeon.dev@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Since the module has to be loaded during bootstrap anyway.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
The helpers are actually shared by the two loaders, so move them
under modules/ directly.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This patch adds a getLazy() method to facilitate initialize-once
lazy loading in the internals.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
So that the file can be snapshotted.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
So that the file can be snapshotted

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This moves the following utils into modules/esm/utils.js:

- Code related to default conditions
- The callbackMap (which is now created in the module instead of
  hanging off the module_wrap binding, since the C++ land
  does not need it).
- Per-isolate module callbacks

These are self-contained code that can be included into the
built-in snapshot.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
This patch makes the top-level access to runtime states in the
CJS loader lazy, and move the side-effects into a
initializeCJS() function that gets called during pre-execution.
As a result the CJS loader can be included into the built-in
snapshot.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Preload essential modules and lazy-load non-essential ones.
After this patch, all modules listed by running this snippet:

```
const list = process.moduleLoadList.join('\n');
require('fs').writeSync(1, list, 'utf-8');
```

(which is roughly the same list as the one in
test-bootstrap-module.js for the main thread)
are loaded from the snapshot so no additional compilation cost
is incurred.

PR-URL: nodejs/node#45849
Backport-PR-URL: nodejs/node#46425
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
sercher added a commit to sercher/graaljs that referenced this pull request Apr 25, 2024
Original commit message:

    [snapshot] Dont defer ByteArray when serializing

    JSTypedArray needs the base_pointer ByteArray immediately
    if it's on heap. JSTypedArray's base_pointer was initialized
    to Smi::uninitialized_deserialization_value at first when
    deserializing, and if base_pointer was deferred, we will
    mistakenly check JSTypedArray not on heap.

    Bug: v8:13149
    Change-Id: I104c83ff9a2017de1c8071a9e116baa602f6977d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3813068
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Commit-Queue: 王澳 <wangao.james@bytedance.com>
    Cr-Commit-Position: refs/heads/main@{#82254}

Refs: v8/v8@d69c793
PR-URL: nodejs/node#46425
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. v18.x Issues that can be reproduced on v18.x or PRs targeting the v18.x-staging branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.