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.

Sign up for GitHub

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

Jump to bottom

deps: update V8 to 13.0 #55014

Draft
wants to merge 16 commits into
base: main
Choose a base branch
from
Draft

deps: update V8 to 13.0 #55014

wants to merge 16 commits into from

Conversation

targos
Copy link
Member

@targos targos commented Sep 19, 2024

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 19, 2024
@nodejs-github-bot
Copy link
Collaborator

Review requested:

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

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Sep 19, 2024
@RedYetiDev
Copy link
Member

@nodejs/tsc per nodejs/Release#1034

@RedYetiDev RedYetiDev added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. labels Sep 19, 2024
ronag
ronag reviewed Sep 19, 2024
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@targos
Copy link
Member Author

targos commented Sep 19, 2024

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?

@targos targos mentioned this pull request Sep 19, 2024
Closed
@ronag
Copy link
Member

ronag commented Sep 19, 2024

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

@targos
Copy link
Member Author

targos commented Sep 19, 2024

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

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

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@codecov Codecov
Copy link

codecov bot commented Sep 19, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (d24c731) to head (141296a).
Report is 123 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55014      +/-   ##
==========================================
- Coverage   88.39%   87.86%   -0.54%     
==========================================
  Files         652      652              
  Lines      186568   186568              
  Branches    36047    35744     -303     
==========================================
- Hits       164924   163925     -999     
- Misses      14915    15870     +955     
- Partials     6729     6773      +44

see 97 files with indirect coverage changes

@targos
Copy link
Member Author

targos commented Sep 20, 2024

@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]

@targos
Copy link
Member Author

targos commented Sep 20, 2024

@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

@ronag
Copy link
Member

ronag commented Sep 20, 2024 •
edited
Loading

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.

mcollina
mcollina approved these changes Sep 20, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@targos
Copy link
Member Author

targos commented Sep 20, 2024

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

@nodejs-github-bot
Copy link
Collaborator

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

@bnoordhuis
Copy link
Member

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.

@targos
Copy link
Member Author

targos commented Sep 20, 2024

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

@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

targos and others added 16 commits October 2, 2024 17:38
@targos
deps: update V8 to 13.0.245.13
1ad85a2
@targos
build: reset embedder string to "-node.0"
9f5ea88
@targos
src: update NODE_MODULE_VERSION to 132
4cb7fc1 
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 13.0.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
@targos
deps: always define V8_EXPORT_PRIVATE as no-op
98ede7c 
dllexport introduces issues when compiling with MSVC.

PR-URL: nodejs#47251
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos
deps: disable V8 concurrent sparkplug compilation
11b0332 
It introduces process hangs on some platforms because Node.js doesn't
tear down V8 correctly.
Disable it while we work on a solution.

Refs: nodejs#47297
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=13902
PR-URL: nodejs#47450
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos
deps: patch V8 to avoid duplicated zlib symbol
a0a9f69 
PR-URL: nodejs#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>
@StefanStojanovic @targos
deps: patch V8 to support compilation with MSVC
c6a5957 
Co-Authored-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#54536
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@StefanStojanovic @targos
deps: fix FP16 bitcasts.h
e9c112f 
PR-URL: nodejs#53134
Refs: nodejs#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>
@targos
deps: always define V8_NODISCARD as no-op
9cbd677 
It's causing compiler errors with some classes on Xcode 11
and the attribute should have no runtime effect.

PR-URL: nodejs#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>
@targos
deps: patch V8 to support older Clang versions
e68b2f6 
PR-URL: nodejs#54536
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@targos
deps: V8: cherry-pick 0c11feeeca4a
fa7c0ee 
Original commit message:

    [turboshaft][tsa] specify namespace for Block

    It is ambiguous otherwise. There is `v8::internal::Block` and
    `v8::internal::compiler::turboshaft::Block`.
    This change is also consistent with the other types used in the macro.

    Change-Id: Ica7e5a09de955d8f38756fe26ab5f7e93e7e16e2
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5878257
    Reviewed-by: Camillo Bruni <cbruni@chromium.org>
    Commit-Queue: Michaël Zasso <mic.besace@gmail.com>
    Reviewed-by: Igor Sheludko <ishell@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#96278}

Refs: v8/v8@0c11fee
@targos
tools: update V8 gypfiles for 13.0
ad7e6fa 
Refs: v8/v8@568f50d
Refs: v8/v8@6437539
@targos
build: remove support for ppc 32-bit
b38b960 
V8 removed support for it.

Refs: v8/v8@6437539
@targos
src: update GetForegroundTaskRunner override
a2ca5ff 
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5758110
@targos
build: link V8 with atomic library
827685d 
Closes: nodejs/node-v8#290
@targos
test: disable fast API call count checks
141296a 
It seems that the arguments are no longer of type `FastOneByteString`.
@targos
Copy link
Member Author

targos commented Oct 2, 2024

Updated e68b2f6 to fix the typename error.

@targos
Copy link
Member Author

targos commented Oct 2, 2024

https://ci.nodejs.org/job/node-test-commit-osx-arm/16970/

@targos targos marked this pull request as draft October 2, 2024 16:04
@targos
Copy link
Member Author

targos commented Oct 2, 2024

This PR requires newer macOS versions in CI.

@richardlau richardlau mentioned this pull request Oct 2, 2024
Closed
2 tasks
@richardlau
Copy link
Member

This PR requires newer macOS versions in CI.

Is this the only blocker for this PR?

@targos
Copy link
Member Author

targos commented Oct 9, 2024

As far as I know, yes

@RedYetiDev RedYetiDev added the blocked PRs that are blocked by other issues or PRs. label Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag left review comments

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. build Issues and PRs related to build files or the CI. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dependencies Pull requests that update a dependency file. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@targos @nodejs-github-bot @RedYetiDev @ronag @bnoordhuis @richardlau @mcollina @StefanStojanovic