Reland "cppgc, heap: Don't eagerly allocate worklist segments"

This is a reland of c99147c6

Original change's description:
> cppgc, heap: Don't eagerly allocate worklist segments
>
> Bug: chromium:1056170
> Change-Id: I75a6b5f52bfe8dd71abc086e5d1e060759ad7fc0
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2391254
> Commit-Queue: Omer Katz <omerkatz@chromium.org>
> Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#69778}

Bug: chromium:1056170
Change-Id: I4633da065976a6b2710d2f23b946fd2af0e65c83
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2401425Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Commit-Queue: Omer Katz <omerkatz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#69806}

BUILD.gn

@@ -4256,6 +4256,7 @@ v8_source_set("v8_cppgc_shared") {
   sources = [
     "src/heap/base/stack.cc",
     "src/heap/base/stack.h",
+    "src/heap/base/worklist.cc",
     "src/heap/base/worklist.h",
   ]
 

src/heap/base/worklist.cc

+// 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.
+
+#include "src/heap/base/worklist.h"
+
+namespace heap {
+namespace base {
+namespace internal {
+SegmentBase SegmentBase::kSentinelSegment(0);
+}  // namespace internal
+}  // namespace base
+}  // namespace heap

src/heap/base/worklist.h

@@ -16,11 +16,29 @@
 namespace heap {
 namespace base {
 
+namespace internal {
+class V8_EXPORT_PRIVATE SegmentBase {
+ public:
+  static SegmentBase kSentinelSegment;
+
+  explicit SegmentBase(uint16_t capacity) : capacity_(capacity) {}
+
+  size_t Size() const { return index_; }
+  bool IsEmpty() const { return index_ == 0; }
+  bool IsFull() const { return index_ == capacity_; }
+  void Clear() { index_ = 0; }
+
+ protected:
+  const uint16_t capacity_;
+  uint16_t index_ = 0;
+};
+}  // namespace internal
+
 // A global marking worklist that is similar the existing Worklist
 // but does not reserve space and keep track of the local segments.
 // Eventually this will replace Worklist after all its current uses
 // are migrated.
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 class Worklist {
  public:
   static const int kSegmentSize = SegmentSize;
@@ -61,34 +79,33 @@ class Worklist {
   std::atomic<size_t> size_{0};
 };
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Push(Segment* segment) {
+  DCHECK(!segment->IsEmpty());
   v8::base::MutexGuard guard(&lock_);
   segment->set_next(top_);
   set_top(segment);
   size_.fetch_add(1, std::memory_order_relaxed);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::Pop(Segment** segment) {
   v8::base::MutexGuard guard(&lock_);
-  if (top_ != nullptr) {
-    DCHECK_LT(0U, size_);
-    size_.fetch_sub(1, std::memory_order_relaxed);
-    *segment = top_;
-    set_top(top_->next());
-    return true;
-  }
-  return false;
+  if (top_ == nullptr) return false;
+  DCHECK_LT(0U, size_);
+  size_.fetch_sub(1, std::memory_order_relaxed);
+  *segment = top_;
+  set_top(top_->next());
+  return true;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::IsEmpty() {
   return v8::base::AsAtomicPtr(&top_)->load(std::memory_order_relaxed) ==
          nullptr;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 size_t Worklist<EntryType, SegmentSize>::Size() {
   // It is safe to read |size_| without a lock since this variable is
   // atomic, keeping in mind that threads may not immediately see the new
@@ -96,7 +113,7 @@ size_t Worklist<EntryType, SegmentSize>::Size() {
   return size_.load(std::memory_order_relaxed);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Clear() {
   v8::base::MutexGuard guard(&lock_);
   size_.store(0, std::memory_order_relaxed);
@@ -109,7 +126,7 @@ void Worklist<EntryType, SegmentSize>::Clear() {
   set_top(nullptr);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 template <typename Callback>
 void Worklist<EntryType, SegmentSize>::Update(Callback callback) {
   v8::base::MutexGuard guard(&lock_);
@@ -137,7 +154,7 @@ void Worklist<EntryType, SegmentSize>::Update(Callback callback) {
   size_.fetch_sub(num_deleted, std::memory_order_relaxed);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 template <typename Callback>
 void Worklist<EntryType, SegmentSize>::Iterate(Callback callback) {
   v8::base::MutexGuard guard(&lock_);
@@ -146,7 +163,7 @@ void Worklist<EntryType, SegmentSize>::Iterate(Callback callback) {
   }
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Merge(
     Worklist<EntryType, SegmentSize>* other) {
   Segment* top = nullptr;
@@ -173,19 +190,14 @@ void Worklist<EntryType, SegmentSize>::Merge(
   }
 }
 
-template <typename EntryType, int SegmentSize>
-class Worklist<EntryType, SegmentSize>::Segment {
+template <typename EntryType, uint16_t SegmentSize>
+class Worklist<EntryType, SegmentSize>::Segment : public internal::SegmentBase {
  public:
-  static const size_t kSize = SegmentSize;
-
-  Segment() = default;
-  bool Push(EntryType entry);
-  bool Pop(EntryType* entry);
+  static const uint16_t kSize = SegmentSize;
 
-  size_t Size() const { return index_; }
-  bool IsEmpty() const { return index_ == 0; }
-  bool IsFull() const { return index_ == kSize; }
-  void Clear() { index_ = 0; }
+  Segment() : internal::SegmentBase(kSize) {}
+  void Push(EntryType entry);
+  void Pop(EntryType* entry);
 
   template <typename Callback>
   void Update(Callback callback);
@@ -197,25 +209,22 @@ class Worklist<EntryType, SegmentSize>::Segment {
 
  private:
   Segment* next_ = nullptr;
-  size_t index_ = 0;
   EntryType entries_[kSize];
 };
 
-template <typename EntryType, int SegmentSize>
-bool Worklist<EntryType, SegmentSize>::Segment::Push(EntryType entry) {
-  if (IsFull()) return false;
+template <typename EntryType, uint16_t SegmentSize>
+void Worklist<EntryType, SegmentSize>::Segment::Push(EntryType entry) {
+  DCHECK(!IsFull());
   entries_[index_++] = entry;
-  return true;
 }
 
-template <typename EntryType, int SegmentSize>
-bool Worklist<EntryType, SegmentSize>::Segment::Pop(EntryType* entry) {
-  if (IsEmpty()) return false;
+template <typename EntryType, uint16_t SegmentSize>
+void Worklist<EntryType, SegmentSize>::Segment::Pop(EntryType* entry) {
+  DCHECK(!IsEmpty());
   *entry = entries_[--index_];
-  return true;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 template <typename Callback>
 void Worklist<EntryType, SegmentSize>::Segment::Update(Callback callback) {
   size_t new_index = 0;
@@ -227,7 +236,7 @@ void Worklist<EntryType, SegmentSize>::Segment::Update(Callback callback) {
   index_ = new_index;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 template <typename Callback>
 void Worklist<EntryType, SegmentSize>::Segment::Iterate(
     Callback callback) const {
@@ -237,7 +246,7 @@ void Worklist<EntryType, SegmentSize>::Segment::Iterate(
 }
 
 // A thread-local view of the marking worklist.
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 class Worklist<EntryType, SegmentSize>::Local {
  public:
   using ItemType = EntryType;
@@ -270,32 +279,55 @@ class Worklist<EntryType, SegmentSize>::Local {
   void PublishPushSegment();
   void PublishPopSegment();
   bool StealPopSegment();
+
   Segment* NewSegment() const {
     // Bottleneck for filtering in crash dumps.
     return new Segment();
   }
+  void DeleteSegment(internal::SegmentBase* segment) const {
+    if (segment == &internal::SegmentBase::kSentinelSegment) return;
+    delete static_cast<Segment*>(segment);
+  }
+
+  inline Segment* push_segment() {
+    DCHECK_NE(&internal::SegmentBase::kSentinelSegment, push_segment_);
+    return static_cast<Segment*>(push_segment_);
+  }
+  inline const Segment* push_segment() const {
+    DCHECK_NE(&internal::SegmentBase::kSentinelSegment, push_segment_);
+    return static_cast<const Segment*>(push_segment_);
+  }
+
+  inline Segment* pop_segment() {
+    DCHECK_NE(&internal::SegmentBase::kSentinelSegment, pop_segment_);
+    return static_cast<Segment*>(pop_segment_);
+  }
+  inline const Segment* pop_segment() const {
+    DCHECK_NE(&internal::SegmentBase::kSentinelSegment, pop_segment_);
+    return static_cast<const Segment*>(pop_segment_);
+  }
 
   Worklist<EntryType, SegmentSize>* worklist_ = nullptr;
-  Segment* push_segment_ = nullptr;
-  Segment* pop_segment_ = nullptr;
+  internal::SegmentBase* push_segment_ = nullptr;
+  internal::SegmentBase* pop_segment_ = nullptr;
 };
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 Worklist<EntryType, SegmentSize>::Local::Local(
     Worklist<EntryType, SegmentSize>* worklist)
     : worklist_(worklist),
-      push_segment_(NewSegment()),
-      pop_segment_(NewSegment()) {}
+      push_segment_(&internal::SegmentBase::kSentinelSegment),
+      pop_segment_(&internal::SegmentBase::kSentinelSegment) {}
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 Worklist<EntryType, SegmentSize>::Local::~Local() {
   CHECK_IMPLIES(push_segment_, push_segment_->IsEmpty());
   CHECK_IMPLIES(pop_segment_, pop_segment_->IsEmpty());
-  delete push_segment_;
-  delete pop_segment_;
+  DeleteSegment(push_segment_);
+  DeleteSegment(pop_segment_);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 Worklist<EntryType, SegmentSize>::Local::Local(
     Worklist<EntryType, SegmentSize>::Local&& other) V8_NOEXCEPT {
   worklist_ = other.worklist_;
@@ -306,7 +338,7 @@ Worklist<EntryType, SegmentSize>::Local::Local(
   other.pop_segment_ = nullptr;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 typename Worklist<EntryType, SegmentSize>::Local&
 Worklist<EntryType, SegmentSize>::Local::operator=(
     Worklist<EntryType, SegmentSize>::Local&& other) V8_NOEXCEPT {
@@ -324,81 +356,75 @@ Worklist<EntryType, SegmentSize>::Local::operator=(
   return *this;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Local::Push(EntryType entry) {
-  if (V8_UNLIKELY(!push_segment_->Push(entry))) {
+  if (V8_UNLIKELY(push_segment_->IsFull())) {
     PublishPushSegment();
-    bool success = push_segment_->Push(entry);
-    USE(success);
-    DCHECK(success);
   }
+  push_segment()->Push(entry);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::Local::Pop(EntryType* entry) {
-  if (!pop_segment_->Pop(entry)) {
+  if (pop_segment_->IsEmpty()) {
     if (!push_segment_->IsEmpty()) {
       std::swap(push_segment_, pop_segment_);
     } else if (!StealPopSegment()) {
       return false;
     }
-    bool success = pop_segment_->Pop(entry);
-    USE(success);
-    DCHECK(success);
   }
+  pop_segment()->Pop(entry);
   return true;
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::Local::IsLocalAndGlobalEmpty() const {
   return IsLocalEmpty() && IsGlobalEmpty();
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::Local::IsLocalEmpty() const {
   return push_segment_->IsEmpty() && pop_segment_->IsEmpty();
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::Local::IsGlobalEmpty() const {
   return worklist_->IsEmpty();
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Local::Publish() {
-  if (!push_segment_->IsEmpty()) {
-    PublishPushSegment();
-  }
-  if (!pop_segment_->IsEmpty()) {
-    PublishPopSegment();
-  }
+  if (!push_segment_->IsEmpty()) PublishPushSegment();
+  if (!pop_segment_->IsEmpty()) PublishPopSegment();
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Local::Merge(
     Worklist<EntryType, SegmentSize>::Local* other) {
   other->Publish();
   worklist_->Merge(other->worklist_);
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Local::PublishPushSegment() {
-  worklist_->Push(push_segment_);
+  if (push_segment_ != &internal::SegmentBase::kSentinelSegment)
+    worklist_->Push(push_segment());
   push_segment_ = NewSegment();
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 void Worklist<EntryType, SegmentSize>::Local::PublishPopSegment() {
-  worklist_->Push(pop_segment_);
+  if (pop_segment_ != &internal::SegmentBase::kSentinelSegment)
+    worklist_->Push(pop_segment());
   pop_segment_ = NewSegment();
 }
 
-template <typename EntryType, int SegmentSize>
+template <typename EntryType, uint16_t SegmentSize>
 bool Worklist<EntryType, SegmentSize>::Local::StealPopSegment() {
   if (worklist_->IsEmpty()) return false;
   Segment* new_segment = nullptr;
   if (worklist_->Pop(&new_segment)) {
-    delete pop_segment_;
+    DeleteSegment(pop_segment_);
     pop_segment_ = new_segment;
     return true;
   }

test/unittests/BUILD.gn

@@ -69,6 +69,7 @@ v8_executable("cppgc_unittests") {
 
   deps = [
     ":cppgc_unittests_sources",
+    ":v8_cppgc_shared_unittests_sources",
     "../..:cppgc_for_testing",
     "//testing/gmock",
     "//testing/gtest",
@@ -136,6 +137,7 @@ v8_executable("unittests") {
   deps = [
     ":cppgc_unittests_sources",
     ":unittests_sources",
+    ":v8_cppgc_shared_unittests_sources",
     "../..:v8_for_testing",
     "../..:v8_libbase",
     "../..:v8_libplatform",

test/unittests/heap/base/worklist-unittest.cc

@@ -4,7 +4,7 @@
 
 #include "src/heap/base/worklist.h"
 
-#include "test/unittests/heap/cppgc/tests.h"
+#include "testing/gtest/include/gtest/gtest.h"
 
 namespace heap {
 namespace base {
@@ -23,17 +23,17 @@ TEST(CppgcWorkListTest, SegmentCreate) {
 TEST(CppgcWorkListTest, SegmentPush) {
   TestWorklist::Segment segment;
   EXPECT_EQ(0u, segment.Size());
-  EXPECT_TRUE(segment.Push(nullptr));
+  segment.Push(nullptr);
   EXPECT_EQ(1u, segment.Size());
 }
 
 TEST(CppgcWorkListTest, SegmentPushPop) {
   TestWorklist::Segment segment;
-  EXPECT_TRUE(segment.Push(nullptr));
+  segment.Push(nullptr);
   EXPECT_EQ(1u, segment.Size());
   SomeObject dummy;
   SomeObject* object = &dummy;
-  EXPECT_TRUE(segment.Pop(&object));
+  segment.Pop(&object);
   EXPECT_EQ(0u, segment.Size());
   EXPECT_EQ(nullptr, object);
 }
@@ -41,7 +41,7 @@ TEST(CppgcWorkListTest, SegmentPushPop) {
 TEST(CppgcWorkListTest, SegmentIsEmpty) {
   TestWorklist::Segment segment;
   EXPECT_TRUE(segment.IsEmpty());
-  EXPECT_TRUE(segment.Push(nullptr));
+  segment.Push(nullptr);
   EXPECT_FALSE(segment.IsEmpty());
 }
 
@@ -49,44 +49,27 @@ TEST(CppgcWorkListTest, SegmentIsFull) {
   TestWorklist::Segment segment;
   EXPECT_FALSE(segment.IsFull());
   for (size_t i = 0; i < TestWorklist::Segment::kSize; i++) {
-    EXPECT_TRUE(segment.Push(nullptr));
+    segment.Push(nullptr);
   }
   EXPECT_TRUE(segment.IsFull());
 }
 
 TEST(CppgcWorkListTest, SegmentClear) {
   TestWorklist::Segment segment;
-  EXPECT_TRUE(segment.Push(nullptr));
+  segment.Push(nullptr);
   EXPECT_FALSE(segment.IsEmpty());
   segment.Clear();
   EXPECT_TRUE(segment.IsEmpty());
   for (size_t i = 0; i < TestWorklist::Segment::kSize; i++) {
-    EXPECT_TRUE(segment.Push(nullptr));
+    segment.Push(nullptr);
   }
 }
 
-TEST(CppgcWorkListTest, SegmentFullPushFails) {
-  TestWorklist::Segment segment;
-  EXPECT_FALSE(segment.IsFull());
-  for (size_t i = 0; i < TestWorklist::Segment::kSize; i++) {
-    EXPECT_TRUE(segment.Push(nullptr));
-  }
-  EXPECT_TRUE(segment.IsFull());
-  EXPECT_FALSE(segment.Push(nullptr));
-}
-
-TEST(CppgcWorkListTest, SegmentEmptyPopFails) {
-  TestWorklist::Segment segment;
-  EXPECT_TRUE(segment.IsEmpty());
-  SomeObject* object;
-  EXPECT_FALSE(segment.Pop(&object));
-}
-
 TEST(CppgcWorkListTest, SegmentUpdateFalse) {
   TestWorklist::Segment segment;
   SomeObject* object;
   object = reinterpret_cast<SomeObject*>(&object);
-  EXPECT_TRUE(segment.Push(object));
+  segment.Push(object);
   segment.Update([](SomeObject* object, SomeObject** out) { return false; });
   EXPECT_TRUE(segment.IsEmpty());
 }
@@ -97,13 +80,13 @@ TEST(CppgcWorkListTest, SegmentUpdate) {
   objectA = reinterpret_cast<SomeObject*>(&objectA);
   SomeObject* objectB;
   objectB = reinterpret_cast<SomeObject*>(&objectB);
-  EXPECT_TRUE(segment.Push(objectA));
+  segment.Push(objectA);
   segment.Update([objectB](SomeObject* object, SomeObject** out) {
     *out = objectB;
     return true;
   });
   SomeObject* object;
-  EXPECT_TRUE(segment.Pop(&object));
+  segment.Pop(&object);
   EXPECT_EQ(object, objectB);
 }
 
@@ -324,5 +307,26 @@ TEST(CppgcWorkListTest, MergeGlobalPool) {
   EXPECT_TRUE(worklist2.IsEmpty());
 }
 
+#ifdef DEBUG
+
+TEST(CppgcWorkListDeathTest, DiesOnPushToFullSegment) {
+  TestWorklist::Segment segment;
+  EXPECT_FALSE(segment.IsFull());
+  for (size_t i = 0; i < TestWorklist::Segment::kSize; i++) {
+    segment.Push(nullptr);
+  }
+  EXPECT_TRUE(segment.IsFull());
+  EXPECT_DEATH_IF_SUPPORTED(segment.Push(nullptr), "");
+}
+
+TEST(CppgcWorkListDeathTest, DiesOnPopFromEmptySegment) {
+  TestWorklist::Segment segment;
+  EXPECT_TRUE(segment.IsEmpty());
+  SomeObject* object;
+  EXPECT_DEATH_IF_SUPPORTED(segment.Pop(&object), "");
+}
+
+#endif  // DEBUG
+
 }  // namespace base
 }  // namespace heap