node
node copied to clipboard
nodejs
src: use V8-owned CppHeap
This is partly based on https://github.com/nodejs/node/pull/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: https://github.com/nodejs/node/issues/52718
After some experiments it seems the option that makes the most sense is what @addaleax proposed in https://github.com/nodejs/node/pull/53086#issuecomment-2128056793 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).
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..
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?)
Failed to start CI
⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/14422646848
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
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. :-(
Also tho... 5000+ files changed!? does this need to be rebased? or is it really that big?
Also tho... 5000+ files changed!? does this need to be rebased? or is it really that big?
This is based on https://github.com/nodejs/node/pull/57753 to check if it works with 13.6. It's not yet ready for review.
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).
Closing since it cannot land separately and need to land along with a V8 upgrade now (will probably be https://github.com/nodejs/node/pull/57753).