[heap] Invoke internal GC callbacks before Heap verification

- Move InnerPointerToCodeCache to using internal callbacks.
- Refactor internal and external callbacks to use a unified interface.

Bug: v8:13184
Change-Id: If0006d324b0433f5d6bbf00b6d0fc1a2589227bc
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3834583Reviewed-by: Hannes Payer <hpayer@chromium.org>
Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#82515}

BUILD.bazel

@@ -1444,6 +1444,7 @@ filegroup(
         "src/heap/free-list-inl.h",
         "src/heap/free-list.cc",
         "src/heap/free-list.h",
+        "src/heap/gc-callbacks.h",
         "src/heap/gc-idle-time-handler.cc",
         "src/heap/gc-idle-time-handler.h",
         "src/heap/gc-tracer.cc",

BUILD.gn

@@ -3124,6 +3124,7 @@ v8_header_set("v8_internal_headers") {
     "src/heap/finalization-registry-cleanup-task.h",
     "src/heap/free-list-inl.h",
     "src/heap/free-list.h",
+    "src/heap/gc-callbacks.h",
     "src/heap/gc-idle-time-handler.h",
     "src/heap/gc-tracer-inl.h",
     "src/heap/gc-tracer.h",

src/baseline/bytecode-offset-iterator.h

@@ -66,7 +66,8 @@ class V8_EXPORT_PRIVATE BytecodeOffsetIterator {
     return current_bytecode_offset_;
   }
 
-  static void UpdatePointersCallback(void* iterator) {
+  static void UpdatePointersCallback(LocalIsolate*, GCType, GCCallbackFlags,
+                                     void* iterator) {
     reinterpret_cast<BytecodeOffsetIterator*>(iterator)->UpdatePointers();
   }
 

src/execution/frames-inl.h

@@ -27,21 +27,20 @@ class InnerPointerToCodeCache {
     InnerPointerToCodeCacheEntry() : safepoint_entry() {}
   };
 
-  static void FlushCallback(v8::Isolate* isolate, v8::GCType type,
-                            v8::GCCallbackFlags flags, void* data) {
-    InnerPointerToCodeCache* cache =
-        static_cast<InnerPointerToCodeCache*>(data);
-    cache->Flush();
+  static void FlushCallback(LocalIsolate*, GCType, GCCallbackFlags,
+                            void* data) {
+    static_cast<InnerPointerToCodeCache*>(data)->Flush();
   }
 
   explicit InnerPointerToCodeCache(Isolate* isolate) : isolate_(isolate) {
     Flush();
-    isolate_->heap()->AddGCEpilogueCallback(FlushCallback,
-                                            kGCTypeMarkSweepCompact, this);
+    isolate_->main_thread_local_heap()->AddGCEpilogueCallback(
+        FlushCallback, this, GCType::kGCTypeMarkSweepCompact);
   }
 
   ~InnerPointerToCodeCache() {
-    isolate_->heap()->RemoveGCEpilogueCallback(FlushCallback, this);
+    isolate_->main_thread_local_heap()->RemoveGCEpilogueCallback(FlushCallback,
+                                                                 this);
   }
 
   InnerPointerToCodeCache(const InnerPointerToCodeCache&) = delete;

src/execution/isolate.cc

@@ -3537,6 +3537,9 @@ void Isolate::Deinit() {
 
   heap_.TearDown();
 
+  delete inner_pointer_to_code_cache_;
+  inner_pointer_to_code_cache_ = nullptr;
+
   main_thread_local_isolate_.reset();
 
   FILE* logfile = v8_file_logger_->TearDownAndGetLogFile();
@@ -3634,8 +3637,6 @@ Isolate::~Isolate() {
   compilation_cache_ = nullptr;
   delete bootstrapper_;
   bootstrapper_ = nullptr;
-  delete inner_pointer_to_code_cache_;
-  inner_pointer_to_code_cache_ = nullptr;
 
   delete thread_manager_;
   thread_manager_ = nullptr;
@@ -4019,7 +4020,6 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
 
   compilation_cache_ = new CompilationCache(this);
   descriptor_lookup_cache_ = new DescriptorLookupCache();
-  inner_pointer_to_code_cache_ = new InnerPointerToCodeCache(this);
   global_handles_ = new GlobalHandles(this);
   eternal_handles_ = new EternalHandles();
   bootstrapper_ = new Bootstrapper(this);
@@ -4067,6 +4067,9 @@ bool Isolate::Init(SnapshotData* startup_snapshot_data,
     main_thread_local_heap()->Unpark();
   }
 
+  // Requires a LocalHeap to be set up to register a GC epilogue callback.
+  inner_pointer_to_code_cache_ = new InnerPointerToCodeCache(this);
+
   // Lock clients_mutex_ in order to prevent shared GCs from other clients
   // during deserialization.
   base::Optional<base::MutexGuard> clients_guard;

src/heap/gc-callbacks.h

+// Copyright 2020 the V8 project authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef V8_HEAP_GC_CALLBACKS_H_
+#define V8_HEAP_GC_CALLBACKS_H_
+
+#include <algorithm>
+#include <tuple>
+#include <vector>
+
+#include "include/v8-callbacks.h"
+#include "src/base/logging.h"
+
+namespace v8::internal {
+
+template <typename IsolateType, typename InvokeScope>
+class GCCallbacks final {
+ public:
+  using CallbackType = void (*)(IsolateType*, GCType, GCCallbackFlags, void*);
+
+  constexpr GCCallbacks() = default;
+
+  void Add(CallbackType callback, IsolateType* isolate, GCType gc_type,
+           void* data) {
+    DCHECK_NOT_NULL(callback);
+    DCHECK_EQ(callbacks_.end(),
+              std::find(callbacks_.begin(), callbacks_.end(),
+                        CallbackData{callback, isolate, gc_type, data}));
+    callbacks_.emplace_back(callback, isolate, gc_type, data);
+  }
+
+  void Remove(CallbackType callback, void* data) {
+    auto it =
+        std::find(callbacks_.begin(), callbacks_.end(),
+                  CallbackData{callback, nullptr, GCType::kGCTypeAll, data});
+    DCHECK_NE(callbacks_.end(), it);
+    *it = callbacks_.back();
+    callbacks_.pop_back();
+  }
+
+  void Invoke(GCType gc_type, GCCallbackFlags gc_callback_flags) const {
+    InvokeScope scope;
+    for (const CallbackData& callback_data : callbacks_) {
+      if (gc_type & callback_data.gc_type) {
+        callback_data.callback(callback_data.isolate, gc_type,
+                               gc_callback_flags, callback_data.user_data);
+      }
+    }
+  }
+
+  bool IsEmpty() const { return callbacks_.empty(); }
+
+ private:
+  struct CallbackData final {
+    CallbackData(CallbackType callback, IsolateType* isolate, GCType gc_type,
+                 void* user_data)
+        : callback(callback),
+          isolate(isolate),
+          gc_type(gc_type),
+          user_data(user_data) {}
+
+    bool operator==(const CallbackData& other) const {
+      return callback == other.callback && user_data == other.user_data;
+    }
+
+    CallbackType callback;
+    IsolateType* isolate;
+    GCType gc_type;
+    void* user_data;
+  };
+
+  std::vector<CallbackData> callbacks_;
+};
+
+}  // namespace v8::internal
+
+#endif  // V8_HEAP_GC_CALLBACKS_H_

src/heap/heap.cc

@@ -183,12 +183,6 @@ void Heap::SetSerializedGlobalProxySizes(FixedArray sizes) {
 void Heap::SetBasicBlockProfilingData(Handle<ArrayList> list) {
   set_basic_block_profiling_data(*list);
 }
-
-bool Heap::GCCallbackTuple::operator==(
-    const Heap::GCCallbackTuple& other) const {
-  return other.callback == callback && other.data == data;
-}
-
 class ScavengeTaskObserver : public AllocationObserver {
  public:
   ScavengeTaskObserver(Heap* heap, intptr_t step_size)
@@ -1383,6 +1377,19 @@ void Heap::DeoptMarkedAllocationSites() {
   Deoptimizer::DeoptimizeMarkedCode(isolate_);
 }
 
+static GCType GetGCTypeFromGarbageCollector(GarbageCollector collector) {
+  switch (collector) {
+    case GarbageCollector::MARK_COMPACTOR:
+      return kGCTypeMarkSweepCompact;
+    case GarbageCollector::SCAVENGER:
+      return kGCTypeScavenge;
+    case GarbageCollector::MINOR_MARK_COMPACTOR:
+      return kGCTypeMinorMarkCompact;
+    default:
+      UNREACHABLE();
+  }
+}
+
 void Heap::GarbageCollectionEpilogueInSafepoint(GarbageCollector collector) {
   if (collector == GarbageCollector::MARK_COMPACTOR) {
     memory_pressure_level_.store(MemoryPressureLevel::kNone,
@@ -1391,8 +1398,9 @@ void Heap::GarbageCollectionEpilogueInSafepoint(GarbageCollector collector) {
 
   TRACE_GC(tracer(), GCTracer::Scope::HEAP_EPILOGUE_SAFEPOINT);
 
-  safepoint()->IterateLocalHeaps([](LocalHeap* local_heap) {
-    local_heap->InvokeGCEpilogueCallbacksInSafepoint();
+  safepoint()->IterateLocalHeaps([this, collector](LocalHeap* local_heap) {
+    local_heap->InvokeGCEpilogueCallbacksInSafepoint(
+        GetGCTypeFromGarbageCollector(collector), current_gc_callback_flags_);
   });
 
 #define UPDATE_COUNTERS_FOR_SPACE(space)                \
@@ -1740,19 +1748,6 @@ Heap::DevToolsTraceEventScope::~DevToolsTraceEventScope() {
                    heap_->SizeOfObjects());
 }
 
-static GCType GetGCTypeFromGarbageCollector(GarbageCollector collector) {
-  switch (collector) {
-    case GarbageCollector::MARK_COMPACTOR:
-      return kGCTypeMarkSweepCompact;
-    case GarbageCollector::SCAVENGER:
-      return kGCTypeScavenge;
-    case GarbageCollector::MINOR_MARK_COMPACTOR:
-      return kGCTypeMinorMarkCompact;
-    default:
-      UNREACHABLE();
-  }
-}
-
 bool Heap::CollectGarbage(AllocationSpace space,
                           GarbageCollectionReason gc_reason,
                           const v8::GCCallbackFlags gc_callback_flags) {
@@ -2344,6 +2339,12 @@ size_t Heap::PerformGarbageCollection(
   }
 #endif  // defined(CPPGC_YOUNG_GENERATION)
 
+  RecomputeLimits(collector);
+
+  GarbageCollectionEpilogueInSafepoint(collector);
+
+  tracer()->StopInSafepoint();
+
 #ifdef VERIFY_HEAP
   if (FLAG_verify_heap) {
     // We don't really perform a GC here but need this scope for the nested
@@ -2353,12 +2354,6 @@ size_t Heap::PerformGarbageCollection(
   }
 #endif  // VERIFY_HEAP
 
-  RecomputeLimits(collector);
-
-  GarbageCollectionEpilogueInSafepoint(collector);
-
-  tracer()->StopInSafepoint();
-
   return freed_global_handles;
 }
 
@@ -2537,22 +2532,12 @@ void Heap::RecomputeLimits(GarbageCollector collector) {
 
 void Heap::CallGCPrologueCallbacks(GCType gc_type, GCCallbackFlags flags) {
   RCS_SCOPE(isolate(), RuntimeCallCounterId::kGCPrologueCallback);
-  for (const GCCallbackTuple& info : gc_prologue_callbacks_) {
-    if (gc_type & info.gc_type) {
-      v8::Isolate* isolate = reinterpret_cast<v8::Isolate*>(this->isolate());
-      info.callback(isolate, gc_type, flags, info.data);
-    }
-  }
+  gc_prologue_callbacks_.Invoke(gc_type, flags);
 }
 
 void Heap::CallGCEpilogueCallbacks(GCType gc_type, GCCallbackFlags flags) {
   RCS_SCOPE(isolate(), RuntimeCallCounterId::kGCEpilogueCallback);
-  for (const GCCallbackTuple& info : gc_epilogue_callbacks_) {
-    if (gc_type & info.gc_type) {
-      v8::Isolate* isolate = reinterpret_cast<v8::Isolate*>(this->isolate());
-      info.callback(isolate, gc_type, flags, info.data);
-    }
-  }
+  gc_epilogue_callbacks_.Invoke(gc_type, flags);
 }
 
 void Heap::MarkCompact() {
@@ -6177,48 +6162,24 @@ void Heap::TearDown() {
 
 void Heap::AddGCPrologueCallback(v8::Isolate::GCCallbackWithData callback,
                                  GCType gc_type, void* data) {
-  DCHECK_NOT_NULL(callback);
-  DCHECK(gc_prologue_callbacks_.end() ==
-         std::find(gc_prologue_callbacks_.begin(), gc_prologue_callbacks_.end(),
-                   GCCallbackTuple(callback, gc_type, data)));
-  gc_prologue_callbacks_.emplace_back(callback, gc_type, data);
+  gc_prologue_callbacks_.Add(
+      callback, reinterpret_cast<v8::Isolate*>(isolate()), gc_type, data);
 }
 
 void Heap::RemoveGCPrologueCallback(v8::Isolate::GCCallbackWithData callback,
                                     void* data) {
-  DCHECK_NOT_NULL(callback);
-  for (size_t i = 0; i < gc_prologue_callbacks_.size(); i++) {
-    if (gc_prologue_callbacks_[i].callback == callback &&
-        gc_prologue_callbacks_[i].data == data) {
-      gc_prologue_callbacks_[i] = gc_prologue_callbacks_.back();
-      gc_prologue_callbacks_.pop_back();
-      return;
-    }
-  }
-  UNREACHABLE();
+  gc_prologue_callbacks_.Remove(callback, data);
 }
 
 void Heap::AddGCEpilogueCallback(v8::Isolate::GCCallbackWithData callback,
                                  GCType gc_type, void* data) {
-  DCHECK_NOT_NULL(callback);
-  DCHECK(gc_epilogue_callbacks_.end() ==
-         std::find(gc_epilogue_callbacks_.begin(), gc_epilogue_callbacks_.end(),
-                   GCCallbackTuple(callback, gc_type, data)));
-  gc_epilogue_callbacks_.emplace_back(callback, gc_type, data);
+  gc_epilogue_callbacks_.Add(
+      callback, reinterpret_cast<v8::Isolate*>(isolate()), gc_type, data);
 }
 
 void Heap::RemoveGCEpilogueCallback(v8::Isolate::GCCallbackWithData callback,
                                     void* data) {
-  DCHECK_NOT_NULL(callback);
-  for (size_t i = 0; i < gc_epilogue_callbacks_.size(); i++) {
-    if (gc_epilogue_callbacks_[i].callback == callback &&
-        gc_epilogue_callbacks_[i].data == data) {
-      gc_epilogue_callbacks_[i] = gc_epilogue_callbacks_.back();
-      gc_epilogue_callbacks_.pop_back();
-      return;
-    }
-  }
-  UNREACHABLE();
+  gc_epilogue_callbacks_.Remove(callback, data);
 }
 
 namespace {

src/heap/heap.h

@@ -29,6 +29,7 @@
 #include "src/heap/allocation-observer.h"
 #include "src/heap/allocation-result.h"
 #include "src/heap/base/stack.h"
+#include "src/heap/gc-callbacks.h"
 #include "src/heap/heap-allocator.h"
 #include "src/init/heap-symbols.h"
 #include "src/objects/allocation-site.h"
@@ -1761,18 +1762,6 @@ class Heap {
     RootIndex index;
   };
 
-  struct GCCallbackTuple {
-    GCCallbackTuple(v8::Isolate::GCCallbackWithData callback, GCType gc_type,
-                    void* data)
-        : callback(callback), gc_type(gc_type), data(data) {}
-
-    bool operator==(const GCCallbackTuple& other) const;
-
-    v8::Isolate::GCCallbackWithData callback;
-    GCType gc_type;
-    void* data;
-  };
-
   static const int kInitialEvalCacheSize = 64;
   static const int kInitialNumberStringCacheSize = 256;
 
@@ -2293,8 +2282,8 @@ class Heap {
   // Weak list tails.
   Object dirty_js_finalization_registries_list_tail_;
 
-  std::vector<GCCallbackTuple> gc_epilogue_callbacks_;
-  std::vector<GCCallbackTuple> gc_prologue_callbacks_;
+  GCCallbacks<v8::Isolate, AllowGarbageCollection> gc_prologue_callbacks_;
+  GCCallbacks<v8::Isolate, AllowGarbageCollection> gc_epilogue_callbacks_;
 
   GetExternallyAllocatedMemoryInBytesCallback external_memory_callback_;
 

src/heap/local-heap.cc

@@ -97,7 +97,7 @@ LocalHeap::~LocalHeap() {
     current_local_heap = nullptr;
   }
 
-  DCHECK(gc_epilogue_callbacks_.empty());
+  DCHECK(gc_epilogue_callbacks_.IsEmpty());
 }
 
 void LocalHeap::SetUpMainThreadForTesting() { SetUpMainThread(); }
@@ -410,30 +410,22 @@ Address LocalHeap::PerformCollectionAndAllocateAgain(
   heap_->FatalProcessOutOfMemory("LocalHeap: allocation failed");
 }
 
-void LocalHeap::AddGCEpilogueCallback(GCEpilogueCallback* callback,
-                                      void* data) {
+void LocalHeap::AddGCEpilogueCallback(GCEpilogueCallback* callback, void* data,
+                                      GCType gc_type) {
   DCHECK(!IsParked());
-  std::pair<GCEpilogueCallback*, void*> callback_and_data(callback, data);
-  DCHECK_EQ(std::find(gc_epilogue_callbacks_.begin(),
-                      gc_epilogue_callbacks_.end(), callback_and_data),
-            gc_epilogue_callbacks_.end());
-  gc_epilogue_callbacks_.push_back(callback_and_data);
+  gc_epilogue_callbacks_.Add(callback, LocalIsolate::FromHeap(this), gc_type,
+                             data);
 }
 
 void LocalHeap::RemoveGCEpilogueCallback(GCEpilogueCallback* callback,
                                          void* data) {
   DCHECK(!IsParked());
-  std::pair<GCEpilogueCallback*, void*> callback_and_data(callback, data);
-  auto it = std::find(gc_epilogue_callbacks_.begin(),
-                      gc_epilogue_callbacks_.end(), callback_and_data);
-  *it = gc_epilogue_callbacks_.back();
-  gc_epilogue_callbacks_.pop_back();
+  gc_epilogue_callbacks_.Remove(callback, data);
 }
 
-void LocalHeap::InvokeGCEpilogueCallbacksInSafepoint() {
-  for (auto callback_and_data : gc_epilogue_callbacks_) {
-    callback_and_data.first(callback_and_data.second);
-  }
+void LocalHeap::InvokeGCEpilogueCallbacksInSafepoint(GCType gc_type,
+                                                     GCCallbackFlags flags) {
+  gc_epilogue_callbacks_.Invoke(gc_type, flags);
 }
 
 void LocalHeap::NotifyObjectSizeChange(

src/heap/local-heap.h

@@ -16,7 +16,7 @@
 #include "src/execution/isolate.h"
 #include "src/handles/persistent-handles.h"
 #include "src/heap/concurrent-allocator.h"
-
+#include "src/heap/gc-callbacks.h"
 namespace v8 {
 namespace internal {
 
@@ -37,7 +37,8 @@ class Safepoint;
 //            some time or for blocking operations like locking a mutex.
 class V8_EXPORT_PRIVATE LocalHeap {
  public:
-  using GCEpilogueCallback = void(void* data);
+  using GCEpilogueCallback = void(LocalIsolate*, GCType, GCCallbackFlags,
+                                  void*);
 
   explicit LocalHeap(
       Heap* heap, ThreadKind kind,
@@ -164,7 +165,11 @@ class V8_EXPORT_PRIVATE LocalHeap {
   // The callback is invoked on the main thread before any background thread
   // resumes. The callback must not allocate or make any other calls that
   // can trigger GC.
-  void AddGCEpilogueCallback(GCEpilogueCallback* callback, void* data);
+  void AddGCEpilogueCallback(GCEpilogueCallback* callback, void* data,
+                             GCType gc_type = static_cast<v8::GCType>(
+                                 GCType::kGCTypeMarkSweepCompact |
+                                 GCType::kGCTypeScavenge |
+                                 GCType::kGCTypeMinorMarkCompact));
   void RemoveGCEpilogueCallback(GCEpilogueCallback* callback, void* data);
 
   // Used to make SetupMainThread() available to unit tests.
@@ -292,7 +297,8 @@ class V8_EXPORT_PRIVATE LocalHeap {
 
   void EnsurePersistentHandles();
 
-  void InvokeGCEpilogueCallbacksInSafepoint();
+  void InvokeGCEpilogueCallbacksInSafepoint(GCType gc_type,
+                                            GCCallbackFlags flags);
 
   void SetUpMainThread();
   void SetUp();
@@ -315,7 +321,7 @@ class V8_EXPORT_PRIVATE LocalHeap {
   std::unique_ptr<PersistentHandles> persistent_handles_;
   std::unique_ptr<MarkingBarrier> marking_barrier_;
 
-  std::vector<std::pair<GCEpilogueCallback*, void*>> gc_epilogue_callbacks_;
+  GCCallbacks<LocalIsolate, DisallowGarbageCollection> gc_epilogue_callbacks_;
 
   std::unique_ptr<ConcurrentAllocator> old_space_allocator_;
   std::unique_ptr<ConcurrentAllocator> code_space_allocator_;

src/interpreter/bytecode-array-iterator.h

@@ -7,6 +7,7 @@
 
 #include <memory>
 
+#include "include/v8-callbacks.h"
 #include "src/common/globals.h"
 #include "src/handles/handles.h"
 #include "src/interpreter/bytecode-register.h"
@@ -159,7 +160,8 @@ class V8_EXPORT_PRIVATE BytecodeArrayIterator {
 
   std::ostream& PrintTo(std::ostream& os) const;
 
-  static void UpdatePointersCallback(void* iterator) {
+  static void UpdatePointersCallback(LocalIsolate*, GCType, GCCallbackFlags,
+                                     void* iterator) {
     reinterpret_cast<BytecodeArrayIterator*>(iterator)->UpdatePointers();
   }
 

src/json/json-parser.h

@@ -327,7 +327,8 @@ class JsonParser final {
 
   static const int kInitialSpecialStringLength = 32;
 
-  static void UpdatePointersCallback(void* parser) {
+  static void UpdatePointersCallback(LocalIsolate*, GCType, GCCallbackFlags,
+                                     void* parser) {
     reinterpret_cast<JsonParser<Char>*>(parser)->UpdatePointers();
   }
 

src/parsing/scanner-character-streams.cc

@@ -347,7 +347,8 @@ class RelocatingCharacterStream final
         UpdateBufferPointersCallback, this);
   }
 
-  static void UpdateBufferPointersCallback(void* stream) {
+  static void UpdateBufferPointersCallback(LocalIsolate*, GCType,
+                                           GCCallbackFlags, void* stream) {
     reinterpret_cast<RelocatingCharacterStream*>(stream)
         ->UpdateBufferPointers();
   }

test/unittests/heap/local-heap-unittest.cc

@@ -83,7 +83,7 @@ namespace {
 
 class GCEpilogue {
  public:
-  static void Callback(void* data) {
+  static void Callback(LocalIsolate*, GCType, GCCallbackFlags, void* data) {
     reinterpret_cast<GCEpilogue*>(data)->was_invoked_ = true;
   }