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

src: use V8-owned CppHeap #53205

Draft
wants to merge 22 commits into
base: canary-base
Choose a base branch
from
Draft

src: use V8-owned CppHeap #53205

wants to merge 22 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented May 29, 2024 •
edited
Loading

This is partly based on #53086 - opening a PR here to test if the CI is happy with it. Still looking into how to make the Isolate disposal happy.

src: use V8-owned CppHeap

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().

Fixes: #52718

targos and others added 16 commits May 28, 2024 10:26
@targos
deps: update V8 to 12.7.137
3566324
@targos
build: reset embedder string to "-node.0"
512a7b9
@targos
src: update NODE_MODULE_VERSION to 128
47be3ba 
Major V8 updates are usually API/ABI incompatible with previous
versions. This commit adapts NODE_MODULE_VERSION for V8 12.7.

Refs: https://github.com/nodejs/CTC/blob/master/meetings/2016-09-28.md
@targos
deps: always define V8_EXPORT_PRIVATE as no-op
223de68 
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: disable V8 concurrent sparkplug compilation
a827b0e 
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
deps: avoid compilation error with ASan
9b83b8e 
Refs: https://bugs.chromium.org/p/v8/issues/detail?id=14221
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos
deps: remove usage of a C++20 feature from V8
3849d83 
We are not ready to compile with C++20 support yet.
This is only a DCHECK that can be removed without affecting the behavior
of release builds.

PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos
deps: patch V8 to avoid duplicated zlib symbol
4da65e9 
PR-URL: nodejs#49639
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
@targos
deps: silence internal V8 deprecation warning
35ebfb1 
PR-URL: nodejs#52293
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Richard Lau <rlau@redhat.com>
@StefanStojanovic @targos
deps: patch V8 to support compilation with MSVC
38a5e0b 
After enabling -std:c++20 on Windows, patch is now much smaller.

PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos
deps: V8: revert CL 5331688
9d544d4 
On Windows debug builds, it is not allowed to dereference empty
iterators.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5331688
PR-URL: nodejs#52465
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos
build: include v8-sandbox.h header in distribution
9d8823a 
Refs: v8/v8@5495678
@fdoray @targos
src: add source location to v8::TaskRunner
8d34ef7 
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5300826
@LeszekSwirski @targos
build: disable ICF for mksnapshot
f553d5d 
Refs: https://chromium-review.googlesource.com/c/v8/v8/+/5447267
Co-authored-by: Michaël Zasso <targos@protonmail.com>
@targos
test: update v8-stats test for V8 12.6
54ad3e4 
Refs: v8/v8@e30e228
@targos
tools: update V8 gypfiles for 12.6
a1c9178 
Refs: v8/v8@f8fa220
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 29, 2024
@joyeecheung joyeecheung mentioned this pull request May 29, 2024
Closed
jasnell
jasnell reviewed May 29, 2024
View reviewed changes
src/api/environment.cc
// Ensure that there is always a CppHeap.
if (settings.cpp_heap == nullptr) {
params->cpp_heap =
CppHeap::Create(platform, CppHeapCreateParams{{}}).release();
Copy link
Member

Choose a reason for hiding this comment

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

FYI... something to investigate... we discovered in workers that v8 might not actually be taking proper ownership of the CppHeap* (we're using v8 12.6 there)... I have a todo to investigate further but we landed a temporary fix to avoid a memory leak... see https://github.com/cloudflare/workerd/pull/2146/files

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the heads up. Yeah from what I can tell V8 is only releasing the CppHeap during disposal and never really deletes it? We should also wait until that's fixed upstream before migrating to V8-owned CppHeap or otherwise it's a leak.

Copy link
Member

Choose a reason for hiding this comment

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

To be honest I really don't understand the logic of how v8 is handling this currently... definitely think it should be investigated and resolved before landing this here.

@nodejs-github-bot
Copy link
Collaborator

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

@joyeecheung
deps: V8: cherry-pick 300451e869ae
cb18042 
Original commit message:

    src: fix v8-sandbox.h multiline comments

    Before it fails to compile with -Werrors=comment because some of
    the `//` comments uses `\` at the end of the line.

    Refs: nodejs#53086

    Change-Id: I10e5b970cff4a8eda78c305f5649d8bf08d7da3c
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/5584738
    Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
    Commit-Queue: Joyee Cheung <joyee@igalia.com>
    Cr-Commit-Position: refs/heads/main@{#94165}

Refs: v8/v8@300451e
@joyeecheung
src: remove dependency on wrapper-descriptor-based CppHeap
6c7c7c5 
As V8 has moved away from wrapper-descriptor-based CppHeap, this
patch:

1. Create the CppHeap without using wrapper descirptors.
2. Deprecates node::SetCppgcReference() in favor of
   v8::Object::Wrap() since the wrapper descriptor is no longer
   relevant. It is still kept as a compatibility layer for addons
   that need to also work on Node.js versions without
   v8::Object::Wrap().
@joyeecheung
src: use V8-owned CppHeap
3df9fa1 
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().
@joyeecheung
src: migrate from deprecated SnapshotCreator constructor
04c21c9
@joyeecheung
deps: add Isolate::Free() and IsolateDisposeFlags::kDontFree
128ffd5
@joyeecheung
src: use Isolate::Free()
4176cef
@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jun 10, 2024
@targos targos force-pushed the canary-base branch 4 times, most recently from 04423fd to 8e3fab5 Compare August 3, 2024 16:14
@targos targos force-pushed the canary-base branch 3 times, most recently from 7177ac5 to 19f043a Compare August 16, 2024 14:14
@targos targos force-pushed the canary-base branch 2 times, most recently from 8d54422 to 290e38c Compare September 8, 2024 07:51
@targos targos force-pushed the canary-base branch 3 times, most recently from 7919d6a to 7b0a20c Compare September 16, 2024 06:09
@targos targos force-pushed the canary-base branch 4 times, most recently from cd3e18c to 6d7aae9 Compare September 25, 2024 20:37
@targos targos force-pushed the canary-base branch 4 times, most recently from eea5f26 to b8bcfb8 Compare October 30, 2024 13:40
@targos targos force-pushed the canary-base branch 3 times, most recently from 4ca1113 to 4d13a5f Compare December 21, 2024 06:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell left review comments

Assignees
No one assigned
Labels
blocked PRs that are blocked by other issues or PRs. c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@joyeecheung @nodejs-github-bot @jasnell @targos @StefanStojanovic @fdoray @LeszekSwirski