deps: update V8 to 13.0 by targos · Pull Request #55014 · nodejs/node · GitHub

Notable changes:

WebAssembly JS String Builtins

Review requested:

@nodejs/gyp
@nodejs/security-wg
@nodejs/v8-update

Now I think we have disposable and Co? Maybe notable?

I get the notable changes from https://chromestatus.com/roadmap.
There's nothing about disposable there (and using still triggers a syntax error), why do you expect it to be available?

It's in the v8 code. Maybe not enabled still?

Probably. The tracking issue is still open: https://issues.chromium.org/issues/42203506

CI: https://ci.nodejs.org/job/node-test-pull-request/62572/

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6203/

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6203/

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6203/

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.69%. Comparing base (304bb9c) to head (63ee9f0).
Report is 22 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55014      +/-   ##
==========================================
- Coverage   89.20%   88.69%   -0.52%     
==========================================
  Files         663      663              
  Lines      192012   192012              
  Branches    36929    36615     -314     
==========================================
- Hits       171286   170306     -980     
- Misses      13582    14530     +948     
- Partials     7144     7176      +32

see 94 files with indirect coverage changes

@nodejs/platform-windows MSVC complains:

23:17:25 C:\workspace\node-compile-windows-debug\node\deps\v8\src\interpreter\interpreter-generator-tsa.cc(333,1): error C2872: 'Block': ambiguous symbol [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_initializers.vcxproj]
23:17:25 C:\workspace\node-compile-windows-debug\node\deps\v8\src\ast\ast.h(318,7): message : could be 'v8::internal::Block' [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_initializers.vcxproj]
23:17:25 C:\workspace\node-compile-windows-debug\node\deps\v8\src\compiler\turboshaft\graph.h(306,7): message : or       'v8::internal::compiler::turboshaft::Block' [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_initializers.vcxproj]
23:17:25 C:\workspace\node-compile-windows-debug\node\deps\v8\src\interpreter\interpreter-generator-tsa.cc(333,1): error C2440: 'initializing': cannot convert from 'v8::internal::compiler::turboshaft::Block *' to 'v8::internal::Block *' [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_initializers.vcxproj]
23:17:25 C:\workspace\node-compile-windows-debug\node\deps\v8\src\interpreter\interpreter-generator-tsa.cc(333,1): message : Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-style cast [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_initializers.vcxproj]
23:17:25 C:\workspace\node-compile-windows-debug\node\deps\v8\src\interpreter\interpreter-generator-tsa.cc(333,1): error C2665: 'v8::internal::compiler::turboshaft::CatchScopeImpl<v8::internal::compiler::turboshaft::Assembler<v8::internal::compiler::turboshaft::reducer_list<v8::internal::compiler::turboshaft::TurboshaftAssemblerOpInterface,Reducer,v8::internal::interpreter::BytecodeHandlerReducer,v8::internal::BuiltinsReducer,v8::internal::FeedbackCollectorReducer,v8::internal::compiler::turboshaft::MachineLoweringReducer,v8::internal::compiler::turboshaft::VariableReducer,v8::internal::compiler::turboshaft::TSReducerBase>>>::CatchScopeImpl': no overloaded function could convert all the argument types [C:\workspace\node-compile-windows-debug\node\tools\v8_gypfiles\v8_initializers.vcxproj]

@ronag @anonrig
It seems that something changed with the fast API which results in fast C++ functions not being called when they expect FastOneByteString arguments (I verified the tests with --trace-opt --trace-deopt. The JS functions are still optimized).
I don't know what to do here other than 112b303

So basically Fast API is "broken"? This will cause some massive perf regressions (maybe good idea to kick of a benchmark CI to confirm). Can we involve someone with V8 knowledge? If benchmarks do not show regressions then I agree that we just apply 112b303 as a workaround.

lgtm

Either fast API is "broken", or the strings used in the test are no longer of the kind "fast one-byte".

CI: https://ci.nodejs.org/job/node-test-pull-request/62584/

So basically Fast API is "broken"?

No, the test is. At least two of the four calls pass two-byte strings.

I believe v8/v8@a1ada77 is the responsible change upstream.

I'm trying to upstream a fix for the MSVC error: https://chromium-review.googlesource.com/c/v8/v8/+/5878257

PR-URL: #53134 Refs: #52809 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: #55014 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

It's causing compiler errors with some classes on Xcode 11 and the attribute should have no runtime effect. PR-URL: #54077 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> PR-URL: #55014 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

It's causing linker errors with node.lib in node-gyp and potentially breaks other 3rd party tools PR-URL: #56238 Refs: #55784 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: #55014 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

PR-URL: nodejs/node#53134 Refs: nodejs/node#52809 Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs/node#55014 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

It's causing compiler errors with some classes on Xcode 11 and the attribute should have no runtime effect. PR-URL: nodejs/node#54077 Reviewed-By: Jiawen Geng <technicalcute@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> PR-URL: nodejs/node#55014 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

It's causing linker errors with node.lib in node-gyp and potentially breaks other 3rd party tools PR-URL: nodejs/node#56238 Refs: nodejs/node#55784 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> PR-URL: nodejs/node#55014 Reviewed-By: Matteo Collina <matteo.collina@gmail.com>