GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — src: use V8-owned CppHeap by joyeecheung · Pull Request #53205 · nodejs/node

Conversation

@joyeecheung

@joyeecheung joyeecheung commented May 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
avivkeller and lin72h reacted with thumbs up emoji
All reactions
  • 👍 2 reactions
@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 involving general changes in the lib/ or src/ directories. needs-ci PRs that need a full CI run. labels May 29, 2024
@joyeecheung joyeecheung mentioned this pull request May 29, 2024
src: remove dependency on wrapper-descriptor-based cpp heap #53086
Closed
@joyeecheung
joyeecheung force-pushed the cppheap-auto-2 branch from 39d25ce to b224a56 Compare May 29, 2024 18:29
jasnell
jasnell reviewed May 29, 2024
View reviewed changes
Comment thread src/api/environment.cc Outdated

@jasnell jasnell May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@joyeecheung joyeecheung May 29, 2024

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell May 29, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented May 29, 2024

Copy link
Copy Markdown
Collaborator

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
joyeecheung force-pushed the cppheap-auto-2 branch from b224a56 to 4176cef Compare May 30, 2024 16:38
@joyeecheung

joyeecheung commented May 30, 2024

Copy link
Copy Markdown
Member Author

After some experiments it seems the option that makes the most sense is what @addaleax proposed in #53086 (comment) so I did a prototype here for Isolate::Free() and IsolateDisposeFlags::kDontFree. Will need to verify the flakes are not coming back, somehow..

Also I noticed that CommonEnvironmentSetup is still using the deprecated SnapshotCreator which owns the Isolate that get's disposed by V8, as a result we need to migrate to the new SnapshotCreator that allows us to own the isolate and do the disposal ourselves...will split that part out as a separate PR (removing the CppHeap bits for main).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nodejs-github-bot

nodejs-github-bot commented Jun 1, 2024

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/59599/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Jun 4, 2024

Copy link
Copy Markdown
Member Author

It seems the original flakes are not coming back, though I am unable to reproduce them in the first place, I wonder if the original reasoning already no longer applies after all these V8 changes over the years..
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@targos
targos force-pushed the canary-base branch from a1c9178 to 30329d0 Compare June 4, 2024 11:22
@joyeecheung

joyeecheung commented Jun 5, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Communicated with mlippautz offline and it seems the owned CppHeap implementation is still experimental and not ready yet (I guess that's why AttachCppHeap/DetachCppHeap are still just DEPRECATED_SOON, not DEPRECATED), we can wait until it matures before migrating. On the other hand, the current platform releasing the task runner while the isolate is still running seems problematic on its own. So we should do something about it at the mean time...(e.g. either just revert the order and see if it flakes again, or upstream the Isolate::Free() patch before reverting the order?)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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 from 30329d0 to cb3ed9c Compare June 14, 2024 07:11
@targos
targos force-pushed the canary-base branch from cb3ed9c to 7f25854 Compare June 30, 2024 09:24
@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 from 19f043a to 8fc9eba Compare August 24, 2024 09:28
@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 from 7b0a20c to 199cba3 Compare September 19, 2024 06:15
@targos
targos force-pushed the canary-base branch from 7e95ee1 to aabb4b6 Compare February 15, 2025 09:55
@targos
targos force-pushed the canary-base branch 2 times, most recently from 94d2497 to b003f0d Compare February 26, 2025 06:28
@targos
targos force-pushed the canary-base branch from b003f0d to 11edc42 Compare March 6, 2025 08:48
@joyeecheung joyeecheung mentioned this pull request Apr 10, 2025
[help wanted] deps: update V8 to 13.6 #57753
Closed
@joyeecheung
joyeecheung force-pushed the cppheap-auto-2 branch from 4176cef to 9edb1f5 Compare April 10, 2025 17:07
@joyeecheung
joyeecheung changed the base branch from canary-base to main April 12, 2025 18:21
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Apr 12, 2025
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Apr 12, 2025
@github-actions

github-actions Bot commented Apr 12, 2025

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/14422646848
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 12, 2025

Copy link
Copy Markdown
Member

This is an unfortunate side effect of the recent CI changes... we can't proactively run a CI job without having at least one sign off on the most recent commit. :-(
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 12, 2025

Copy link
Copy Markdown
Member

Also tho... 5000+ files changed!? does this need to be rebased? or is it really that big?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Apr 12, 2025

Copy link
Copy Markdown
Member Author

Also tho... 5000+ files changed!? does this need to be rebased? or is it really that big?

This is based on #57753 to check if it works with 13.6. It's not yet ready for review.
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung
joyeecheung force-pushed the cppheap-auto-2 branch from b101988 to 47c759b Compare April 12, 2025 20:08
@nodejs-github-bot

nodejs-github-bot commented Apr 12, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66224/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
joyeecheung added 3 commits April 14, 2025 13:34
@joyeecheung
src: migrate from deprecated SnapshotCreator constructor
7a4984f 
Previously we have been using the variant of SnapshotCreator that
only passes the external references instead of
v8::Isolate::CreateParams and it's about to be deprecated.
Switch to using the new constructor that takes a fully CreateParams
instead.

This also makes sure that the snapshot building script is using
the Node.js array buffer allocator instead of a separate default
one that was previously used by the old constructor. The zero fill
toggle in the Node.js array buffer allocator would still be ignored
during snapshot building, however, until we fixes the array buffer
allocator and let V8 own the toggle backing store instead, because
otherwise the snapshot would contain the external toggle address
and become unreproducible.
@joyeecheung
fixup! src: migrate from deprecated SnapshotCreator constructor
b007f46
@joyeecheung
src: use V8-owned CppHeap
0d97990 
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
joyeecheung force-pushed the cppheap-auto-2 branch from 47c759b to 0d97990 Compare April 14, 2025 13:26
@joyeecheung joyeecheung added dont-land-on-v18.x dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed blocked PRs that are blocked by other issues or PRs. request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Apr 14, 2025
@nodejs-github-bot

nodejs-github-bot commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/66260/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Apr 14, 2025

Copy link
Copy Markdown
Member Author

It seems test-heapdump-vm-script is failing on v8 13.0 (main branch) though it does not fail with v8 13.6. I think that might mean that this has to land together with the next v8 upgrade (since the owned CppHeap is not yet ready in 13.0).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung

joyeecheung commented Apr 16, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

Closing since it cannot land separately and need to land along with a V8 upgrade now (will probably be #57753).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@joyeecheung joyeecheung closed this Apr 16, 2025
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

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. 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.

v8::Isolate::AttachCppHeap is deprecated

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@joyeecheung @nodejs-github-bot @jasnell
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.