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

This is a reland of f25cb50a

Removed the problematic tests.
The problem with the test was that we try to pop from an empty segment.
GCC flags that as accessing beyond the array (i.e. index is uint16_t
equivalent of -1). Preceding the actual pop is a DCHECK that asserts
the segment isn't empty. In practice, since we have the DCHECK and
access to the segment is always via a Local, this shouldn't be a
problem.
Unfortunately, GCC flags the access regardless. The DCHECK goes through
a function pointer so GCC cannot determine that in our unittest the
DCHECK would crash if index is 0 and the access would not happen (The
indirection was added to allow for test DCHECK handlers that don't
crash, so we can't mark the function pointer as noreturn).

Drive-by: Segment::Pop and Segment::Push rely on the their Local
counterparts checking of emptiness/fullness, so we should always
access segments via Locals. Making the Segment ctor private.

Original change's description:
> 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/+/2401425
> Reviewed-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}

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

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,13 @@ 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;
+  static const uint16_t kSize = SegmentSize;
 
-  Segment() = default;
-  bool Push(EntryType entry);
-  bool Pop(EntryType* entry);
-
-  size_t Size() const { return index_; }
-  bool IsEmpty() const { return index_ == 0; }
-  bool IsFull() const { return index_ == kSize; }
-  void Clear() { index_ = 0; }
+  void Push(EntryType entry);
+  void Pop(EntryType* entry);
 
   template <typename Callback>
   void Update(Callback callback);
@@ -196,26 +207,36 @@ class Worklist<EntryType, SegmentSize>::Segment {
   void set_next(Segment* segment) { next_ = segment; }
 
  private:
+  Segment() : internal::SegmentBase(kSize) {}
+
   Segment* next_ = nullptr;
-  size_t index_ = 0;
   EntryType entries_[kSize];
+
+  friend class Worklist<EntryType, SegmentSize>::Local;
+
+  FRIEND_TEST(CppgcWorkListTest, SegmentCreate);
+  FRIEND_TEST(CppgcWorkListTest, SegmentPush);
+  FRIEND_TEST(CppgcWorkListTest, SegmentPushPop);
+  FRIEND_TEST(CppgcWorkListTest, SegmentIsEmpty);
+  FRIEND_TEST(CppgcWorkListTest, SegmentIsFull);
+  FRIEND_TEST(CppgcWorkListTest, SegmentClear);
+  FRIEND_TEST(CppgcWorkListTest, SegmentUpdateFalse);
+  FRIEND_TEST(CppgcWorkListTest, SegmentUpdate);
 };
 
-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 +248,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 +258,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 +291,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 +350,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 +368,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);
 }