node
node copied to clipboard
nodejs
deps: update V8 to 13.8
Review requested:
- [ ] @nodejs/gyp
- [ ] @nodejs/security-wg
- [ ] @nodejs/v8-update
I removed the reverts. It's not sustainable to keep them. Let's wait for https://github.com/nodejs/build/issues/4083 to be done.
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/6587/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/6587/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/6587/
@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/
@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 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).
@targos -> This commit is supposed to re-fix the VA48 in V8 post-IBM-fix:
https://github.com/danmcd/node/commit/4370850da862e12f5cab13d4f5522305966e91af
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.
Yeah my first post-IBM patch was broken badly. This one:
https://github.com/danmcd/node/commit/4370850da862e12f5cab13d4f5522305966e91af
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.
@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?
@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.
@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.
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
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).
@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 🎉
CI: https://ci.nodejs.org/job/node-test-pull-request/67403/
The illumos patch https://github.com/nodejs/node/pull/58491/commits/96b370f5049014f42752e064b44dafdfda24976f 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.
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.
Should we just put all the effort into upgrading to 14.1?
- https://github.com/nodejs/node/pull/59805
Or is it still valuable to continue trying to upgrade to 13.8 first?
