build: create V8 code cache after script is run - #21567
Conversation
Sorry, something went wrong.
|
@joyeecheung build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/168/pipeline |
Sorry, something went wrong.
|
From #21563
@hashseed from the logs (posted in the OP) it does not look like the size changes much. Some of them changed about 1-2KB, but most of them are unchanged. I excluded the scripts under deps and the prof processor polyfills so the set of modules being cached is not exactly the same. |
Sorry, something went wrong.
|
Seems like most scripts run during bootstrapping are straight-line code that only set up things and do not often call inner functions, which is why not a lot of lazily compiled functions get included, and therefore there is no benefit. |
Sorry, something went wrong.
|
How come we don't see acorn being cached in the log for after? |
Sorry, something went wrong.
There was a problem hiding this comment.
I'm not sure I understand this comment. Why can't these scripts be cached?
Sorry, something went wrong.
There was a problem hiding this comment.
- config is a JSON string, it's not going to be wrapped
- sys is deprecated and is redirected from util (executing it would trigger a deprecation warning)
- internal/test/binding triggers a warning as well (also, it's not supposed to be loaded by users)
- internal/v8_prof_polyfill is not supposed to be required directly (see the comments there)
- internal/v8_prof_processor is an actual script (instead of a module) that is supposed to write a log file
The files in v8/tools are not really Node.js modules. They are mostly concatenated and run by internal/v8_prof_processor since they expose stuff to the global space and rely on that.
Sorry, something went wrong.
There was a problem hiding this comment.
I see. Thanks for the explanation.
Sorry, something went wrong.
I've removed the internal/deps modules for now since there is no guarantee that they are Node.js modules that can be required (instead of scripts that are supposed to be run directly). We could add a white list for those that actually are. |
Sorry, something went wrong.
There was a problem hiding this comment.
^ unneeded var assignment return cached.script.createCachedData()
Sorry, something went wrong.
This patch makes it possible to generate the code cache for the builtins directly from the original script object (instead of compiling a new one) and after the script has been run (via `NativeModule.require`). Before this patch only the top level functions (the wrapped ones) are included in the cache, after this patch the inner functions in those modules will be included as well. Also blacklists modules from dependencies like V8 and node-inspect since we cannot guarantee that they are suitable to be executed directly.
|
Rebased and addressed review from @jdalton. I think this is worth landing even though it does not affect the performance since it cleans up the code a bit and we no longer need to recompile the source. |
Sorry, something went wrong.
Sorry, something went wrong.
|
Another CI is green: https://ci.nodejs.org/job/node-test-pull-request/16001/ |
Sorry, something went wrong.
This patch makes it possible to generate the code cache for the builtins directly from the original script object (instead of compiling a new one) and after the script has been run (via `NativeModule.require`). Before this patch only the top level functions (the wrapped ones) are included in the cache, after this patch the inner functions in those modules will be included as well. Also blacklists modules from dependencies like V8 and node-inspect since we cannot guarantee that they are suitable to be executed directly. PR-URL: #21567 Refs: #21563 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This patch makes it possible to generate the code cache for the builtins directly from the original script object (instead of compiling a new one) and after the script has been run (via `NativeModule.require`). Before this patch only the top level functions (the wrapped ones) are included in the cache, after this patch the inner functions in those modules will be included as well. Also blacklists modules from dependencies like V8 and node-inspect since we cannot guarantee that they are suitable to be executed directly. PR-URL: #21567 Refs: #21563 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This patch makes it possible to generate the code cache
for the builtins directly from the original script object
(instead of compiling a new one) and after the script has
been run (via NativeModule.require). Before this patch
only the top level functions (the wrapped ones)
are included in the cache, after this patch the inner
functions in those modules will be included as well.
Also blacklists modules from dependencies like V8 and
node-inspect since we cannot guarantee that they are suitable
to be executed directly.
Refs: #21563
Performance stats
Ideally this should improve the startup time now that we include more into the cache,
but here is what I get now:
Before this patch, the logs of tools/generate_code_cache.js
See logsAfter:
See logsChecklist