Skip to content

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

@RaisinTen 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

@nodejs-github-bot

nodejs-github-bot commented May 26, 2023

Copy link
Copy Markdown
Collaborator

Review requested:

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

@nodejs-github-bot 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
@RaisinTen RaisinTen mentioned this pull request May 26, 2023
Closed
anonrig
anonrig reviewed May 26, 2023
View reviewed changes
Comment thread src/node_sea.cc Outdated
Comment thread src/node_contextify.cc Outdated
Comment thread src/node_sea.cc Outdated
@RaisinTen 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
joyeecheung
joyeecheung reviewed May 28, 2023
View reviewed changes
Comment thread lib/internal/util/embedding.js
jasnell
jasnell reviewed May 29, 2023
View reviewed changes
Comment thread src/node_contextify.cc Outdated
joyeecheung
joyeecheung reviewed May 31, 2023
View reviewed changes
Comment thread src/node_sea.cc Outdated
joyeecheung
joyeecheung reviewed Jun 1, 2023
View reviewed changes

@joyeecheung 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.

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
@RaisinTen

RaisinTen commented Jun 2, 2023

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 RaisinTen added the single-executable Issues and PRs related to single-executable applications label Jun 2, 2023
RaisinTen
RaisinTen commented Jun 2, 2023
View reviewed changes
Comment thread src/node_sea.cc
kvakil
kvakil reviewed Jun 9, 2023
View reviewed changes

@kvakil 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.

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

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 RaisinTen mentioned this pull request Jun 27, 2023
Closed
@RaisinTen RaisinTen force-pushed the sea-V8-code-cache branch from 300f3f4 to 91bf2df Compare July 21, 2023 07:28
@RaisinTen

RaisinTen commented Jul 21, 2023

Copy link
Copy Markdown
Member Author

Done @joyeecheung, PTAL

joyeecheung
joyeecheung approved these changes Jul 25, 2023
View reviewed changes

@joyeecheung 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.

LGTM, thanks

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions Bot mentioned this pull request Jul 26, 2023
Open
14 tasks
@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

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

@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 26, 2023
@nodejs-github-bot 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 nodejs-github-bot merged commit 6cd6789 into nodejs:main Jul 26, 2023
@nodejs-github-bot

nodejs-github-bot commented Jul 26, 2023

Copy link
Copy Markdown
Collaborator

Landed in 6cd6789

@RaisinTen RaisinTen deleted the sea-V8-code-cache branch July 26, 2023 10:25
This was referenced Jul 27, 2023
Open
Open
Open
Open
Open
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Merged
@robertsLando robertsLando mentioned this pull request Apr 10, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments

@Qard Qard Qard approved these changes

@anonrig anonrig anonrig approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

+2 more reviewers

@kvakil kvakil kvakil left review comments

@aymen94 aymen94 aymen94 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bytecode-only SEA caching?

8 participants

@RaisinTen @nodejs-github-bot @joyeecheung @Qard @jasnell @anonrig @aymen94 @kvakil