FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

sea: add support for V8 bytecode-only caching by RaisinTen · Pull Request #48191 · nodejs/node · GitHub

/ node Public

sea: add support for V8 bytecode-only caching - #48191

Merged
nodejs-github-bot merged 27 commits into
nodejs:mainfrom
RaisinTen:sea-V8-code-cache
Jul 26, 2023
Merged

sea: add support for V8 bytecode-only caching#48191
nodejs-github-bot merged 27 commits into
nodejs:mainfrom
RaisinTen:sea-V8-code-cache

Conversation

RaisinTen commented May 26, 2023
edited
Loading

Copy link
Copy Markdown
Member

This improves the startup performance of SEAs.

For example, the comparison of the startup performance benchmarks of a Node.js SEA built using the CJS bundle of Yarn's source code shows a 24% improvement on my x86_64 macOS:

$ hyperfine -N -w 30 -L cmd 'with_code_cache/yarn help','without_code_cache/yarn help' '{cmd}'
Benchmark 1: with_code_cache/yarn help
  Time (mean ± σ):     210.8 ms ±   1.3 ms    [User: 224.0 ms, System: 25.2 ms]
  Range (min  max):   209.0 ms  213.4 ms    14 runs

Benchmark 2: without_code_cache/yarn help
  Time (mean ± σ):     279.1 ms ±   2.3 ms    [User: 284.9 ms, System: 30.1 ms]
  Range (min  max):   276.6 ms  284.4 ms    10 runs

Summary
  'with_code_cache/yarn help' ran
    1.32 ± 0.01 times faster than 'without_code_cache/yarn help'

Fixes: nodejs/single-executable#73

cc @kvakil

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/single-executable
  • @nodejs/startup

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. 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 May 26, 2023
Comment thread src/node_sea.cc Outdated
Comment thread src/node_contextify.cc Outdated
Comment thread src/node_sea.cc Outdated
RaisinTen added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label May 27, 2023
Comment thread lib/internal/util/embedding.js Outdated
Comment thread src/node_contextify.cc Outdated
Comment thread src/node_sea.cc Outdated

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this includes any tests for the new feature? We should at least have some to exercise the code paths..

Comment thread src/node_sea.cc Outdated
Comment thread lib/internal/modules/cjs/loader.js Outdated

Copy link
Copy Markdown
Member Author

I don't think this includes any tests for the new feature? We should at least have some to exercise the code paths..

This change generates the code cache and uses it by default, so the existing E2E SEA tests should already be exercising the added code paths, right?

RaisinTen added the single-executable Issues and PRs related to single-executable applications label Jun 2, 2023
Comment thread src/node_sea.cc Outdated

kvakil left a comment
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, this is great! I left some nits

(One last note wrt startup time: it's unfortunate that we store the source code as UTF8 because then we need to transcode/copy it into Latin1/UTF16 in order to actually put it into V8. Ideally we'd use external strings instead. That's not really related to this diff, but thought I'd note it since we're one the subject. :)

Comment thread src/node_contextify.cc Outdated
Comment thread src/node_contextify.cc Outdated
Comment thread src/node_sea.cc Outdated

joyeecheung commented Jun 10, 2023
edited
Loading

Copy link
Copy Markdown
Member

it's unfortunate that we store the source code as UTF8 because then we need to transcode/copy it into Latin1/UTF16 in order to actually put it into V8

I think this could be made configurable because storing the strings as UTF8 could save quite some space if the source actually contains two-byte characters, which may be more important to users than the startup cost depending on their use case.

RaisinTen force-pushed the sea-V8-code-cache branch from 300f3f4 to 91bf2df Compare July 21, 2023 07:28

Copy link
Copy Markdown
Member Author

Done @joyeecheung, PTAL

joyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks

This comment was marked as outdated.

Copy link
Copy Markdown
Collaborator

RaisinTen added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 26, 2023
nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 26, 2023
nodejs-github-bot merged commit 6cd6789 into nodejs:main Jul 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 6cd6789

RaisinTen deleted the sea-V8-code-cache branch July 26, 2023 10:25
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191
Fixes: nodejs/single-executable#73
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191
Fixes: nodejs/single-executable#73
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191
Fixes: nodejs/single-executable#73
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
UlisesGascon pushed a commit to UlisesGascon/node that referenced this pull request Aug 14, 2023
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191
Fixes: nodejs/single-executable#73
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Aug 15, 2023
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #48191
Fixes: nodejs/single-executable#73
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
UlisesGascon mentioned this pull request Aug 15, 2023
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #48191
Fixes: nodejs/single-executable#73
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request Nov 6, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. single-executable Issues and PRs related to single-executable applications

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bytecode-only SEA caching?

8 participants


Back | FazBrowse Home | New Git URL