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

src: compile code eagerly in snapshot builder #51672

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 6, 2024

By default V8 only compiles the top-level function and skips code generation for inner functions - that would only be done when those inner functions are invoked. Since builtins are compiled as wrapped functions, most functions that look visually top-level are not actually included in the built-in code cache. For most of the builtins this is not too bad because usually only a subset of all builtin functions are needed by a particular
application and including all their code in the binary would incur an unnecessary size overhead. But there is also a subset of more commonly used builtins and it would be better to include the inner functions in the built-in code cache because they are more universally used by most applications.

This patch changes the compilation strategy to eager compilation
(including inner functions) for the following scripts:

  1. Primordials (internal/per_context/*), in all situations.
  2. Bootstrap scripts (internal/bootstrap/) and main scripts
    (internal/main/
    ), when being compiled for built-in code
    cache.
  3. Any scripts loaded during built-in snapshot generation.

We can't compile the code eagerly during snapshot generation
and include them into the V8 snapshot itself just now because
we need to start the inspector before context deserialization
for coverage collection to work. So leave that as a TODO.

With this patch the binary size increases by about 666KB
(~0.6% increase) in return the worker startup can be 18-19% faster.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @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 Feb 6, 2024
@joyeecheung
Copy link
Member Author

Just noticed that we can't compile the code eagerly during snapshot creation just yet because that requires refactoring of the V8ProfilerConnection classes. Added a TODO instead. The accompanying code cache is still compiled eagerly.

New numbers (only workers are more significantly impacted for now):

                                                                                     confidence improvement accuracy (*)   (**)  (***)
misc/startup-cli-version.js count=30 cli='deps/corepack/dist/corepack.js'                           -0.40 %       ±0.98% ±1.30% ±1.69%
misc/startup-cli-version.js count=30 cli='deps/npm/bin/npm-cli.js'                                  -0.15 %       ±0.74% ±0.99% ±1.29%
misc/startup-cli-version.js count=30 cli='deps/npm/bin/npx-cli.js'                                  -0.18 %       ±1.02% ±1.36% ±1.77%
misc/startup-cli-version.js count=30 cli='tools/node_modules/eslint/bin/eslint.js'                  -0.63 %       ±0.85% ±1.13% ±1.47%
misc/startup.js count=30 mode='process' script='benchmark/fixtures/require-builtins'                 1.11 %       ±2.52% ±3.37% ±4.42%
misc/startup.js count=30 mode='process' script='test/fixtures/semicolon'                             0.29 %       ±2.07% ±2.76% ±3.59%
misc/startup.js count=30 mode='worker' script='benchmark/fixtures/require-builtins'         ***     18.46 %       ±3.94% ±5.27% ±6.94%
misc/startup.js count=30 mode='worker' script='test/fixtures/semicolon'                     ***     19.29 %       ±4.84% ±6.47% ±8.46%

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 15, 2024
@joyeecheung joyeecheung force-pushed the eager-compile branch 3 times, most recently from 4e42997 to c4020c3 Compare February 15, 2024 17:52
@nodejs nodejs deleted a comment from nodejs-github-bot Feb 15, 2024
@nodejs nodejs deleted a comment from nodejs-github-bot Feb 15, 2024
@nodejs-github-bot
Copy link
Collaborator

By default V8 only compiles the top-level function and
skips code generation for inner functions - that would
only be done when those inner functions are invoked.
Since builtins are compiled as wrapped functions, most
functions that look visually top-level are not actually
included in the built-in code cache. For most of the
builtins this is not too bad because usually only a subset of
all builtin functions are needed by a particular
application and including all their code in the binary
would incur an unnecessary size overhead. But there is also
a subset of more commonly used builtins and it would be
better to include the inner functions in the built-in
code cache because they are more universally used by
most applications.

This patch changes the compilation strategy to eager compilation
(including inner functions) for the following scripts:

1. Primordials (internal/per_context/*), in all situations.
2. Bootstrap scripts (internal/bootstrap/*) and main scripts
   (internal/main/*), when being compiled for built-in code
   cache.
3. Any scripts loaded during built-in snapshot generation.

We can't compile the code eagerly during snapshot generation
and include them into the V8 snapshot itself just now because
we need to start the inspector before context deserialization
for coverage collection to work. So leave that as a TODO.

With this patch the binary size increases by about 666KB
(~0.6% increase) in return the worker startup can be 18-19% faster.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@joyeecheung joyeecheung added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 20, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 20, 2024
@nodejs-github-bot nodejs-github-bot merged commit 3e57b93 into nodejs:main Feb 20, 2024
55 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3e57b93

marco-ippolito pushed a commit that referenced this pull request Feb 26, 2024
By default V8 only compiles the top-level function and
skips code generation for inner functions - that would
only be done when those inner functions are invoked.
Since builtins are compiled as wrapped functions, most
functions that look visually top-level are not actually
included in the built-in code cache. For most of the
builtins this is not too bad because usually only a subset of
all builtin functions are needed by a particular
application and including all their code in the binary
would incur an unnecessary size overhead. But there is also
a subset of more commonly used builtins and it would be
better to include the inner functions in the built-in
code cache because they are more universally used by
most applications.

This patch changes the compilation strategy to eager compilation
(including inner functions) for the following scripts:

1. Primordials (internal/per_context/*), in all situations.
2. Bootstrap scripts (internal/bootstrap/*) and main scripts
   (internal/main/*), when being compiled for built-in code
   cache.
3. Any scripts loaded during built-in snapshot generation.

We can't compile the code eagerly during snapshot generation
and include them into the V8 snapshot itself just now because
we need to start the inspector before context deserialization
for coverage collection to work. So leave that as a TODO.

With this patch the binary size increases by about 666KB
(~0.6% increase) in return the worker startup can be 18-19% faster.

PR-URL: #51672
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 20, 2024
By default V8 only compiles the top-level function and
skips code generation for inner functions - that would
only be done when those inner functions are invoked.
Since builtins are compiled as wrapped functions, most
functions that look visually top-level are not actually
included in the built-in code cache. For most of the
builtins this is not too bad because usually only a subset of
all builtin functions are needed by a particular
application and including all their code in the binary
would incur an unnecessary size overhead. But there is also
a subset of more commonly used builtins and it would be
better to include the inner functions in the built-in
code cache because they are more universally used by
most applications.

This patch changes the compilation strategy to eager compilation
(including inner functions) for the following scripts:

1. Primordials (internal/per_context/*), in all situations.
2. Bootstrap scripts (internal/bootstrap/*) and main scripts
   (internal/main/*), when being compiled for built-in code
   cache.
3. Any scripts loaded during built-in snapshot generation.

We can't compile the code eagerly during snapshot generation
and include them into the V8 snapshot itself just now because
we need to start the inspector before context deserialization
for coverage collection to work. So leave that as a TODO.

With this patch the binary size increases by about 666KB
(~0.6% increase) in return the worker startup can be 18-19% faster.

PR-URL: nodejs#51672
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
By default V8 only compiles the top-level function and
skips code generation for inner functions - that would
only be done when those inner functions are invoked.
Since builtins are compiled as wrapped functions, most
functions that look visually top-level are not actually
included in the built-in code cache. For most of the
builtins this is not too bad because usually only a subset of
all builtin functions are needed by a particular
application and including all their code in the binary
would incur an unnecessary size overhead. But there is also
a subset of more commonly used builtins and it would be
better to include the inner functions in the built-in
code cache because they are more universally used by
most applications.

This patch changes the compilation strategy to eager compilation
(including inner functions) for the following scripts:

1. Primordials (internal/per_context/*), in all situations.
2. Bootstrap scripts (internal/bootstrap/*) and main scripts
   (internal/main/*), when being compiled for built-in code
   cache.
3. Any scripts loaded during built-in snapshot generation.

We can't compile the code eagerly during snapshot generation
and include them into the V8 snapshot itself just now because
we need to start the inspector before context deserialization
for coverage collection to work. So leave that as a TODO.

With this patch the binary size increases by about 666KB
(~0.6% increase) in return the worker startup can be 18-19% faster.

PR-URL: #51672
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
By default V8 only compiles the top-level function and
skips code generation for inner functions - that would
only be done when those inner functions are invoked.
Since builtins are compiled as wrapped functions, most
functions that look visually top-level are not actually
included in the built-in code cache. For most of the
builtins this is not too bad because usually only a subset of
all builtin functions are needed by a particular
application and including all their code in the binary
would incur an unnecessary size overhead. But there is also
a subset of more commonly used builtins and it would be
better to include the inner functions in the built-in
code cache because they are more universally used by
most applications.

This patch changes the compilation strategy to eager compilation
(including inner functions) for the following scripts:

1. Primordials (internal/per_context/*), in all situations.
2. Bootstrap scripts (internal/bootstrap/*) and main scripts
   (internal/main/*), when being compiled for built-in code
   cache.
3. Any scripts loaded during built-in snapshot generation.

We can't compile the code eagerly during snapshot generation
and include them into the V8 snapshot itself just now because
we need to start the inspector before context deserialization
for coverage collection to work. So leave that as a TODO.

With this patch the binary size increases by about 666KB
(~0.6% increase) in return the worker startup can be 18-19% faster.

PR-URL: #51672
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants