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

build: tracking issue for V8 code cache intergration #21563

Closed
7 tasks done
joyeecheung opened this issue Jun 27, 2018 · 11 comments
Closed
7 tasks done

build: tracking issue for V8 code cache intergration #21563

joyeecheung opened this issue Jun 27, 2018 · 11 comments
Labels
build Issues and PRs related to build files or the CI. module Issues and PRs related to the module subsystem.

Comments

@joyeecheung
Copy link
Member

joyeecheung commented Jun 27, 2018

Following up on #21405

  • Better integrity check when using the code cache in NativeModule.prototype.compile (right now SerializedCodeData::SourceHash returns source->length()) e.g. calculating the checksums of the source code in both js2c and the code cache generator and compare them before using the cache. See build: speed up startup with V8 code cache #21405 (comment) simply relying on V8 to do the checks is fine
  • build: create V8 code cache after script is run #21567: Use script.createCachedData to generate the code cache after the builtin modules have runInThisContext(), so that the cache of the lazily compiled inner functions are also included (right now only the top level function is included in the cache),see build: speed up startup with V8 code cache #21405 (comment) . We may need to include the code cache generator in core or expose the scripts to user land.
  • Cache internal/bootstrap/loaders.js and internal/bootstrap/node.js (these two are compiled in C++): src: use NativeModuleLoader to compile all the bootstrappers #24775
  • (might be related if we still use JS to generate the cache) Add support for the full range of CompileOptions (and possibly NoCacheReason) to ContextifyScript/vm.Script it's not using the JS land function anymore

Long-term action items

@joyeecheung joyeecheung added the build Issues and PRs related to build files or the CI. label Jun 27, 2018
@joyeecheung
Copy link
Member Author

cc @devsnek @hashseed @TimothyGu

@joyeecheung joyeecheung added the module Issues and PRs related to the module subsystem. label Jun 27, 2018
@joyeecheung
Copy link
Member Author

BTW: I explored the idea of generating the code cache after the script has been run and it does not seem to make much difference:

See benchmark results
                       confidence improvement accuracy (*)   (**)  (***)
 misc/startup.js dur=1                -0.18 %       ±1.89% ±2.52% ±3.28%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 1 comparisons, you can thus
expect the following amount of false-positive results:
  0.05 false positives, when considering a   5% risk acceptance (*, **, ***),
  0.01 false positives, when considering a   1% risk acceptance (**, ***),
  0.00 false positives, when considering a 0.1% risk acceptance (***)

@hashseed
Copy link
Member

hashseed commented Jun 27, 2018

As a sanity check, does the code cache size change significantly if you create it after the script has been run?

How many functions do you see lazily compiled with both variants (creating cache data before and after execution) with --trace-lazy?

@hashseed
Copy link
Member

Also, I don't think you need NoCacheReason. It's only purpose is to provide data for Chrome's telemetry.

@joyeecheung
Copy link
Member Author

@hashseed I opened #21567 so there is something concrete to discuss

@devsnek
Copy link
Member

devsnek commented Jun 27, 2018

@joyeecheung when the files that node_js2c depends on change the makefile should run it again automatically right?

@joyeecheung
Copy link
Member Author

joyeecheung commented Jun 27, 2018

@devsnek It is supposed to. It doesn't (that's in the TODO list in the OP, feel free to pick it up).

joyeecheung added a commit that referenced this issue Jul 27, 2018
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>
targos pushed a commit that referenced this issue Jul 31, 2018
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>
@joyeecheung
Copy link
Member Author

joyeecheung commented Aug 6, 2018

A few notes on better hash checks of the code cache: in theory it should be possible to make sure node_javascript.cc and node_code_cache.cc are in sync by performing a static_assert of the hashes in C++ (generated by js2c.py and generate_code_cache.js). But since the two C++ files are generated separately, it's still possible that the generated static_assert are out of sync with the sources no matter where you put them. The only way to make sure they are in sync is to make sure node_javascript.cc and node_code_cache.cc are generated together - either in one script or in one build step. The best way to do this that I can think of is still to make js2c.py and generate_code_cache.js two binaries instead of two scripts that relie on Python/Node.js, and that requires some refactoring.

joyeecheung added a commit that referenced this issue Oct 30, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung added a commit that referenced this issue Oct 30, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Nov 2, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Nov 2, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 27, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 27, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Nov 28, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this issue Nov 28, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
e7f710c broke the code cache generation since internalBinding
is now passed in through the wrapper and cannot be redeclared.
This patch fixes that.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Nov 29, 2018
- Add the code cache tests to the default test suite, and test
  the bookkeeping when the binary is not built with the code cache.
- Test the code cache generator to make sure we do not accidentally
  break it - until we enable code cache in the CI.

Refs: #21563
PR-URL: #23855
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member

Trott commented Nov 29, 2018

Anything to update here? @joyeecheung It's been quiet for a while but I don't know if that means nothing relevant is happening or if it means it just isn't being updated. :-D

@joyeecheung
Copy link
Member Author

@Trott The changes I've been making to node_native_module.h/node_native_module.cc are all kind of related to this issue..just forgot to add a Refs for them

joyeecheung added a commit that referenced this issue Apr 4, 2019
This allows us to query the categories of modules in C++
so we can implement the code cache generator in C++ that
does not depend on a Node.js binary.

PR-URL: #27046
Refs: #21563
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
BethGriggs pushed a commit that referenced this issue Apr 4, 2019
This allows us to query the categories of modules in C++
so we can implement the code cache generator in C++ that
does not depend on a Node.js binary.

PR-URL: #27046
Refs: #21563
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@joyeecheung
Copy link
Member Author

Fixed in #27161

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

4 participants