Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
nodejs-github-bot merged 27 commits into from
Jul 26, 2023
Merged

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

nodejs-github-bot merged 27 commits into from
Jul 26, 2023

Conversation

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented May 26, 2023 •
edited
Loading

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
Copy link
Collaborator

nodejs-github-bot commented May 26, 2023

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
RaisinTen added a commit to RaisinTen/node that referenced this pull request May 27, 2023
@RaisinTen
src: use std::nullopt instead of {}
d8593ca 
It expresses the intent in a clearer way.

Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
RaisinTen added a commit to RaisinTen/node that referenced this pull request May 27, 2023
@RaisinTen
src: pass parameters vector by a pointer
05199d8 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
@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
RaisinTen added a commit to RaisinTen/node that referenced this pull request May 29, 2023
@RaisinTen
sea: fix entry point file name
397f384 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
jasnell
jasnell reviewed May 29, 2023
View reviewed changes
joyeecheung
joyeecheung reviewed May 31, 2023
View reviewed changes
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jun 1, 2023
@RaisinTen
src: use DataView instead of Buffer for code cache
5dfdbd1 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
joyeecheung
joyeecheung reviewed Jun 1, 2023
View reviewed changes
Copy link
Member

@joyeecheung joyeecheung left a comment

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

@RaisinTen
Copy link
Member Author

RaisinTen commented Jun 2, 2023

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 a commit to RaisinTen/node that referenced this pull request Jun 2, 2023
@RaisinTen
sea: warn if code cache is rejected
1610dd6 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
@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
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jun 8, 2023
@RaisinTen
src: add TODO for supporting import()
7a19ac1 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
kvakil
kvakil reviewed Jun 9, 2023
View reviewed changes
Copy link
Contributor

@kvakil kvakil left a comment •
edited
Loading

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. :)

@joyeecheung
Copy link
Member

joyeecheung commented Jun 10, 2023 •
edited
Loading

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 added a commit to RaisinTen/node that referenced this pull request Jun 27, 2023
@RaisinTen
deps: V8: cherry-pick cb00db4dba6c
dfc9435 
Original commit message:

    [compiler] fix CompileFunction ignoring kEagerCompile

    v8::ScriptCompiler::CompileFunction was ignoring kEagerCompile. Unlike
    the other functions in v8::ScriptCompiler, it was not actually
    propagating kEagerCompile to the parser. The newly updated test fails
    without this change.

    I did some archeology and found that this was commented out since the
    original CL in https://crrev.com/c/980944.

    As far as I know Node.js is the main consumer of this particular API.
    This CL speeds up Node.js's overall startup time by ~13%.

    Change-Id: Ifc3cd6653555194d46ca48db14f7ba7a4afe0053
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4571822
    Commit-Queue: Marja Hölttä <[email protected]>
    Reviewed-by: Marja Hölttä <[email protected]>
    Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
@RaisinTen RaisinTen mentioned this pull request Jun 27, 2023
Closed
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jun 27, 2023
@RaisinTen
src: use std::nullopt instead of {}
ee798d6 
It expresses the intent in a clearer way.

Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jun 27, 2023
@RaisinTen
src: pass parameters vector by a pointer
dc8bca5 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jun 27, 2023
@RaisinTen
sea: fix entry point file name
f5eec14 
Refs: nodejs#48191 (comment)
Signed-off-by: Darshan Sen <[email protected]>
@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
Copy link
Collaborator

nodejs-github-bot commented Jul 26, 2023

Landed in 6cd6789

@RaisinTen RaisinTen deleted the sea-V8-code-cache branch July 26, 2023 10:25
@github-actions github-actions bot mentioned this pull request Jul 27, 2023
Open
18 tasks
This was referenced Jul 28, 2023
Open
Open
Open
Open
@UlisesGascon UlisesGascon mentioned this pull request Aug 15, 2023
Merged
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