Skip to content

deps: update V8 to 13.8 #58491

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
Closed
targos wants to merge 12 commits into from
Closed

deps: update V8 to 13.8 #58491

targos wants to merge 12 commits into from

Conversation

@targos
Copy link
Member

@targos targos commented May 28, 2025 •
edited
Loading

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 28, 2025

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 May 28, 2025
@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 28, 2025
@targos targos force-pushed the v8-138 branch 2 times, most recently from aeefb42 to f0e6449 Compare May 28, 2025 12:56
@targos
Copy link
Member Author

targos commented May 28, 2025 •
edited
Loading

I removed the reverts. It's not sustainable to keep them. Let's wait for nodejs/build#4083 to be done.

@targos targos force-pushed the v8-138 branch 2 times, most recently from d5baa5d to eeee442 Compare May 30, 2025 06:32
@targos targos added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 30, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 30, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 30, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 30, 2025

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 30, 2025

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

@targos
Copy link
Member Author

targos commented May 31, 2025

@danmcd the illumos patch doesn't apply anymore so I had to skip it, but SmartOS still fails to build: https://ci.nodejs.org/job/node-test-commit-smartos/60903/nodes=smartos23-x64/

@targos
Copy link
Member Author

targos commented May 31, 2025

@nodejs/platform-aix PTAL at https://ci.nodejs.org/job/node-test-commit-aix/57505/nodes=aix72-ppc64/console

18:58:48 ../deps/v8/src/objects/feedback-vector.cc: In member function 'void v8::internal::FeedbackVector::ClearOptimizedCode()':
18:58:48 ../deps/v8/src/objects/feedback-vector.cc:441:41: error: 'GetIsolate' was not declared in this scope; did you mean 'Isolate'?
18:58:48   441 |   set_maybe_optimized_code(ClearedValue(GetIsolate()));
18:58:48       |                                         ^~~~~~~~~~
18:58:48       |                                         Isolate
18:58:48 gmake[2]: *** [tools/v8_gypfiles/v8_base_without_compiler.target.mk:1140: /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/objects/feedback-vector.o] Error 1

@danmcd
Copy link
Contributor

danmcd commented May 31, 2025

@danmcd the illumos patch doesn't apply anymore so I had to skip it, but SmartOS still fails to build: https://ci.nodejs.org/job/node-test-commit-smartos/60903/nodes=smartos23-x64/

Yeah... this is because of the AIX patch to V8 that's now in. (I referred to this earlier). I'll have an updated one that can drop here very soon, based on this v8-138 branch. Because of Google's overhead for direct contributions to V8, and the centrifuge of mergers-and-acquisitions my $DAYJOB has been through, I cannot engage them directly just yet.

Also, what's the status on #58237 ? I want THAT also landing upstream in V8 too later, but it really helps folks running modern illumos (most everyone these days save a few Triton Data Center VMs/components, which is why Brie set up the Jenkins agents for you the way she did).

@danmcd
Copy link
Contributor

danmcd commented May 31, 2025 •
edited
Loading

@targos -> This commit is supposed to re-fix the VA48 in V8 post-IBM-fix:

danmcd@4370850

but I'm getting coredumps from node_mksnapshot like I did last time around and I'm wondering if V8 is doing MORE stupid pointer tricks assuming low-47-bit-only VA beyond the ones we guarded against in V8 13.6. OR this patch is stale given some things I did for the actual pull-in-13.6 fix.

@danmcd
Copy link
Contributor

danmcd commented May 31, 2025

Yeah my first post-IBM patch was broken badly. This one:

danmcd@4370850

has both compiled and shown only this for gmake test:


[07:16|% 100|+ 4430|-   1]: Done                                              

Failed tests:
out/Release/node --test-reporter=./test/common/test-error-reporter.js --test-reporter-destination=stdout /home/danmcd/node/test/parallel/test-child-process-stdio-reuse-readable-stdio.js
gmake[1]: *** [Makefile:315: jstest] Error 1
gmake: *** [Makefile:342: test] Error 2

Which resembles the last my-up-to-date-illumos-environment.

@abmusse
Copy link
Contributor

abmusse commented Jun 2, 2025

@nodejs/platform-aix PTAL at https://ci.nodejs.org/job/node-test-commit-aix/57505/nodes=aix72-ppc64/console

18:58:48 ../deps/v8/src/objects/feedback-vector.cc: In member function 'void v8::internal::FeedbackVector::ClearOptimizedCode()':
18:58:48 ../deps/v8/src/objects/feedback-vector.cc:441:41: error: 'GetIsolate' was not declared in this scope; did you mean 'Isolate'?
18:58:48   441 |   set_maybe_optimized_code(ClearedValue(GetIsolate()));
18:58:48       |                                         ^~~~~~~~~~
18:58:48       |                                         Isolate
18:58:48 gmake[2]: *** [tools/v8_gypfiles/v8_base_without_compiler.target.mk:1140: /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/objects/feedback-vector.o] Error 1

@targos

Looks like 13.8 includes the leaptiering fix for AIX / IBM i.

Can we try to remove the changes we added here (this change disabled leaptiering for AIX and IBM i). Then rebuild to see if that resolves the error above?

@danmcd
Copy link
Contributor

danmcd commented Jun 2, 2025

@nodejs/platform-aix PTAL at https://ci.nodejs.org/job/node-test-commit-aix/57505/nodes=aix72-ppc64/console

18:58:48 ../deps/v8/src/objects/feedback-vector.cc: In member function 'void v8::internal::FeedbackVector::ClearOptimizedCode()':
18:58:48 ../deps/v8/src/objects/feedback-vector.cc:441:41: error: 'GetIsolate' was not declared in this scope; did you mean 'Isolate'?
18:58:48   441 |   set_maybe_optimized_code(ClearedValue(GetIsolate()));
18:58:48       |                                         ^~~~~~~~~~
18:58:48       |                                         Isolate
18:58:48 gmake[2]: *** [tools/v8_gypfiles/v8_base_without_compiler.target.mk:1140: /home/iojs/build/workspace/node-test-commit-aix/nodes/aix72-ppc64/out/Release/obj.target/v8_base_without_compiler/deps/v8/src/objects/feedback-vector.o] Error 1

@targos

Looks like 13.8 includes the leaptiering fix for AIX / IBM i.

Can we try to remove the changes we added here (this change disabled leaptiering for AIX and IBM i). Then rebuild to see if that resolves the error above?

IF AIX leaptier gets removed, the old illumos VA48 fix needs to return. I've not had cycles or corporate clearance to put that into V8 directly.

@abmusse
Copy link
Contributor

abmusse commented Jun 2, 2025

@danmcd

My suggestion above was to just turn off the switch that disables building with leaptiering (added back in 13.4). Now that the leaptiering changes are upstreamed and included in 13.8 we can turn turn that switch off. AIX leaptiering changes will remain the same and not be removed.

@ryanaslett
Copy link

ryanaslett commented Jun 2, 2025

I removed the reverts. It's not sustainable to keep them. Let's wait for nodejs/build#4083 to be done.

How urgent is the above issue?

@danmcd
Copy link
Contributor

danmcd commented Jun 2, 2025

@danmcd

My suggestion above was to just turn off the switch that disables building with leaptiering (added back in 13.4). Now that the leaptiering changes are upstreamed and included in 13.8 we can turn turn that switch off. AIX leaptiering changes will remain the same and not be removed.

OH MY GOODNESS, I'm so sorry for not reading that clearly. My new post-IBM-leaptiering illumos VA48 commit stands, then, and it'd be great to have it here (and in V8, but that's not a here problem).

@abmusse
Copy link
Contributor

abmusse commented Jun 9, 2025

@targos

I created a branch to test enabling leaptiering with V8 13.8 for AIX: https://github.com/abmusse/node/tree/v8-138-aix

Ran a test build and things are building again: https://ci.nodejs.org/job/node-test-commit-aix/nodes=aix72-ppc64/57652/console 🎉

targos added 3 commits June 11, 2025 11:11
@targos
deps: update V8 to 13.8.258.14
013370e
@targos
build: reset embedder string to "-node.0"
072d27f
@targos
src: update NODE_MODULE_VERSION to 139
649f52c 
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 13.8.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
targos and others added 5 commits June 11, 2025 11:12
@targos
deps: patch V8 to avoid duplicated zlib symbol
6675e95 
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>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@StefanStojanovic @targos
deps: fix FP16 bitcasts.h
6f8ac5c 
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>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@StefanStojanovic @targos
deps: define V8_PRESERVE_MOST as no-op on Windows
e94a210 
It's causing linker errors with node.lib in node-gyp and potentially
breaks other 3rd party tools

PR-URL: nodejs#56238
Refs: nodejs#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>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@targos
deps: remove problematic comment from v8-internal
9a1075e 
GCC emits warnings because of the trailing backslashes.

PR-URL: nodejs#58070
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@joyeecheung @targos
deps: use std::map in MSVC STL for EphemeronRememberedSet
51ab753 
PR-URL: nodejs#58070
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
@targos
Copy link
Member Author

targos commented Jun 11, 2025

Thanks @abmusse, @danmcd. I included both of your patches.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 11, 2025

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

@danmcd
Copy link
Contributor

danmcd commented Jun 11, 2025 •
edited
Loading

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

The illumos patch 96b370f is not the correct one to apply after the IBM commit.

(EDIT/AMEND): Uggh, let's try this again. See here: https://github.com/danmcd/node/commits/v8-138/

There's a revert-commit of the bad one, and a new-commit of the good one.

danmcd and others added 4 commits June 12, 2025 08:39
@danmcd @targos
deps: patch V8 for illumos
9f7c68d 
illumos pointers are VA48, can allocate from the top of the 64-bit range
as well.
@targos
deps: V8: cherry-pick 249de887a8d3
3e5e552 
Original commit message:

    [explicit-resource-management] Fix parsing for (using of=null;;)

    Apparently `using of` is allowed in the initializer position of C-style
    for loops.

    See tc39/proposal-explicit-resource-management#248

    Bug: 42203506
    Change-Id: Ia056b161f4ea28a0f3ba4e3e420f1718195274a4
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6594471
    Commit-Queue: Shu-yu Guo <syg@chromium.org>
    Reviewed-by: Rezvan Mahdavi Hezaveh <rezvan@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#100531}

Refs: v8/v8@249de88
PR-URL: nodejs#58561
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@abmusse @targos
tools: enable leaptiering for aix/ibmi
299c19d
@targos
tools: update V8 gypfiles for 13.8
7b3b542
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 12, 2025

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

@mcollina
Copy link
Member

mcollina commented Aug 1, 2025

@targos any updates on this?

@targos
Copy link
Member Author

targos commented Aug 1, 2025

It's still blocked on macOS (new version is needed in Jenkins CI)

@YurySolovyov
Copy link

YurySolovyov commented Aug 4, 2025

Could be release notes worthy: https://v8.dev/blog/json-stringify

@legendecas legendecas mentioned this pull request Aug 5, 2025
Closed
@Dhruv-Garg79
Copy link

Dhruv-Garg79 commented Aug 29, 2025

Will this be part of v24?

@mcollina
Copy link
Member

mcollina commented Aug 29, 2025

Will this be part of v24?

I think it's unlikely.

@targos targos mentioned this pull request Sep 8, 2025
Closed
@Rush
Copy link

Rush commented Sep 10, 2025

Will this be part of v24?

I think it's unlikely.

Pretty please? :-) v24 is not yet LTS - last chance. That JSON.stringify perf bump looks very very promising.

@styfle
Copy link
Member

styfle commented Sep 17, 2025

Should we just put all the effort into upgrading to 14.1?

Or is it still valuable to continue trying to upgrade to 13.8 first?

@targos targos closed this Sep 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

build Issues and PRs related to build files or the CI. 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.