FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

src: use V8-owned CppHeap by joyeecheung · Pull Request #53205 · nodejs/node · GitHub

/ node Public

src: use V8-owned CppHeap - #53205

Closed
joyeecheung wants to merge 3 commits into
nodejs:mainfrom
joyeecheung:cppheap-auto-2
ConversationCommits (3)ChecksFiles changed
Closed

src: use V8-owned CppHeap#53205
joyeecheung wants to merge 3 commits into
nodejs:mainfrom
joyeecheung:cppheap-auto-2

Conversation

joyeecheung commented May 29, 2024 •
edited
Loading

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

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 mentioned this pull request May 29, 2024
Closed
jasnell reviewed May 29, 2024
View reviewed changes
Comment thread src/api/environment.cc Outdated

jasnell May 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

joyeecheung May 29, 2024

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

jasnell May 29, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented May 29, 2024

Copy link
Copy Markdown
Collaborator

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

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

nodejs-github-bot commented Jun 1, 2024

Copy link
Copy Markdown
Collaborator

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

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

joyeecheung commented Jun 5, 2024 •
edited
Loading

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?)

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jun 10, 2024
targos force-pushed the canary-base branch 4 times, most recently from 04423fd to 8e3fab5 Compare August 3, 2024 16:14
targos force-pushed the canary-base branch 3 times, most recently from 7177ac5 to 19f043a Compare August 16, 2024 14:14
targos force-pushed the canary-base branch 2 times, most recently from 8d54422 to 290e38c Compare September 8, 2024 07:51
targos force-pushed the canary-base branch 3 times, most recently from 7919d6a to 7b0a20c Compare September 16, 2024 06:09
targos force-pushed the canary-base branch 2 times, most recently from 94d2497 to b003f0d Compare February 26, 2025 06:28
joyeecheung mentioned this pull request Apr 10, 2025
Closed
joyeecheung changed the base branch from canary-base to main April 12, 2025 18:21
joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 12, 2025
github-actions Bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 12, 2025

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

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

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?

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.

nodejs-github-bot commented Apr 12, 2025

Copy link
Copy Markdown
Collaborator

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

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.
fixup! src: migrate from deprecated SnapshotCreator constructor
b007f46
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 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 An error occurred while starting CI via request-ci label, and manual interventon is needed. labels Apr 14, 2025

nodejs-github-bot commented Apr 14, 2025

Copy link
Copy Markdown
Collaborator

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

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

joyeecheung commented Apr 16, 2025 •
edited
Loading

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

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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments

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

v8::Isolate::AttachCppHeap is deprecated

3 participants

Back | FazBrowse Home | New Git URL