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

@RaisinTenRaisinTen 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-w30-Lcmd'with_code_cache/yarn help','without_code_cache/yarn help''{cmd}'
Benchmark 1: with_code_cache/yarnhelpTime(mean±σ): 210.8ms±1.3ms[User: 224.0ms,System: 25.2ms]Range(min…max): 209.0ms…213.4ms14runs
Benchmark 2: without_code_cache/yarnhelpTime(mean±σ): 279.1ms±2.3ms[User: 284.9ms,System: 30.1ms]Range(min…max): 276.6ms…284.4ms10runsSummary'with_code_cache/yarn help'ran1.32±0.01timesfasterthan'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-botnodejs-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
@RaisinTenRaisinTen mentioned this pull request May 26, 2023
Closed
anonrig
anonrig reviewed May 26, 2023
View reviewed changes
Comment threadsrc/node_sea.cc Outdated
Comment threadsrc/node_contextify.cc Outdated
Comment threadsrc/node_sea.cc Outdated
@RaisinTenRaisinTen 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 threadlib/internal/util/embedding.js Outdated
jasnell
jasnell reviewed May 29, 2023
View reviewed changes
Comment threadsrc/node_contextify.cc Outdated
joyeecheung
joyeecheung reviewed May 31, 2023
View reviewed changes
Comment threadsrc/node_sea.cc Outdated
joyeecheung
joyeecheung reviewed Jun 1, 2023
View reviewed changes

@joyeecheungjoyeecheung 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 threadsrc/node_sea.cc Outdated
Comment threadlib/internal/modules/cjs/loader.js Outdated
@RaisinTen

RaisinTen commented Jun 2, 2023

Copy link
Copy Markdown
MemberAuthor

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?

@RaisinTenRaisinTen 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 threadsrc/node_sea.cc Outdated
kvakil
kvakil reviewed Jun 9, 2023
View reviewed changes

@kvakilkvakil 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 threadsrc/node_contextify.cc Outdated
Comment threadsrc/node_contextify.cc Outdated
Comment threadsrc/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.

@RaisinTenRaisinTen mentioned this pull request Jun 27, 2023
Closed
@RaisinTen

RaisinTen commented Jul 21, 2023

Copy link
Copy Markdown
MemberAuthor

Done @joyeecheung, PTAL

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

@joyeecheungjoyeecheung 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-actionsgithub-actionsBot 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/

@RaisinTenRaisinTen 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-botnodejs-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:mainJul 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
@github-actionsgithub-actionsBot mentioned this pull request Jul 27, 2023
Open
18 tasks
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Jul 27, 2023
@RaisinTen@Ceres6
sea: add support for V8 bytecode-only caching
5c9ad5c
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191Fixes: 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>
This was referenced Jul 28, 2023
Open
Open
Open
Open
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@RaisinTen@Ceres6
sea: add support for V8 bytecode-only caching
6ae2d74
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191Fixes: 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
@RaisinTen@Ceres6
sea: add support for V8 bytecode-only caching
7a8bb62
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191Fixes: 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
@RaisinTen@UlisesGascon
sea: add support for V8 bytecode-only caching
3de883d
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: nodejs#48191Fixes: 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
@RaisinTen@RafaelGSS
sea: add support for V8 bytecode-only caching
b206233
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #48191Fixes: 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>
@UlisesGasconUlisesGascon mentioned this pull request Aug 15, 2023
Merged
RafaelGSS pushed a commit that referenced this pull request Aug 17, 2023
@RaisinTen@RafaelGSS
sea: add support for V8 bytecode-only caching
bc009d0
Refs: nodejs/single-executable#73
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #48191Fixes: 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
@codebytere
sea: add support for V8 bytecode-only caching
43c93e2
nodejs/node#48191
@robertsLandorobertsLando mentioned this pull request Apr 10, 2026
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments
@QardQardQard approved these changes
@anonriganonriganonrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes
+2 more reviewers
@kvakilkvakilkvakil left review comments
@aymen94aymen94aymen94 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs 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-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.lib / srcIssues and PRs related to general changes in the lib or src directory.needs-ciPRs that need a full CI run.single-executableIssues 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