[help wanted] deps: update V8 to 13.6 #57753
Conversation
|
Review requested:
|
nodejs-github-bot
added
build
|
@joyeecheung (or maybe someone else from @nodejs/cpp-reviewers) Can you help finalizing 05eab64 ? |
targos
added
the
semver-major
targos
force-pushed
the
v8-136
branch
2 times, most recently
from
e1ed931 to
7774826
Compare
targos
added
the
help wanted
|
I will run a build on illumos/SmartOS. If you fixed the |
I used some brute-force to clear the high-16-bits in the userland address space so v8 can do its thing. It stopped failing where it used to and started failing somewhere else, but still in In non-debug/stock: In debug (using I noticed on #57114 that @bnoordhuis had a suggestion too that might ease the requirement for me to do something drastic (at least in the short term?). If v8 documents "pointer requirements" somewhere, that'd help me a lot. I'm visiting the v8-dev google group now, so they may give me the clues I need. EDIT/UPDATE ==> I didn't look carefully before at @targos 's update ==> I'm encountering the same issue. |
|
#57753 (comment) basically requires a revert of #30909. |
|
/cc @addaleax |
|
Now it's failing embedtest: |
|
@nodejs/platform-ppc @nodejs/platform-s390 V8 gn build fails: https://ci.nodejs.org/job/node-test-commit-v8-linux/6492/nodes=rhel8-ppc64le,v8test=v8test/console |
|
@targos try updating |
|
@richardlau are you able to do that? |
Yes, I'll look at it after the TSC meeting. |
|
illumos (on SmartOS) update: I can build this branch on the hacked-userlimit illumos system. I've yet to try it on the stock-userlimit ( |
Updated in nodejs/build#4066. https://ci.nodejs.org/job/node-test-commit-v8-linux/6493/ ✅ is with the updated |
|
I suspect that now the use of |
|
I checked the implementation of DisallowJavascriptExecutionScope and it relies on checking a field in the Isolate, so it should not be used during Isolate disposal because otherwise on scope closing it's going to attempt to access a field from the freed isolate. Though we can keep it but only for the IsolateData destruction now diff --git a/src/node_main_instance.cc b/src/node_main_instance.cc
index edbc3c0dc96..2d20b8d3a99 100644
--- a/src/node_main_instance.cc
+++ b/src/node_main_instance.cc
@@ -81,10 +81,10 @@ NodeMainInstance::~NodeMainInstance() {
// This should only be done on a main instance that owns its isolate.
// IsolateData must be freed before UnregisterIsolate() is called.
isolate_data_.reset();
- isolate_->Dispose();
- platform_->UnregisterIsolate(isolate_);
- // TODO(joyeecheung): split Isolate::Free() here?
}
+ // TODO(joyeecheung): split Isolate::Free() here?
+ isolate_->Dispose();
+ platform_->UnregisterIsolate(isolate_);
}
ExitCode NodeMainInstance::Run() { |
As V8 is moving towards built-in CppHeap creation, change the management so that the automatic CppHeap creation on Node.js's end is also enforced at Isolate creation time. 1. If embedder uses NewIsolate(), either they use IsolateSettings::cpp_heap to specify a CppHeap that will be owned by V8, or if it's not configured, Node.js will create a CppHeap that will be owned by V8. 2. If the embedder uses SetIsolateUpForNode(), IsolateSettings::cpp_heap will be ignored (as V8 has deprecated attaching CppHeap post-isolate-creation). The embedders need to ensure that the v8::Isolate has a CppHeap attached while it's still used by Node.js, preferably using v8::CreateParams. See https://issues.chromium.org/issues/42203693 for details. In future version of V8, this CppHeap will be created by V8 if not provided, and we can remove our own "if no CppHeap provided, create one" code in NewIsolate().
|
There's a consistent test failure on macOS that happens only in GHA: |
The original idea was to prevent JS execution in draining platform tasks when shutting down: 5db35b4, via The node/src/node_main_instance.cc Lines 69 to 86 in cb5f671 From the stack trace in #57753 (comment): The line number looks strange to me because |
|
Great news from illumos-land! With these two V8 patches, the first from IBM for generic grow-from-bottom-only that exceeds 47 bits: https://chromium-review.googlesource.com/c/v8/v8/+/6320599/5 and an illumos addition to the end, because we grow from both the TOP of VA space, and the BOTTOM (IOW we use both sides of the 4-level Virtual Address 48-bit space, not just the lower-47) and need to account for that: I'm now getting identical results between a stock illumos with these patches, and an altered illumos without these patches but with all 64-bit user processes being limited to 47-bit Virtual Address space. ADDITIONALLY, not for illumos yet, but it IS available in Oracle Solaris, there's a link-time trick to reserve address space away from anonymous mmap(), and only available to explicit mmap(). We are considering this in illumos, especially when the time comes for 5-level paging with 57-bit Virtual Address space. Those identical results are still: |
@joyeecheung It happens while compiling for the host (we see Edit: full compiler command is |
|
There's another issue with macos: https://ci.nodejs.org/job/node-test-commit-osx/64642/nodes=osx13-x64/testReport/junit/(root)/parallel/test_worker_memory/ |
|
a213815 compiled successfully. I'll try to open a V8 CL Edit: https://chromium-review.googlesource.com/c/v8/v8/+/6469692 |
|
We can see on https://ci.nodejs.org/job/node-test-binary-armv7l/16772/ that the brotli issue is still here. |
|
I see that the following test became flaky: tools/test.py test/parallel/test-fs-stat-bigint.js --repeat=100 |
|
I'm curious about how to proceed with the VA48/full-4-level fixes for illumos? I could probably shrink it down to illumos-only, but that might not help AIX with BIG VA spaces, never mind any other 64-bit architectures that might grow into the top-16-bits of VA space that might kneecap V8. (There STILL may be some V8 abuses of the top-16 I've missed, but the ones I have fixes for are immediately helpful to your overarching import-V8 changes here.) Also, if/when illumos brings in something akin to Oracle Solaris's mapfile (i.e. |
|
@danmcd If https://chromium-review.googlesource.com/c/v8/v8/+/6320599 is accepted by the V8 team, the best would be to submit a follow-up patch to them for illumos |
|
In the mean time, would it be possible to apply the workaround you found to our CI hosts? How can we do it? |
|
For this armv7l error: https://ci.nodejs.org/job/node-test-commit-arm/58141/nodes=ubuntu2204-armv7l/ Reproduction on the CI host is: |
|
This can be "fixed" by reverting https://chromium-review.googlesource.com/c/v8/v8/+/6297948. Should we do that and come back to it for the 13.7 update? Note that the flag no longer exists in V8 13.7 so we won't be able to disable it anymore. |
|
/cc @omerktz FYI. |
|
I can reproduce locally, but it's rare (2% failures) By looking at the code in https://github.com/nodejs/node/blob/main/src/node_platform.cc, it seems possible to have |
|
@joyeecheung is it related to this? |
I'll see if it's as simple as scribbling a value into the kernel tunable. I'll ping here when I've done it so a build can spin. You'll have to use the MNX-hosted agents, however. Unfortunately, this isn't something you can just tell Node users to Just Apply. I don't know how much pressure we can put on V8. There's also (with illumos) a fact that if you're illumos is past April, 2022, you don't need the |
I'd have to revisit the whole of how the current MNX agents work. That work was done by Brie Bennett, who is no longer associated with Triton, SmartOS, or illumos.
I've followed up on the IBM-contributed Gerrit CR. We'll see if they pay attention or not. I'm happy to supply something for Node directly that can be backed-out once V8 gets their act together. |
I hope it can replace #57114
Notable changes:
@nodejs/v8-update @nodejs/tsc