src: use V8-owned CppHeap #53205

joyeecheung · 2024-05-29T18:14:12Z

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.

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

jasnell · 2024-05-29T18:32:26Z

src/api/environment.cc

+  // Ensure that there is always a CppHeap.
+  if (settings.cpp_heap == nullptr) {
+    params->cpp_heap =
+        CppHeap::Create(platform, CppHeapCreateParams{{}}).release();


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

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.

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 · 2024-05-29T18:41:58Z

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

joyeecheung · 2024-05-30T16:42:18Z

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 · 2024-06-01T13:14:12Z

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

joyeecheung · 2024-06-04T11:14:24Z

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 · 2024-06-05T08:07:08Z

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

github-actions · 2025-04-12T19:20:37Z

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 · 2025-04-12T19:48:17Z

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 · 2025-04-12T19:49:57Z

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

joyeecheung · 2025-04-12T20:06:58Z

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 · 2025-04-12T20:09:32Z

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

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.

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

nodejs-github-bot · 2025-04-14T13:28:08Z

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

joyeecheung · 2025-04-14T15:47:17Z

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 · 2025-04-16T15:05:46Z

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

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

src: remove dependency on wrapper-descriptor-based cpp heap #53086

Closed

joyeecheung force-pushed the cppheap-auto-2 branch from 39d25ce to b224a56 Compare May 29, 2024 18:29

jasnell reviewed May 29, 2024

View reviewed changes

joyeecheung force-pushed the cppheap-auto-2 branch from b224a56 to 4176cef Compare May 30, 2024 16:38

targos force-pushed the canary-base branch from a1c9178 to 30329d0 Compare June 4, 2024 11:22

joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jun 10, 2024

targos force-pushed the canary-base branch from 30329d0 to cb3ed9c Compare June 14, 2024 07:11

targos force-pushed the canary-base branch from cb3ed9c to 7f25854 Compare June 30, 2024 09:24

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 from 19f043a to 8fc9eba Compare August 24, 2024 09:28

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 from 7b0a20c to 199cba3 Compare September 19, 2024 06:15

targos force-pushed the canary-base branch from 7e95ee1 to aabb4b6 Compare February 15, 2025 09:55

targos force-pushed the canary-base branch 2 times, most recently from 94d2497 to b003f0d Compare February 26, 2025 06:28

targos force-pushed the canary-base branch from b003f0d to 11edc42 Compare March 6, 2025 08:48

joyeecheung mentioned this pull request Apr 10, 2025

[help wanted] deps: update V8 to 13.6 #57753

Closed

joyeecheung force-pushed the cppheap-auto-2 branch from 4176cef to 9edb1f5 Compare April 10, 2025 17:07

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

joyeecheung force-pushed the cppheap-auto-2 branch from b101988 to 47c759b Compare April 12, 2025 20:08

joyeecheung added 3 commits April 14, 2025 13:34

fixup! src: migrate from deprecated SnapshotCreator constructor

b007f46

joyeecheung force-pushed the cppheap-auto-2 branch from 47c759b to 0d97990 Compare April 14, 2025 13:26

joyeecheung closed this Apr 16, 2025

Uh oh!

src: use V8-owned CppHeap #53205

src: use V8-owned CppHeap #53205

Uh oh!

Conversation

joyeecheung commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell May 29, 2024

Choose a reason for hiding this comment

Uh oh!

joyeecheung May 29, 2024

Choose a reason for hiding this comment

Uh oh!

jasnell May 29, 2024

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented May 29, 2024

Uh oh!

joyeecheung commented May 30, 2024

Uh oh!

nodejs-github-bot commented Jun 1, 2024

Uh oh!

joyeecheung commented Jun 4, 2024

Uh oh!

joyeecheung commented Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Apr 12, 2025

Uh oh!

jasnell commented Apr 12, 2025

Uh oh!

jasnell commented Apr 12, 2025

Uh oh!

joyeecheung commented Apr 12, 2025

Uh oh!

nodejs-github-bot commented Apr 12, 2025

Uh oh!

nodejs-github-bot commented Apr 14, 2025

Uh oh!

joyeecheung commented Apr 14, 2025

Uh oh!

joyeecheung commented Apr 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joyeecheung commented May 29, 2024 •

edited

Loading

joyeecheung commented Jun 5, 2024 •

edited

Loading

joyeecheung commented Apr 16, 2025 •

edited

Loading