[help wanted] deps: update V8 to 13.6 #57753
Open
+404,351
−218,977
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
6 tasks
|
#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. |
8 tasks
|
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 |
The API was removed from V8.
This reverts commit 6857dbc.
New V8 version includes more information about regular expressions.
We depend on V8's version of simdutf now.
The location of some third-party code has changed.
`Isolate::AdjustAmountOfExternalAllocatedMemory` is deprecated. Refs: v8/v8@7dc4c18
The `--expose_externalize_string` flag adds a new global.
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().
Original commit message:
[api] add Isolate::Deinitialize() and Isolate::Free()
This allows embedders to mirror the isolate disposal routine
with an initialization routine that uses Isolate::Allocate().
```
v8::Isolate* isolate = v8::Isolate::Allocate();
// Use the isolate address as a key.
v8::Isolate::Initialize(isolate, params);
isolate->Deinitialize();
// Remove the entry keyed by isolate address.
v8::Isolate::Free(isolate);
```
Previously, the only way to dispose the isolate bundles the
de-initialization and the freeing of the address together in
v8::Isolate::Dispose(). This is inadequate for embedders like
Node.js that uses the isolate address as a key to manage the
task runner associated with it, if another thread gets an
isolate allocated at the aligned address before the other
thread finishes cleanup for the isolate previously allocated
at the same address, and locking on the entire disposal can
be too risky since it may post GC tasks that in turn requires
using the isolate address to locate the task runner. It's a
lot simpler to handle the issue if the disposal process of
the isolate can mirror the initialization of it and split
into two routines.
Refs: nodejs#57753 (comment)
Refs: nodejs#30850
Bug: 412943769
Change-Id: I3865c27395aded3a6f32de74d96d0698b2d891b9
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6480071
Reviewed-by: Leszek Swirski <leszeks@chromium.org>
Commit-Queue: Joyee Cheung <joyee@igalia.com>
Cr-Commit-Position: refs/heads/main@{#99890}
Refs: v8/v8@954187b
The order of these calls is important. When the Isolate is disposed, it may still post tasks to the platform, so it must still be registered for the task runner to be found from the map. After the isolate is torn down, we need to remove it from the map before we can free the address, so that when another Isolate::Allocate() is called, that would not be allocated to the same address and be registered on an existing map entry.
|
rebased to address the conflict with simdutf update (in this branch it's still skipping Also changed the ABI version to 137 since electron is taking 136 #57979 |
|
@danmcd It seems the illumos should be upstreamable. Do you want to upload it to V8? In general it's preferred to get the patch reviewed and landed in the upstream before we float it to Node.js's fork, instead of having to maintain it in the fork again during the next V8 upgrades. |
I would like to get it into V8, BUT V8 also has the related IBM AIX change under review which slightly complicates things. I do have a version of what you accepted as a post-IBM-AIX followup so V8 folks can take THAT after the IBM one (it's been locally tested with node too). I'll gladly accept any assistance from those with better knowledge of the V8 process. |
|
Observations about the Windows crashes:
It looks like either an upstream bug, or we happen to run in a configuration not supported by V8's GC now. I'll open an upstream issue |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I hope it can replace #57114
Notable changes:
@nodejs/v8-update @nodejs/tsc