[static-roots] Refactor setup_isolate_delegate

Make the setup_isolate_delegate stateless. It does not make sense to
pass a setup delegate to Isolate::Init that would contradict the
configuration of the isolate, hence it does not make sense to let the
delegate decide if heap objects should be created. Instead let the
isolate decide on how to invoke the delegate.

Cleanup in preparation for later changes to mksnapshot.

Bug: v8:13466
Change-Id: I5ca36a1db3e94baf068ba0dc91729a78086a023c
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4020172
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Olivier Flückiger <olivf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#84550}

src/execution/isolate.cc

@@ -4352,12 +4352,12 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
 #endif  // defined(V8_OS_WIN)
 
   if (setup_delegate_ == nullptr) {
-    setup_delegate_ = new SetupIsolateDelegate(create_heap_objects);
+    setup_delegate_ = new SetupIsolateDelegate;
   }
 
   if (!v8_flags.inline_new) heap_.DisableInlineAllocation();
 
-  if (!setup_delegate_->SetupHeap(&heap_)) {
+  if (!setup_delegate_->SetupHeap(this, create_heap_objects)) {
     V8::FatalProcessOutOfMemory(this, "heap object creation");
   }
 
@@ -4378,7 +4378,7 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
   if (create_heap_objects) {
     builtins_constants_table_builder_ = new BuiltinsConstantsTableBuilder(this);
 
-    setup_delegate_->SetupBuiltins(this);
+    setup_delegate_->SetupBuiltins(this, true);
 
     builtins_constants_table_builder_->Finalize();
     delete builtins_constants_table_builder_;
@@ -4386,7 +4386,7 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
 
     CreateAndSetEmbeddedBlob();
   } else {
-    setup_delegate_->SetupBuiltins(this);
+    setup_delegate_->SetupBuiltins(this, false);
     MaybeRemapEmbeddedBuiltinsIntoCodeRange();
   }
 

src/heap/setup-heap-internal.cc

@@ -68,8 +68,8 @@ Handle<SharedFunctionInfo> CreateSharedFunctionInfo(
 
 }  // namespace
 
-bool SetupIsolateDelegate::SetupHeapInternal(Heap* heap) {
-  return heap->CreateHeapObjects();
+bool SetupIsolateDelegate::SetupHeapInternal(Isolate* isolate) {
+  return isolate->heap()->CreateHeapObjects();
 }
 
 bool Heap::CreateHeapObjects() {

src/init/setup-isolate-deserialize.cc

@@ -2,22 +2,26 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "src/init/setup-isolate.h"
-
 #include "src/base/logging.h"
+#include "src/execution/isolate.h"
+#include "src/init/setup-isolate.h"
 
 namespace v8 {
 namespace internal {
 
-void SetupIsolateDelegate::SetupBuiltins(Isolate* isolate) {
-  CHECK(!create_heap_objects_);
-  // No actual work to be done; builtins will be deserialized from the snapshot.
+bool SetupIsolateDelegate::SetupHeap(Isolate* isolate,
+                                     bool create_heap_objects) {
+  // No actual work to be done; heap will be deserialized from the snapshot.
+  CHECK_WITH_MSG(!create_heap_objects,
+                 "Heap setup supported only in mksnapshot");
+  return true;
 }
 
-bool SetupIsolateDelegate::SetupHeap(Heap* heap) {
-  CHECK(!create_heap_objects_);
-  // No actual work to be done; heap will be deserialized from the snapshot.
-  return true;
+void SetupIsolateDelegate::SetupBuiltins(Isolate* isolate,
+                                         bool compile_builtins) {
+  // No actual work to be done; builtins will be deserialized from the snapshot.
+  CHECK_WITH_MSG(!compile_builtins,
+                 "Builtin compilation supported only in mksnapshot");
 }
 
 }  // namespace internal

src/init/setup-isolate-full.cc

@@ -11,24 +11,25 @@
 namespace v8 {
 namespace internal {
 
-void SetupIsolateDelegate::SetupBuiltins(Isolate* isolate) {
-  if (create_heap_objects_) {
-    SetupBuiltinsInternal(isolate);
-#ifdef DEBUG
-    DebugEvaluate::VerifyTransitiveBuiltins(isolate);
-#endif  // DEBUG
-  } else {
+bool SetupIsolateDelegate::SetupHeap(Isolate* isolate,
+                                     bool create_heap_objects) {
+  if (!create_heap_objects) {
     CHECK(isolate->snapshot_available());
-  }
-}
-
-bool SetupIsolateDelegate::SetupHeap(Heap* heap) {
-  if (create_heap_objects_) {
-    return SetupHeapInternal(heap);
-  } else {
-    CHECK(heap->isolate()->snapshot_available());
     return true;
   }
+  return SetupHeapInternal(isolate);
+}
+
+void SetupIsolateDelegate::SetupBuiltins(Isolate* isolate,
+                                         bool compile_builtins) {
+  if (!compile_builtins) {
+    CHECK(isolate->snapshot_available());
+    return;
+  }
+  SetupBuiltinsInternal(isolate);
+#ifdef DEBUG
+  DebugEvaluate::VerifyTransitiveBuiltins(isolate);
+#endif  // DEBUG
 }
 
 }  // namespace internal

src/init/setup-isolate.h

@@ -31,13 +31,11 @@ class Isolate;
 // linked in by the latter two Delegate implementations.
 class V8_EXPORT_PRIVATE SetupIsolateDelegate {
  public:
-  explicit SetupIsolateDelegate(bool create_heap_objects)
-      : create_heap_objects_(create_heap_objects) {}
+  SetupIsolateDelegate() = default;
   virtual ~SetupIsolateDelegate() = default;
 
-  virtual void SetupBuiltins(Isolate* isolate);
-
-  virtual bool SetupHeap(Heap* heap);
+  virtual bool SetupHeap(Isolate* isolate, bool create_heap_objects);
+  virtual void SetupBuiltins(Isolate* isolate, bool compile_builtins);
 
  protected:
   static void SetupBuiltinsInternal(Isolate* isolate);
@@ -45,9 +43,7 @@ class V8_EXPORT_PRIVATE SetupIsolateDelegate {
   static void PopulateWithPlaceholders(Isolate* isolate);
   static void ReplacePlaceholders(Isolate* isolate);
 
-  static bool SetupHeapInternal(Heap* heap);
-
-  const bool create_heap_objects_;
+  static bool SetupHeapInternal(Isolate* isolate);
 };
 
 }  // namespace internal

test/cctest/setup-isolate-for-tests.cc

@@ -4,20 +4,22 @@
 
 #include "test/cctest/setup-isolate-for-tests.h"
 
+// Almost identical to setup-isolate-full.cc. The difference is that while
+// testing the embedded snapshot blob can be missing.
+
 namespace v8 {
 namespace internal {
 
-void SetupIsolateDelegateForTests::SetupBuiltins(Isolate* isolate) {
-  if (create_heap_objects_) {
-    SetupBuiltinsInternal(isolate);
-  }
+bool SetupIsolateDelegateForTests::SetupHeap(Isolate* isolate,
+                                             bool create_heap_objects) {
+  if (!create_heap_objects) return true;
+  return SetupHeapInternal(isolate);
 }
 
-bool SetupIsolateDelegateForTests::SetupHeap(Heap* heap) {
-  if (create_heap_objects_) {
-    return SetupHeapInternal(heap);
-  }
-  return true;
+void SetupIsolateDelegateForTests::SetupBuiltins(Isolate* isolate,
+                                                 bool compile_builtins) {
+  if (!compile_builtins) return;
+  SetupBuiltinsInternal(isolate);
 }
 
 }  // namespace internal

test/cctest/setup-isolate-for-tests.h

@@ -12,13 +12,10 @@ namespace internal {
 
 class SetupIsolateDelegateForTests : public SetupIsolateDelegate {
  public:
-  explicit SetupIsolateDelegateForTests(bool create_heap_objects)
-      : SetupIsolateDelegate(create_heap_objects) {}
-  ~SetupIsolateDelegateForTests() override = default;
+  SetupIsolateDelegateForTests() = default;
 
-  void SetupBuiltins(Isolate* isolate) override;
-
-  bool SetupHeap(Heap* heap) override;
+  bool SetupHeap(Isolate* isolate, bool create_heap_objects) override;
+  void SetupBuiltins(Isolate* isolate, bool compile_builtins) override;
 };
 
 }  // namespace internal

test/cctest/test-serialize.cc

@@ -91,14 +91,12 @@ class TestSerializer {
  public:
   static v8::Isolate* NewIsolateInitialized() {
     const bool kEnableSerializer = true;
-    const bool kGenerateHeap = true;
     const bool kIsShared = false;
     DisableEmbeddedBlobRefcounting();
-    v8::Isolate* v8_isolate =
-        NewIsolate(kEnableSerializer, kGenerateHeap, kIsShared);
+    v8::Isolate* v8_isolate = NewIsolate(kEnableSerializer, kIsShared);
     v8::Isolate::Scope isolate_scope(v8_isolate);
     i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
-    isolate->Init(nullptr, nullptr, nullptr, false);
+    isolate->InitWithoutSnapshot();
     return v8_isolate;
   }
 
@@ -107,10 +105,8 @@ class TestSerializer {
   // the production Isolate class has one or the other behavior baked in.
   static v8::Isolate* NewIsolate(const v8::Isolate::CreateParams& params) {
     const bool kEnableSerializer = false;
-    const bool kGenerateHeap = params.snapshot_blob == nullptr;
     const bool kIsShared = false;
-    v8::Isolate* v8_isolate =
-        NewIsolate(kEnableSerializer, kGenerateHeap, kIsShared);
+    v8::Isolate* v8_isolate = NewIsolate(kEnableSerializer, kIsShared);
     v8::Isolate::Initialize(v8_isolate, params);
     return v8_isolate;
   }
@@ -120,14 +116,12 @@ class TestSerializer {
     SnapshotData read_only_snapshot(blobs.read_only);
     SnapshotData shared_space_snapshot(blobs.shared_space);
     const bool kEnableSerializer = false;
-    const bool kGenerateHeap = false;
     const bool kIsShared = false;
-    v8::Isolate* v8_isolate =
-        NewIsolate(kEnableSerializer, kGenerateHeap, kIsShared);
+    v8::Isolate* v8_isolate = NewIsolate(kEnableSerializer, kIsShared);
     v8::Isolate::Scope isolate_scope(v8_isolate);
     i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
-    isolate->Init(&startup_snapshot, &read_only_snapshot,
-                  &shared_space_snapshot, false);
+    isolate->InitWithSnapshot(&startup_snapshot, &read_only_snapshot,
+                              &shared_space_snapshot, false);
     return v8_isolate;
   }
 
@@ -141,14 +135,12 @@ class TestSerializer {
     SnapshotData read_only_snapshot(blobs.read_only);
     SnapshotData shared_space_snapshot(blobs.shared_space);
     const bool kEnableSerializer = false;
-    const bool kGenerateHeap = false;
     const bool kIsShared = true;
-    v8::Isolate* v8_isolate =
-        NewIsolate(kEnableSerializer, kGenerateHeap, kIsShared);
+    v8::Isolate* v8_isolate = NewIsolate(kEnableSerializer, kIsShared);
     v8::Isolate::Scope isolate_scope(v8_isolate);
     i::Isolate* isolate = reinterpret_cast<i::Isolate*>(v8_isolate);
-    isolate->Init(&startup_snapshot, &read_only_snapshot,
-                  &shared_space_snapshot, false);
+    isolate->InitWithSnapshot(&startup_snapshot, &read_only_snapshot,
+                              &shared_space_snapshot, false);
     i::Isolate::process_wide_shared_isolate_ = isolate;
   }
 
@@ -158,8 +150,7 @@ class TestSerializer {
 
  private:
   // Creates an Isolate instance configured for testing.
-  static v8::Isolate* NewIsolate(bool with_serializer, bool generate_heap,
-                                 bool is_shared) {
+  static v8::Isolate* NewIsolate(bool with_serializer, bool is_shared) {
     i::Isolate* isolate;
     if (is_shared) {
       isolate = i::Isolate::Allocate(true);
@@ -170,7 +161,7 @@ class TestSerializer {
 
     if (with_serializer) isolate->enable_serializer();
     isolate->set_array_buffer_allocator(CcTest::array_buffer_allocator());
-    isolate->setup_delegate_ = new SetupIsolateDelegateForTests(generate_heap);
+    isolate->setup_delegate_ = new SetupIsolateDelegateForTests;
 
     return v8_isolate;
   }