[api] Allow passing CppHeap on Isolate creation

mlippautz · V8 LUCI CQ · commit d5631f63d2ea · 2023-10-31T08:39:46.000Z
This is a first step towards always having a CppHeap when constructing V8 Isolates. AttachCppHeap() and DetachedCppHeap() are deprecated. Once they are removed, V8 will either be constructed with an API provided CppHeap or will initialize a Heap internally, so that code can rely on its existence. Bug: v8:13754 Change-Id: Ica37dca5da7d30e7f0f3fcbac96c2afe7624f84d Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4989254 Commit-Queue: Michael Lippautz <mlippautz@chromium.org> Reviewed-by: Anton Bikineev <bikineev@chromium.org> Reviewed-by: Dominik Inführ <dinfuehr@chromium.org> Cr-Commit-Position: refs/heads/main@{#90675}
diff --git a/include/v8-isolate.h b/include/v8-isolate.h
@@ -294,6 +294,12 @@ class V8_EXPORT Isolate {
      */
     FatalErrorCallback fatal_error_callback = nullptr;
     OOMErrorCallback oom_error_callback = nullptr;
+
+    /**
+     * A CppHeap used to construct the Isolate. V8 takes ownership of the
+     * CppHeap passed this way.
+     */
+    CppHeap* cpp_heap = nullptr;
   };
 
   /**
@@ -1013,12 +1019,20 @@ class V8_EXPORT Isolate {
    *
    * Multi-threaded use requires the use of v8::Locker/v8::Unlocker, see
    * CppHeap.
+   *
+   * If a CppHeap is set via CreateParams, then this call is a noop.
    */
+  V8_DEPRECATE_SOON(
+      "Set the heap on Isolate creation using CreateParams instead.")
   void AttachCppHeap(CppHeap*);
 
   /**
    * Detaches a managed C++ heap if one was attached using `AttachCppHeap()`.
+   *
+   * If a CppHeap is set via CreateParams, then this call is a noop.
    */
+  V8_DEPRECATE_SOON(
+      "Set the heap on Isolate creation using CreateParams instead.")
   void DetachCppHeap();
 
   /**
diff --git a/src/api/api.cc b/src/api/api.cc
@@ -9594,7 +9594,7 @@ void Isolate::Initialize(Isolate* v8_isolate,
   i_isolate->set_api_external_references(params.external_references);
   i_isolate->set_allow_atomics_wait(params.allow_atomics_wait);
 
-  i_isolate->heap()->ConfigureHeap(params.constraints);
+  i_isolate->heap()->ConfigureHeap(params.constraints, params.cpp_heap);
   if (params.constraints.stack_limit() != nullptr) {
     uintptr_t limit =
         reinterpret_cast<uintptr_t>(params.constraints.stack_limit());
diff --git a/src/heap/heap.cc b/src/heap/heap.cc
@@ -4893,7 +4893,8 @@ size_t Heap::OldGenerationToSemiSpaceRatioLowMemory() {
   return kOldGenerationToSemiSpaceRatioLowMemory / (v8_flags.minor_ms ? 2 : 1);
 }
 
-void Heap::ConfigureHeap(const v8::ResourceConstraints& constraints) {
+void Heap::ConfigureHeap(const v8::ResourceConstraints& constraints,
+                         v8::CppHeap* cpp_heap) {
   // Initialize max_semi_space_size_.
   {
     max_semi_space_size_ = DefaultMaxSemiSpaceSize();
@@ -5054,6 +5055,11 @@ void Heap::ConfigureHeap(const v8::ResourceConstraints& constraints) {
 
   code_range_size_ = constraints.code_range_size_in_bytes();
 
+  if (cpp_heap) {
+    AttachCppHeap(cpp_heap);
+    owning_cpp_heap_.reset(CppHeap::From(cpp_heap));
+  }
+
   configured_ = true;
 }
 
@@ -5081,7 +5087,7 @@ void Heap::GetFromRingBuffer(char* buffer) {
 
 void Heap::ConfigureHeapDefault() {
   v8::ResourceConstraints constraints;
-  ConfigureHeap(constraints);
+  ConfigureHeap(constraints, nullptr);
 }
 
 void Heap::RecordStats(HeapStats* stats, bool take_snapshot) {
@@ -5818,12 +5824,24 @@ EmbedderRootsHandler* Heap::GetEmbedderRootsHandler() const {
 }
 
 void Heap::AttachCppHeap(v8::CppHeap* cpp_heap) {
+  // The API function should be a noop in case a CppHeap was passed on Isolate
+  // creation.
+  if (owning_cpp_heap_) {
+    return;
+  }
+
   CHECK(!incremental_marking()->IsMarking());
   CppHeap::From(cpp_heap)->AttachIsolate(isolate());
   cpp_heap_ = cpp_heap;
 }
 
 void Heap::DetachCppHeap() {
+  // The API function should be a noop in case a CppHeap was passed on Isolate
+  // creation.
+  if (owning_cpp_heap_) {
+    return;
+  }
+
   CppHeap::From(cpp_heap_)->DetachIsolate();
   cpp_heap_ = nullptr;
 }
@@ -5840,6 +5858,15 @@ void Heap::SetStackStart(void* stack_start) {
 ::heap::base::Stack& Heap::stack() { return isolate_->stack(); }
 
 void Heap::StartTearDown() {
+  if (owning_cpp_heap_) {
+    // Release the pointer. The non-owning pointer is still set which allows
+    // DetachCppHeap() to work properly.
+    auto* cpp_heap = owning_cpp_heap_.release();
+    DetachCppHeap();
+    // Termination will free up all managed C++ memory and invoke destructors.
+    cpp_heap->Terminate();
+  }
+
   // Finish any ongoing sweeping to avoid stray background tasks still accessing
   // the heap during teardown.
   CompleteSweepingFull();
diff --git a/src/heap/heap.h b/src/heap/heap.h
@@ -712,7 +712,8 @@ class Heap final {
   // Initialization. ===========================================================
   // ===========================================================================
 
-  void ConfigureHeap(const v8::ResourceConstraints& constraints);
+  void ConfigureHeap(const v8::ResourceConstraints& constraints,
+                     v8::CppHeap* cpp_heap);
   void ConfigureHeapDefault();
 
   // Prepares the heap, setting up for deserialization.
@@ -2269,7 +2270,14 @@ class Heap final {
   TrustedRange* trusted_range_ = nullptr;
 #endif
 
-  v8::CppHeap* cpp_heap_ = nullptr;  // Owned by the embedder.
+  // V8 configuration where V8 owns the heap which is either created or passed
+  // in during Isolate initialization.
+  std::unique_ptr<CppHeap> owning_cpp_heap_;
+  // Deprecated API where the heap is owned by the embedder. This field is
+  // always set, independent of which CppHeap configuration (owned, unowned) is
+  // used. As soon as Isolate::AttachCppHeap() is removed, this field should
+  // also be removed and we should exclusively rely on the owning version.
+  v8::CppHeap* cpp_heap_ = nullptr;
   EmbedderRootsHandler* embedder_roots_handler_ =
       nullptr;  // Owned by the embedder.
 
diff --git a/src/snapshot/mksnapshot.cc b/src/snapshot/mksnapshot.cc
@@ -279,7 +279,7 @@ int main(int argc, char** argv) {
                          i::kMaxPCRelativeCodeRangeInMB);
       v8::ResourceConstraints constraints;
       constraints.set_code_range_size_in_bytes(code_range_size_mb * i::MB);
-      i_isolate->heap()->ConfigureHeap(constraints);
+      i_isolate->heap()->ConfigureHeap(constraints, nullptr);
       // The isolate contains data from builtin compilation that needs
       // to be written out if builtins are embedded.
       i_isolate->RegisterEmbeddedFileWriter(&embedded_writer);
