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 11.4 #48029

Closed
wants to merge 12 commits into from
Closed

deps: update V8 to 11.4 #48029

wants to merge 12 commits into from

Conversation

targos
Copy link
Member

@targos targos commented May 16, 2023

No description provided.

@targos
deps: update V8 to 11.4.183.8
77ac9bc
@targos
build: reset embedder string to "-node.0"
78e6dbb
@targos
src: update NODE_MODULE_VERSION to 117
25aded8 
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 11.4.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
@targos
deps: always define V8_EXPORT_PRIVATE as no-op
958931c 
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: silence irrelevant V8 warning
7bebfcd 
PR-URL: nodejs#45579
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
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: patch V8 to support compilation on win-arm64
9603655
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @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 16, 2023
targos and others added 6 commits May 16, 2023 12:08
@targos
deps: disable V8 concurrent sparkplug compilation
80942b3 
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
tools: update V8 gypfiles for 11.4
123369b
@targos
tools: add new V8 headers to distribution
c72a91b
@targos
lib: update usage of always on Atomics API
25539ea
@pfaffe @targos
test: adapt debugger tests to V8 11.4
61e265a 
Accept a new `step` break message.
@targos
test: update flag to disable SharedArrayBuffer
174ca27 
`--no-harmony-sharedarraybuffer` was removed from V8 but it's still
possible to disable the feature with `--enable-sharedarraybuffer-per-context`.
@targos targos added the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 16, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@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/5366/

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@gengjiawen
Copy link
Member

I can't believe this, only smartOS failed 😭. Especially all windows version works.

@nodejs-github-bot
Copy link
Collaborator

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

aduh95
aduh95 approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@aduh95 aduh95 left a comment

Choose a reason for hiding this comment

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

25539ea and 174ca27 LGTM
RSLGTM for the rest :)

@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member Author

targos commented May 17, 2023

Unfortunately, I think the SmartOS failure is not a flake :(

12:17:10 not ok 3586 parallel/test-tick-processor-arguments
12:17:10   ---
12:17:10   duration_ms: 300052.04700
12:17:10   severity: fail
12:17:10   exitcode: -15
12:17:10   stack: |-
12:17:10     timeout
12:17:10   ...

@nodejs/platform-smartos

@richardlau
Copy link
Member

Unfortunately, I think the SmartOS failure is not a flake :(

12:17:10 not ok 3586 parallel/test-tick-processor-arguments
12:17:10   ---
12:17:10   duration_ms: 300052.04700
12:17:10   severity: fail
12:17:10   exitcode: -15
12:17:10   stack: |-
12:17:10     timeout
12:17:10   ...

@nodejs/platform-smartos

@bahamat (we should probably add you to that team for notifications for smartos).

@bahamat
Copy link

bahamat commented May 17, 2023

@richardlau Yes, that would be a good idea. I’ll have my team take a look at this.

@richardlau
Copy link
Member

@richardlau Yes, that would be a good idea.

I've sent out an invite for you to join the team.

@bahamat
Copy link

bahamat commented May 17, 2023

I've joined.

@targos
Copy link
Member Author

targos commented May 17, 2023

@StefanStojanovic We still need to apply a small patch for ARM64 Windows: 9603655. Do you know if something can be done about it upstream?

@StefanStojanovic
Copy link
Contributor

@StefanStojanovic We still need to apply a small patch for ARM64 Windows: 9603655. Do you know if something can be done about it upstream?

Yes, that patch is still required. I've already landed 2 CLs upstream in the V8 repo with some changes regarding cross-compilation and MSVC in general. I have one more CL to open there that fixes an issue with the asm(...); usage. Hopefully, by the next time V8 is updated in Node.js that will all be done, and the patch will no longer be needed.

@jperkin
Copy link

jperkin commented May 18, 2023

Hi, I'm looking at the failure on SmartOS - to start with I've tried to reproduce the build, but I see different test failures:

Failed tests:
out/Release/node /root/node/test/parallel/test-debugger-auto-resume.mjs
out/Release/node /root/node/test/parallel/test-http-client-timeout-event.js
out/Release/node /root/node/test/parallel/test-http-client-upload-buf.js
make[1]: *** [Makefile:550: test-ci] Error 1

If I run parallel/test-tick-processor-arguments manually it appears to be fine:

$ ./out/Release/node ./test/parallel/test-tick-processor-arguments.js
$ echo $?
0

Is it just that these tests can occasionally fail, or did I do something wrong in my build?

I'm using cb4fc99, building using gcc10 as required, and using the same image as the failing build (though the platform might be different, it isn't shown in the log).

@jperkin
Copy link

jperkin commented May 18, 2023

I should also have mentioned, re-running the 3 failing tests also made them complete successfully.

@jperkin
Copy link

jperkin commented May 18, 2023

Ok, never mind, looks like I was looking at the wrong Jenkins failure - I'm now retesting against 174ca27.

@jperkin
Copy link

jperkin commented May 22, 2023 •
edited
Loading

Ok, so I think I've found the problem, but I'm not sure what the correct approach is for fixing it.

The V8 update includes numerous changes to heap allocation, and one side effect of this is that addresses in the profiling log are now outside of the safe Integer range. Comparing an isolate log from an earlier version of node:

code-creation,Builtin,3,6183,0x1841000,1632,RecordWrite
code-creation,Builtin,3,6207,0x1841680,424,EphemeronKeyBarrier
code-creation,Builtin,3,6223,0x1841840,44,AdaptorWithBuiltinExitFrame
code-creation,Builtin,3,6235,0x1841880,291,ArgumentsAdaptorTrampoline
code-creation,Builtin,3,6251,0x18419c0,207,CallFunction_ReceiverIsNullOrUndefined

with one from the proposed V8 update:

code-creation,Builtin,2,16468475,0xfffffc7fdda40000,768,DeoptimizationEntry_Eager
code-creation,Builtin,2,16468500,0xfffffc7fdda40340,772,DeoptimizationEntry_Lazy
code-creation,Builtin,2,16468514,0xfffffc7fdda40680,2720,RecordWriteSaveFP
code-creation,Builtin,2,16468522,0xfffffc7fdda41140,1180,RecordWriteIgnoreFP
code-creation,Builtin,2,16468531,0xfffffc7fdda41600,332,EphemeronKeyBarrierSaveFP
code-creation,Builtin,2,16468552,0xfffffc7fdda41780,116,EphemeronKeyBarrierIgnoreFP
code-creation,Builtin,2,16468565,0xfffffc7fdda41800,64,AdaptorWithBuiltinExitFrame
code-creation,Builtin,2,16468574,0xfffffc7fdda41880,256,CallFunction_ReceiverIsNullOrUndefined

What happens with the test case is that node ends up in a loop in findGreatestLessThan from deps/v8/tools/splaytree.mjs. The addresses are parsed by deps/v8/tools/tickprocessor.mjs, but as they use parseInt they are not unique and this ends up in the weeds once it tries to parse the CallFunction_ReceiverIsNullOrUndefined line.

> parseInt(0xfffffc7fdda41800) === parseInt(0xfffffc7fdda41880)
true

The same log hangs older versions of node, as well as node on other platforms.

There appear to be at least 2 approaches to the problem: update the profiling code to handle higher addresses, or revert some of the proposed V8 changes to avoid the higher address ranges. Given that node itself appears to be functioning correctly apart from this single test case it feels like fixing the profiling code would be the better approach.

I'm happy to help test any changes for this. One thing I noticed (and wondered for a while if it was a contributing factor) while investigating is that deps/v8/src/base/platform/platform-solaris.cc is missing an implementation of OS::GetSharedLibraryAddresses() so the isolate logs are missing shared-library entries, and I'll look at implementing this so that profiling output includes the right objects.

@jperkin
Copy link

jperkin commented May 22, 2023

Here's an example log if anyone wants to test against it, it should reliably hang --prof-process.
isolate-0x68d8010-261056-v8.log

@targos
Copy link
Member Author

targos commented May 23, 2023

Thanks @jperkin for the deep analysis. I found out that V8 actually supports parsing numbers as BigInt in some of the files since v8/v8@aee5fb0.
I also think that fixing the profiling code would be the better approach.

@targos targos mentioned this pull request Jun 14, 2023
Closed
@targos
Copy link
Member Author

targos commented Jun 14, 2023

Replaced by #48456

@targos targos closed this Jun 14, 2023
@targos targos deleted the v8-114 branch September 18, 2023 09:05
@targos targos mentioned this pull request Sep 18, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tniessen tniessen tniessen approved these changes

@StefanStojanovic StefanStojanovic StefanStojanovic approved these changes

@aduh95 aduh95 aduh95 approved these changes

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. 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.
10 participants
@targos @nodejs-github-bot @gengjiawen @richardlau @bahamat @StefanStojanovic @jperkin @tniessen @aduh95 @pfaffe