deps: V8: cherry-pick cb00db4dba6c

[RaisinTen]

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ä <marja@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/main@{#87944}

Refs: v8/v8@cb00db4
Refs: #48191 (comment)

[RaisinTen]

@kvakil test/parallel/test-node-output-errors.mjs was failing, so I had to update these to pass the test. Could you please confirm if this is expected behavior?

[kvakil]

I don't know, sorry.

[kvakil]

When I tried this earlier I saw that it would increase the size of the code cache / binary by quite a bit. I'm not sure what our thinking around binary size is.

I guess if binary size is important to us, we could land it, and then find the modules with the biggest code caches and switch them to non-eager compiles?

[kvakil]

I don't know, sorry.

[kvakil]

Also, did this produce an improvement when you tried it for the SEA bytecode caching?

[RaisinTen]

@kvakil

When I tried this earlier I saw that it would increase the size of the code cache / binary by quite a bit.

Indeed, when I try to generate the SEA prep blob with the yarn cjs bundle as input (2.6M), the blob generated without this fix is 3.0M but the one with this fix is 9.1M.

I'm not sure what our thinking around binary size is.

Since the default Node.js binary itself is already big (92M on macOS), I'm not really sure if users would really mind it. I guess we could depend on some user feedback to understand if this needs to be fixed?

I guess if binary size is important to us, we could land it, and then find the modules with the biggest code caches and switch them to non-eager compiles?

Not sure I fully understand, are you referring to the modules in the input code? Currently, the requirement is that the input code needs to be bundled into a single module before generating the SEA prep blob, so when the code that generates the code cache runs, there would only be a single input module it can compile eagerly / non-eagerly.

Also, did this produce an improvement when you tried it for the SEA bytecode caching?

Without this fix, the improvement on #48191 was 24% and with this fix, it changed to 33%.

[targos]

Note that since this fix landed upstream, it will be applied to Node.js automatically when V8 gets upgraded.

[RaisinTen]

I have no idea why the coverage runs are consistently failing for this change :/

Seems to be marking large chunks of code uncovered in the report which sounds like false data:

-----------------------------------|---------|----------|---------|---------|-----------------------
File                               | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s     
-----------------------------------|---------|----------|---------|---------|-----------------------
All files                          |   69.61 |    71.53 |   62.39 |   69.61 |                       
 lib                               |   68.79 |    64.79 |   64.25 |   68.79 |                       
  _http_agent.js                   |   85.76 |    59.25 |     100 |   85.76 | ...78-479,499-500,539 
...

Lines 78 to 479 in lib/_http_agent.js are covered by our tests because these are colored green in https://app.codecov.io/gh/nodejs/node/blob/main/lib%2F_http_agent.js#L78.

Could this be a regression?

[joyeecheung]

Since the default Node.js binary itself is already big (92M on macOS), I'm not really sure if users would really mind it. I guess we could depend on some user feedback to understand if this needs to be fixed?

I looked into the binary size some time ago, and about 46% of the size comes from the ICU data (it was smaller when we only had small-ICU, and we doubled the size of the binary after we enabled full-ICU by default). The rest of the increase of the binary size over the years mostly come from the bloating of the core, especially the increased amount of JavaScript sources (though the native side also bloated to a lesser extent), which is one of my motivation to implement #46997 so that we can explore better compression options (zlib is too slow). I am working on a prototype that enables compression of the RO data (JS source / code cache / snapshot etc.) with lz4/ztsd/others and investigate which options are better/whether it's good enough to be enabled by default. Although in general I would say the increase from the code cache seems trivial compared to the size of the ICU data...

[richardlau]

I have no idea why the coverage runs are consistently failing for this change :/

According the the GH logs (which you have to download because they're truncated in the web UI), there are test failures

2023-07-03T10:02:34.7337054Z ===
2023-07-03T10:02:34.7337116Z === 21 tests failed
2023-07-03T10:02:34.7337188Z === 1 tests CRASHED
2023-07-03T10:02:34.7337246Z ===
2023-07-03T10:02:34.7337253Z 
2023-07-03T10:02:34.7337324Z Failed tests:
2023-07-03T10:02:34.7337664Z out/Release/node --expose-internals /home/runner/work/node/node/test/parallel/test-bootstrap-modules.js
2023-07-03T10:02:34.7337921Z out/Release/node /home/runner/work/node/node/test/parallel/test-debugger-profile.js
2023-07-03T10:02:34.7338211Z out/Release/node /home/runner/work/node/node/test/parallel/test-inspector-wait-for-connection.js
2023-07-03T10:02:34.7338643Z out/Release/node --experimental-shadow-realm --max-old-space-size=20 /home/runner/work/node/node/test/parallel/test-shadow-realm-gc.js
2023-07-03T10:02:34.7338917Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-api.js
2023-07-03T10:02:34.7339169Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-argv1.js
2023-07-03T10:02:34.7339417Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-console.js
2023-07-03T10:02:34.7339665Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-cjs-main.js
2023-07-03T10:02:34.7339980Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-lookup-localhost-promise.js
2023-07-03T10:02:34.7340269Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-lookup-localhost.js
2023-07-03T10:02:34.7340585Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-resolve-localhost-promise.js
2023-07-03T10:02:34.7340877Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-dns-resolve-localhost.js
2023-07-03T10:02:34.7341117Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-eval.js
2023-07-03T10:02:34.7341355Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-gzip.js
2023-07-03T10:02:34.7341661Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-namespaced-builtin.js
2023-07-03T10:02:34.7341928Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-incompatible.js
2023-07-03T10:02:34.7342162Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-net.js
2023-07-03T10:02:34.7342416Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-typescript.js
2023-07-03T10:02:34.7342644Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-umd.js
2023-07-03T10:02:34.7342885Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-warning.js
2023-07-03T10:02:34.7343149Z out/Release/node /home/runner/work/node/node/test/parallel/test-snapshot-weak-reference.js

I'm not sure those would explain the quoted drop in coverage.

Could this be a regression?

I'd imagine that is likely. Coverage data is provided by V8 after all.

[RaisinTen]

Posted about the test failures in https://chromium-review.googlesource.com/c/v8/v8/+/4571822/comments/1f25968d_d8669bb9, closing