[heap] Make Heap::UnregisterStrongRoots work in constant time

Heap::UnregisterStrongRoots needs to iterate the list of all strong
roots to delete the given slot. This CL changes
Heap::RegisterStrongRoots to return the pointer to the linked list node.
Heap::UnregisterStrongRoots gets the node as argument and can directly
delete it in constant time.

The CL also introduces Heap::UpdateStrongRoots which can update a
node without locking the mutex.

Bug: v8:10315
Change-Id: I2c021517c010a659821f8c10de758bb49b28449f
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2364511Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Dominik Inführ <dinfuehr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69499}

src/deoptimizer/deoptimizer.cc

@@ -181,12 +181,12 @@ class FrameWriter {
 DeoptimizerData::DeoptimizerData(Heap* heap) : heap_(heap), current_(nullptr) {
   Code* start = &deopt_entry_code_[0];
   Code* end = &deopt_entry_code_[DeoptimizerData::kLastDeoptimizeKind + 1];
-  heap_->RegisterStrongRoots(FullObjectSlot(start), FullObjectSlot(end));
+  strong_roots_entry_ =
+      heap_->RegisterStrongRoots(FullObjectSlot(start), FullObjectSlot(end));
 }
 
 DeoptimizerData::~DeoptimizerData() {
-  Code* start = &deopt_entry_code_[0];
-  heap_->UnregisterStrongRoots(FullObjectSlot(start));
+  heap_->UnregisterStrongRoots(strong_roots_entry_);
 }
 
 Code DeoptimizerData::deopt_entry_code(DeoptimizeKind kind) {

src/deoptimizer/deoptimizer.h

@@ -17,6 +17,7 @@
 #include "src/diagnostics/code-tracer.h"
 #include "src/execution/frame-constants.h"
 #include "src/execution/isolate.h"
+#include "src/heap/heap.h"
 #include "src/objects/feedback-vector.h"
 #include "src/objects/js-function.h"
 #include "src/objects/shared-function-info.h"
@@ -35,6 +36,7 @@ class TranslatedFrame;
 class TranslatedState;
 class RegisterValues;
 class MacroAssembler;
+class StrongRootsEntry;
 
 enum class BuiltinContinuationMode;
 
@@ -821,6 +823,7 @@ class DeoptimizerData {
   void set_deopt_entry_code(DeoptimizeKind kind, Code code);
 
   Deoptimizer* current_;
+  StrongRootsEntry* strong_roots_entry_;
 
   friend class Deoptimizer;
 

src/heap/heap.cc

@@ -170,12 +170,6 @@ bool Heap::GCCallbackTuple::operator==(
 Heap::GCCallbackTuple& Heap::GCCallbackTuple::operator=(
     const Heap::GCCallbackTuple& other) V8_NOEXCEPT = default;
 
-struct Heap::StrongRootsList {
-  FullObjectSlot start;
-  FullObjectSlot end;
-  StrongRootsList* next;
-};
-
 class ScavengeTaskObserver : public AllocationObserver {
  public:
   ScavengeTaskObserver(Heap* heap, intptr_t step_size)
@@ -4573,8 +4567,10 @@ void Heap::IterateRoots(RootVisitor* v, base::EnumSet<SkipRoot> options) {
 
     // Iterate over other strong roots (currently only identity maps and
     // deoptimization entries).
-    for (StrongRootsList* list = strong_roots_list_; list; list = list->next) {
-      v->VisitRootPointers(Root::kStrongRoots, nullptr, list->start, list->end);
+    for (StrongRootsEntry* current = strong_roots_head_; current;
+         current = current->next) {
+      v->VisitRootPointers(Root::kStrongRoots, nullptr, current->start,
+                           current->end);
     }
     v->Synchronize(VisitorSynchronization::kStrongRoots);
 
@@ -5605,12 +5601,13 @@ void Heap::TearDown() {
 
   memory_allocator()->TearDown();
 
-  StrongRootsList* next = nullptr;
-  for (StrongRootsList* list = strong_roots_list_; list; list = next) {
-    next = list->next;
-    delete list;
+  StrongRootsEntry* next = nullptr;
+  for (StrongRootsEntry* current = strong_roots_head_; current;
+       current = next) {
+    next = current->next;
+    delete current;
   }
-  strong_roots_list_ = nullptr;
+  strong_roots_head_ = nullptr;
 
   memory_allocator_.reset();
 }
@@ -6184,33 +6181,46 @@ size_t Heap::OldArrayBufferBytes() {
   return array_buffer_sweeper()->OldBytes();
 }
 
-void Heap::RegisterStrongRoots(FullObjectSlot start, FullObjectSlot end) {
+StrongRootsEntry* Heap::RegisterStrongRoots(FullObjectSlot start,
+                                            FullObjectSlot end) {
   base::MutexGuard guard(&strong_roots_mutex_);
-  StrongRootsList* list = new StrongRootsList();
-  list->next = strong_roots_list_;
-  list->start = start;
-  list->end = end;
-  strong_roots_list_ = list;
+
+  StrongRootsEntry* entry = new StrongRootsEntry();
+  entry->start = start;
+  entry->end = end;
+  entry->prev = nullptr;
+  entry->next = strong_roots_head_;
+
+  if (strong_roots_head_) {
+    DCHECK_NULL(strong_roots_head_->prev);
+    strong_roots_head_->prev = entry;
+  }
+  strong_roots_head_ = entry;
+
+  return entry;
+}
+
+void Heap::UpdateStrongRoots(StrongRootsEntry* entry, FullObjectSlot start,
+                             FullObjectSlot end) {
+  entry->start = start;
+  entry->end = end;
 }
 
-void Heap::UnregisterStrongRoots(FullObjectSlot start) {
+void Heap::UnregisterStrongRoots(StrongRootsEntry* entry) {
   base::MutexGuard guard(&strong_roots_mutex_);
-  StrongRootsList* prev = nullptr;
-  StrongRootsList* list = strong_roots_list_;
-  while (list != nullptr) {
-    StrongRootsList* next = list->next;
-    if (list->start == start) {
-      if (prev) {
-        prev->next = next;
-      } else {
-        strong_roots_list_ = next;
-      }
-      delete list;
-    } else {
-      prev = list;
-    }
-    list = next;
+
+  StrongRootsEntry* prev = entry->prev;
+  StrongRootsEntry* next = entry->next;
+
+  if (prev) prev->next = next;
+  if (next) next->prev = prev;
+
+  if (strong_roots_head_ == entry) {
+    DCHECK_NULL(prev);
+    strong_roots_head_ = next;
   }
+
+  delete entry;
 }
 
 void Heap::SetBuiltinsConstantsTable(FixedArray cache) {

src/heap/heap.h

@@ -177,6 +177,18 @@ enum class SkipRoot {
   kWeak
 };
 
+class StrongRootsEntry {
+  StrongRootsEntry() = default;
+
+  FullObjectSlot start;
+  FullObjectSlot end;
+
+  StrongRootsEntry* prev;
+  StrongRootsEntry* next;
+
+  friend class Heap;
+};
+
 class AllocationResult {
  public:
   static inline AllocationResult Retry(AllocationSpace space = NEW_SPACE) {
@@ -866,8 +878,11 @@ class Heap {
   V8_INLINE void SetMessageListeners(TemplateList value);
   V8_INLINE void SetPendingOptimizeForTestBytecode(Object bytecode);
 
-  void RegisterStrongRoots(FullObjectSlot start, FullObjectSlot end);
-  void UnregisterStrongRoots(FullObjectSlot start);
+  StrongRootsEntry* RegisterStrongRoots(FullObjectSlot start,
+                                        FullObjectSlot end);
+  void UnregisterStrongRoots(StrongRootsEntry* entry);
+  void UpdateStrongRoots(StrongRootsEntry* entry, FullObjectSlot start,
+                         FullObjectSlot end);
 
   void SetBuiltinsConstantsTable(FixedArray cache);
   void SetDetachedContexts(WeakArrayList detached_contexts);
@@ -1571,8 +1586,6 @@ class Heap {
     void Wait();
   };
 
-  struct StrongRootsList;
-
   struct StringTypeTable {
     InstanceType type;
     int size;
@@ -2199,7 +2212,8 @@ class Heap {
   std::unique_ptr<AllocationObserver> stress_concurrent_allocation_observer_;
   std::unique_ptr<LocalEmbedderHeapTracer> local_embedder_heap_tracer_;
   std::unique_ptr<MarkingBarrier> marking_barrier_;
-  StrongRootsList* strong_roots_list_ = nullptr;
+
+  StrongRootsEntry* strong_roots_head_ = nullptr;
   base::Mutex strong_roots_mutex_;
 
   bool need_to_remove_stress_concurrent_allocation_observer_ = false;

src/utils/identity-map.cc

@@ -5,6 +5,7 @@
 #include "src/utils/identity-map.h"
 
 #include "src/base/functional.h"
+#include "src/base/logging.h"
 #include "src/heap/heap.h"
 #include "src/roots/roots-inl.h"
 
@@ -23,10 +24,12 @@ IdentityMapBase::~IdentityMapBase() {
 void IdentityMapBase::Clear() {
   if (keys_) {
     DCHECK(!is_iterable());
-    heap_->UnregisterStrongRoots(FullObjectSlot(keys_));
+    DCHECK_NOT_NULL(strong_roots_entry_);
+    heap_->UnregisterStrongRoots(strong_roots_entry_);
     DeletePointerArray(reinterpret_cast<void**>(keys_), capacity_);
     DeletePointerArray(values_, capacity_);
     keys_ = nullptr;
+    strong_roots_entry_ = nullptr;
     values_ = nullptr;
     size_ = 0;
     capacity_ = 0;
@@ -164,8 +167,8 @@ IdentityMapBase::RawEntry IdentityMapBase::GetEntry(Address key) {
     values_ = NewPointerArray(capacity_);
     memset(values_, 0, sizeof(void*) * capacity_);
 
-    heap_->RegisterStrongRoots(FullObjectSlot(keys_),
-                               FullObjectSlot(keys_ + capacity_));
+    strong_roots_entry_ = heap_->RegisterStrongRoots(
+        FullObjectSlot(keys_), FullObjectSlot(keys_ + capacity_));
   }
   int index = LookupOrInsert(key);
   return &values_[index];
@@ -284,9 +287,9 @@ void IdentityMapBase::Resize(int new_capacity) {
   }
 
   // Unregister old keys and register new keys.
-  heap_->UnregisterStrongRoots(FullObjectSlot(old_keys));
-  heap_->RegisterStrongRoots(FullObjectSlot(keys_),
-                             FullObjectSlot(keys_ + capacity_));
+  DCHECK_NOT_NULL(strong_roots_entry_);
+  heap_->UpdateStrongRoots(strong_roots_entry_, FullObjectSlot(keys_),
+                           FullObjectSlot(keys_ + capacity_));
 
   // Delete old storage;
   DeletePointerArray(reinterpret_cast<void**>(old_keys), old_capacity);

src/utils/identity-map.h

@@ -14,6 +14,7 @@ namespace internal {
 
 // Forward declarations.
 class Heap;
+class StrongRootsEntry;
 
 // Base class of identity maps contains shared code for all template
 // instantions.
@@ -38,6 +39,7 @@ class V8_EXPORT_PRIVATE IdentityMapBase {
         capacity_(0),
         mask_(0),
         keys_(nullptr),
+        strong_roots_entry_(nullptr),
         values_(nullptr),
         is_iterable_(false) {}
   virtual ~IdentityMapBase();
@@ -76,6 +78,7 @@ class V8_EXPORT_PRIVATE IdentityMapBase {
   int capacity_;
   int mask_;
   Address* keys_;
+  StrongRootsEntry* strong_roots_entry_;
   void** values_;
   bool is_iterable_;