heap: ArrayBufferSweeper refactoring

The refactoring is triggered by https://crrev.com/c/3121905 where we
noticed that a bunch of tricky counter paths could be simplified,
making reasoning about corectness easier.

In this CL:
1. Use uniqe_ptr instead of Optional to allow moving SweepingJob away
   from the header file.
2. sweeping_in_progress_ is replaced with simply checking for a job.
3. freed_bytes_ are moved to the job and the dependency is reversed,
   avoiding the inside-out (Job->Sweeper) dependency completely.
4. Merge() and counter updates are merged into a Finalize() method.
5. FinishIfDone() allows for conditional finization.
6. young_bytes_ and old_bytes_ are removed as they were always updated
   when the corresponding bytes in the ArrayBufferList was updated.

Change-Id: I56e5b04087166ce03d3a9195ac48359122a84c73
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/3124776Reviewed-by: Dominik Inführ <dinfuehr@chromium.org>
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Cr-Commit-Position: refs/heads/main@{#76795}

src/heap/array-buffer-sweeper.h

@@ -5,6 +5,9 @@
 #ifndef V8_HEAP_ARRAY_BUFFER_SWEEPER_H_
 #define V8_HEAP_ARRAY_BUFFER_SWEEPER_H_
 
+#include <memory>
+
+#include "src/base/logging.h"
 #include "src/base/platform/mutex.h"
 #include "src/objects/js-array-buffer.h"
 #include "src/tasks/cancelable-task.h"
@@ -17,47 +20,38 @@ class Heap;
 
 // Singly linked-list of ArrayBufferExtensions that stores head and tail of the
 // list to allow for concatenation of lists.
-struct ArrayBufferList {
-  ArrayBufferList() : head_(nullptr), tail_(nullptr), bytes_(0) {}
-
-  ArrayBufferExtension* head_;
-  ArrayBufferExtension* tail_;
-  size_t bytes_;
-
-  bool IsEmpty() {
-    DCHECK_IMPLIES(head_, tail_);
-    return head_ == nullptr;
-  }
-
-  size_t Bytes() { return bytes_; }
-  size_t BytesSlow();
-
-  void Reset() {
-    head_ = tail_ = nullptr;
-    bytes_ = 0;
-  }
+struct ArrayBufferList final {
+  bool IsEmpty() const;
+  size_t ApproximateBytes() const { return bytes_; }
+  size_t BytesSlow() const;
 
   void Append(ArrayBufferExtension* extension);
   void Append(ArrayBufferList* list);
 
-  V8_EXPORT_PRIVATE bool Contains(ArrayBufferExtension* extension);
+  V8_EXPORT_PRIVATE bool ContainsSlow(ArrayBufferExtension* extension) const;
+
+ private:
+  ArrayBufferExtension* head_ = nullptr;
+  ArrayBufferExtension* tail_ = nullptr;
+  // Bytes are approximate as they may be subtracted eagerly, while the
+  // `ArrayBufferExtension` is still in the list. The extension will only be
+  // dropped on next sweep.
+  size_t bytes_ = 0;
+
+  friend class ArrayBufferSweeper;
 };
 
 // The ArrayBufferSweeper iterates and deletes ArrayBufferExtensions
 // concurrently to the application.
-class ArrayBufferSweeper {
+class ArrayBufferSweeper final {
  public:
-  explicit ArrayBufferSweeper(Heap* heap)
-      : heap_(heap),
-        sweeping_in_progress_(false),
-        freed_bytes_(0),
-        young_bytes_(0),
-        old_bytes_(0) {}
-  ~ArrayBufferSweeper() { ReleaseAll(); }
+  enum class SweepingType { kYoung, kFull };
 
+  explicit ArrayBufferSweeper(Heap* heap);
+  ~ArrayBufferSweeper();
+
+  void RequestSweep(SweepingType sweeping_type);
   void EnsureFinished();
-  void RequestSweepYoung();
-  void RequestSweepFull();
 
   // Track the given ArrayBufferExtension for the given JSArrayBuffer.
   void Append(JSArrayBuffer object, ArrayBufferExtension* extension);
@@ -65,70 +59,40 @@ class ArrayBufferSweeper {
   // Detaches an ArrayBufferExtension from a JSArrayBuffer.
   void Detach(JSArrayBuffer object, ArrayBufferExtension* extension);
 
-  ArrayBufferList young() { return young_; }
-  ArrayBufferList old() { return old_; }
+  const ArrayBufferList& young() const { return young_; }
+  const ArrayBufferList& old() const { return old_; }
 
-  size_t YoungBytes();
-  size_t OldBytes();
+  // Bytes accounted in the young generation. Rebuilt during sweeping.
+  size_t YoungBytes() const { return young().ApproximateBytes(); }
+  // Bytes accounted in the old generation. Rebuilt during sweeping.
+  size_t OldBytes() const { return old().ApproximateBytes(); }
 
  private:
-  enum class SweepingScope { kYoung, kFull };
+  struct SweepingJob;
 
   enum class SweepingState { kInProgress, kDone };
 
-  struct SweepingJob {
-    ArrayBufferSweeper* sweeper_;
-    CancelableTaskManager::Id id_;
-    std::atomic<SweepingState> state_;
-    ArrayBufferList young_;
-    ArrayBufferList old_;
-    SweepingScope scope_;
-
-    SweepingJob(ArrayBufferSweeper* sweeper, ArrayBufferList young,
-                ArrayBufferList old, SweepingScope scope)
-        : sweeper_(sweeper),
-          id_(0),
-          state_(SweepingState::kInProgress),
-          young_(young),
-          old_(old),
-          scope_(scope) {}
-
-    void Sweep();
-    void SweepYoung();
-    void SweepFull();
-    ArrayBufferList SweepListFull(ArrayBufferList* list);
-  };
-
-  base::Optional<SweepingJob> job_;
-
-  void Merge();
-  void MergeBackExtensionsWhenSwept();
-
-  void UpdateCountersForConcurrentlySweptExtensions();
+  bool sweeping_in_progress() const { return job_.get(); }
+
+  // Finishes sweeping if it is already done.
+  void FinishIfDone();
+
+  // Increments external memory counters outside of ArrayBufferSweeper.
+  // Increment may trigger GC.
   void IncrementExternalMemoryCounters(size_t bytes);
   void DecrementExternalMemoryCounters(size_t bytes);
-  void IncrementFreedBytes(size_t bytes);
 
-  void RequestSweep(SweepingScope sweeping_task);
-  void Prepare(SweepingScope sweeping_task);
+  void Prepare(SweepingType type);
+  void Finalize();
 
-  ArrayBufferList SweepYoungGen();
-  void SweepOldGen(ArrayBufferExtension* extension);
-
-  void ReleaseAll();
   void ReleaseAll(ArrayBufferList* extension);
 
   Heap* const heap_;
-  bool sweeping_in_progress_;
+  std::unique_ptr<SweepingJob> job_;
   base::Mutex sweeping_mutex_;
   base::ConditionVariable job_finished_;
-  std::atomic<size_t> freed_bytes_;
-
   ArrayBufferList young_;
   ArrayBufferList old_;
-
-  size_t young_bytes_;
-  size_t old_bytes_;
 };
 
 }  // namespace internal

src/heap/heap.cc

@@ -4143,11 +4143,13 @@ void Heap::RemoveNearHeapLimitCallback(v8::NearHeapLimitCallback callback,
 
 void Heap::AppendArrayBufferExtension(JSArrayBuffer object,
                                       ArrayBufferExtension* extension) {
+  // ArrayBufferSweeper is managing all counters and updating Heap counters.
   array_buffer_sweeper_->Append(object, extension);
 }
 
 void Heap::DetachArrayBufferExtension(JSArrayBuffer object,
                                       ArrayBufferExtension* extension) {
+  // ArrayBufferSweeper is managing all counters and updating Heap counters.
   return array_buffer_sweeper_->Detach(object, extension);
 }
 

src/heap/mark-compact.cc

@@ -1019,7 +1019,8 @@ void MarkCompactCollector::Finish() {
 
 void MarkCompactCollector::SweepArrayBufferExtensions() {
   TRACE_GC(heap()->tracer(), GCTracer::Scope::MC_FINISH_SWEEP_ARRAY_BUFFERS);
-  heap_->array_buffer_sweeper()->RequestSweepFull();
+  heap_->array_buffer_sweeper()->RequestSweep(
+      ArrayBufferSweeper::SweepingType::kFull);
 }
 
 class MarkCompactCollector::RootMarkingVisitor final : public RootVisitor {
@@ -4622,7 +4623,8 @@ void MinorMarkCompactCollector::CleanupSweepToIteratePages() {
 }
 
 void MinorMarkCompactCollector::SweepArrayBufferExtensions() {
-  heap_->array_buffer_sweeper()->RequestSweepYoung();
+  heap_->array_buffer_sweeper()->RequestSweep(
+      ArrayBufferSweeper::SweepingType::kYoung);
 }
 
 class YoungGenerationMigrationObserver final : public MigrationObserver {

src/heap/scavenger.cc

@@ -490,7 +490,8 @@ void ScavengerCollector::IterateStackAndScavenge(
 }
 
 void ScavengerCollector::SweepArrayBufferExtensions() {
-  heap_->array_buffer_sweeper()->RequestSweepYoung();
+  heap_->array_buffer_sweeper()->RequestSweep(
+      ArrayBufferSweeper::SweepingType::kYoung);
 }
 
 void ScavengerCollector::HandleSurvivingNewLargeObjects() {

src/objects/js-array-buffer.h

@@ -160,52 +160,27 @@ class JSArrayBuffer
 // extension-object. The GC periodically iterates all extensions concurrently
 // and frees unmarked ones.
 // https://docs.google.com/document/d/1-ZrLdlFX1nXT3z-FAgLbKal1gI8Auiaya_My-a0UJ28/edit
-class ArrayBufferExtension : public Malloced {
-  enum class GcState : uint8_t { Dead = 0, Copied, Promoted };
-
-  std::atomic<bool> marked_;
-  std::atomic<GcState> young_gc_state_;
-  std::shared_ptr<BackingStore> backing_store_;
-  ArrayBufferExtension* next_;
-  std::atomic<size_t> accounting_length_;
-
-  GcState young_gc_state() {
-    return young_gc_state_.load(std::memory_order_relaxed);
-  }
-
-  void set_young_gc_state(GcState value) {
-    young_gc_state_.store(value, std::memory_order_relaxed);
-  }
-
+class ArrayBufferExtension final : public Malloced {
  public:
-  ArrayBufferExtension()
-      : marked_(false),
-        young_gc_state_(GcState::Dead),
-        backing_store_(std::shared_ptr<BackingStore>()),
-        next_(nullptr),
-        accounting_length_(0) {}
+  ArrayBufferExtension() : backing_store_(std::shared_ptr<BackingStore>()) {}
   explicit ArrayBufferExtension(std::shared_ptr<BackingStore> backing_store)
-      : marked_(false),
-        young_gc_state_(GcState::Dead),
-        backing_store_(backing_store),
-        next_(nullptr),
-        accounting_length_(0) {}
+      : backing_store_(backing_store) {}
 
   void Mark() { marked_.store(true, std::memory_order_relaxed); }
   void Unmark() { marked_.store(false, std::memory_order_relaxed); }
-  bool IsMarked() { return marked_.load(std::memory_order_relaxed); }
+  bool IsMarked() const { return marked_.load(std::memory_order_relaxed); }
 
   void YoungMark() { set_young_gc_state(GcState::Copied); }
   void YoungMarkPromoted() { set_young_gc_state(GcState::Promoted); }
   void YoungUnmark() { set_young_gc_state(GcState::Dead); }
-  bool IsYoungMarked() { return young_gc_state() != GcState::Dead; }
+  bool IsYoungMarked() const { return young_gc_state() != GcState::Dead; }
 
-  bool IsYoungPromoted() { return young_gc_state() == GcState::Promoted; }
+  bool IsYoungPromoted() const { return young_gc_state() == GcState::Promoted; }
 
   std::shared_ptr<BackingStore> backing_store() { return backing_store_; }
   BackingStore* backing_store_raw() { return backing_store_.get(); }
 
-  size_t accounting_length() {
+  size_t accounting_length() const {
     return accounting_length_.load(std::memory_order_relaxed);
   }
 
@@ -227,8 +202,25 @@ class ArrayBufferExtension : public Malloced {
 
   void reset_backing_store() { backing_store_.reset(); }
 
-  ArrayBufferExtension* next() { return next_; }
+  ArrayBufferExtension* next() const { return next_; }
   void set_next(ArrayBufferExtension* extension) { next_ = extension; }
+
+ private:
+  enum class GcState : uint8_t { Dead = 0, Copied, Promoted };
+
+  std::atomic<bool> marked_{false};
+  std::atomic<GcState> young_gc_state_{GcState::Dead};
+  std::shared_ptr<BackingStore> backing_store_;
+  ArrayBufferExtension* next_ = nullptr;
+  std::atomic<size_t> accounting_length_{0};
+
+  GcState young_gc_state() const {
+    return young_gc_state_.load(std::memory_order_relaxed);
+  }
+
+  void set_young_gc_state(GcState value) {
+    young_gc_state_.store(value, std::memory_order_relaxed);
+  }
 };
 
 class JSArrayBufferView

test/cctest/heap/test-array-buffer-tracker.cc

@@ -15,22 +15,22 @@
 namespace {
 
 bool IsTrackedYoung(i::Heap* heap, i::ArrayBufferExtension* extension) {
-  bool in_young = heap->array_buffer_sweeper()->young().Contains(extension);
-  bool in_old = heap->array_buffer_sweeper()->old().Contains(extension);
+  bool in_young = heap->array_buffer_sweeper()->young().ContainsSlow(extension);
+  bool in_old = heap->array_buffer_sweeper()->old().ContainsSlow(extension);
   CHECK(!(in_young && in_old));
   return in_young;
 }
 
 bool IsTrackedOld(i::Heap* heap, i::ArrayBufferExtension* extension) {
-  bool in_young = heap->array_buffer_sweeper()->young().Contains(extension);
-  bool in_old = heap->array_buffer_sweeper()->old().Contains(extension);
+  bool in_young = heap->array_buffer_sweeper()->young().ContainsSlow(extension);
+  bool in_old = heap->array_buffer_sweeper()->old().ContainsSlow(extension);
   CHECK(!(in_young && in_old));
   return in_old;
 }
 
 bool IsTracked(i::Heap* heap, i::ArrayBufferExtension* extension) {
-  bool in_young = heap->array_buffer_sweeper()->young().Contains(extension);
-  bool in_old = heap->array_buffer_sweeper()->old().Contains(extension);
+  bool in_young = heap->array_buffer_sweeper()->young().ContainsSlow(extension);
+  bool in_old = heap->array_buffer_sweeper()->old().ContainsSlow(extension);
   CHECK(!(in_young && in_old));
   return in_young || in_old;
 }