Revert "[heap] Concurrently free ArrayBuffer allocations."

This reverts commit b6658adee0.

Reason for revert: Breaks TSAN :(

Original change's description:
> [heap] Concurrently free ArrayBuffer allocations.
> 
> Free ArrayBuffer backing stores on a background thread, rather than
> blocking the main thread after processing. Could potentially cause
> contention with the array buffer allocator once JS execution resumes.
> 
> The new ArrayBufferCollector class tracks these dead allocations.
> 
> Later, the processing of array buffers can happen in parallel.
> 
> Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
> 
> Bug: v8:6992
> Change-Id: I49ae4db12ed62d8400ba2bbafeda05a11479d904
> Reviewed-on: https://chromium-review.googlesource.com/739829
> Commit-Queue: Peter Marshall <petermarshall@chromium.org>
> Reviewed-by: Michael Lippautz <mlippautz@chromium.org>
> Reviewed-by: Hannes Payer <hpayer@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#49485}

TBR=hpayer@chromium.org,mlippautz@chromium.org,petermarshall@chromium.org

Change-Id: If6743b83f871c0fd0d6e83a3083dce0eecd99021
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:6992
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_chromium_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/779159
Reviewed-by: Peter Marshall <petermarshall@chromium.org>
Commit-Queue: Peter Marshall <petermarshall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49488}

BUILD.gn

@@ -1617,8 +1617,6 @@ v8_source_set("v8_base") {
     "src/handles.cc",
     "src/handles.h",
     "src/heap-symbols.h",
-    "src/heap/array-buffer-collector.cc",
-    "src/heap/array-buffer-collector.h",
     "src/heap/array-buffer-tracker-inl.h",
     "src/heap/array-buffer-tracker.cc",
     "src/heap/array-buffer-tracker.h",

src/flag-definitions.h

@@ -635,8 +635,6 @@ DEFINE_BOOL(trace_gc_object_stats, false,
             "trace object counts and memory usage")
 DEFINE_BOOL(track_retaining_path, false,
             "enable support for tracking retaining path")
-DEFINE_BOOL(concurrent_array_buffer_freeing, true,
-            "free array buffer allocations on a background thread")
 DEFINE_INT(gc_stats, 0, "Used by tracing internally to enable gc statistics")
 DEFINE_IMPLICATION(trace_gc_object_stats, track_gc_object_stats)
 DEFINE_VALUE_IMPLICATION(track_gc_object_stats, gc_stats, 1)

src/heap/array-buffer-collector.cc

@@ -1,58 +0,0 @@
-// Copyright 2017 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/array-buffer-collector.h"
-
-#include "src/heap/array-buffer-tracker.h"
-#include "src/heap/heap-inl.h"
-
-namespace v8 {
-namespace internal {
-
-void ArrayBufferCollector::AddGarbageAllocations(
-    std::vector<JSArrayBuffer::Allocation>* allocations) {
-  base::LockGuard<base::Mutex> guard(&allocations_mutex_);
-  allocations_.push_back(allocations);
-}
-
-void ArrayBufferCollector::FreeAllocations() {
-  base::LockGuard<base::Mutex> guard(&allocations_mutex_);
-  for (std::vector<JSArrayBuffer::Allocation>* allocations : allocations_) {
-    for (auto alloc : *allocations) {
-      JSArrayBuffer::FreeBackingStore(heap_->isolate(), alloc);
-    }
-    delete allocations;
-  }
-  allocations_.clear();
-  heap_->account_external_memory_concurrently_freed();
-}
-
-class ArrayBufferCollector::FreeingTask final : public CancelableTask {
- public:
-  explicit FreeingTask(Heap* heap)
-      : CancelableTask(heap->isolate()), heap_(heap) {}
-
-  virtual ~FreeingTask() {}
-
- private:
-  void RunInternal() final {
-    heap_->array_buffer_collector()->FreeAllocations();
-  }
-
-  Heap* heap_;
-};
-
-void ArrayBufferCollector::FreeAllocationsOnBackgroundThread() {
-  if (heap_->use_tasks() && FLAG_concurrent_array_buffer_freeing) {
-    FreeingTask* task = new FreeingTask(heap_);
-    V8::GetCurrentPlatform()->CallOnBackgroundThread(
-        task, v8::Platform::kShortRunningTask);
-  } else {
-    // Fallback for when concurrency is disabled/restricted.
-    FreeAllocations();
-  }
-}
-
-}  // namespace internal
-}  // namespace v8

src/heap/array-buffer-collector.h

@@ -1,51 +0,0 @@
-// Copyright 2017 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_ARRAY_BUFFER_COLLECTOR_H_
-#define V8_HEAP_ARRAY_BUFFER_COLLECTOR_H_
-
-#include <vector>
-
-#include "src/base/platform/mutex.h"
-#include "src/objects/js-array.h"
-
-namespace v8 {
-namespace internal {
-
-class Heap;
-
-// To support background processing of array buffer backing stores, we process
-// array buffers using the ArrayBufferTracker class. The ArrayBufferCollector
-// keeps track of garbage backing stores so that they can be freed on a
-// background thread.
-class ArrayBufferCollector {
- public:
-  explicit ArrayBufferCollector(Heap* heap) : heap_(heap) {}
-
-  ~ArrayBufferCollector() { FreeAllocations(); }
-
-  // These allocations will begin to be freed once FreeAllocations() is called,
-  // or on TearDown.
-  void AddGarbageAllocations(
-      std::vector<JSArrayBuffer::Allocation>* allocations);
-
-  // Calls FreeAllocations() on a background thread.
-  void FreeAllocationsOnBackgroundThread();
-
- private:
-  class FreeingTask;
-
-  // Begin freeing the allocations added through AddGarbageAllocations. Also
-  // called by TearDown.
-  void FreeAllocations();
-
-  Heap* heap_;
-  base::Mutex allocations_mutex_;
-  std::vector<std::vector<JSArrayBuffer::Allocation>*> allocations_;
-};
-
-}  // namespace internal
-}  // namespace v8
-
-#endif  // V8_HEAP_ARRAY_BUFFER_COLLECTOR_H_

src/heap/array-buffer-tracker.cc

@@ -3,10 +3,6 @@
 // found in the LICENSE file.
 
 #include "src/heap/array-buffer-tracker.h"
-
-#include <vector>
-
-#include "src/heap/array-buffer-collector.h"
 #include "src/heap/array-buffer-tracker-inl.h"
 #include "src/heap/heap.h"
 #include "src/heap/spaces.h"
@@ -20,9 +16,6 @@ LocalArrayBufferTracker::~LocalArrayBufferTracker() {
 
 template <typename Callback>
 void LocalArrayBufferTracker::Process(Callback callback) {
-  std::vector<JSArrayBuffer::Allocation>* backing_stores_to_free =
-      new std::vector<JSArrayBuffer::Allocation>();
-
   JSArrayBuffer* new_buffer = nullptr;
   JSArrayBuffer* old_buffer = nullptr;
   size_t new_retained_size = 0;
@@ -52,12 +45,7 @@ void LocalArrayBufferTracker::Process(Callback callback) {
       it = array_buffers_.erase(it);
     } else if (result == kRemoveEntry) {
       // Size of freed memory is computed to avoid looking at dead objects.
-      void* allocation_base = old_buffer->allocation_base();
-      DCHECK_NOT_NULL(allocation_base);
-
-      backing_stores_to_free->emplace_back(allocation_base,
-                                           old_buffer->allocation_length(),
-                                           old_buffer->allocation_mode());
+      old_buffer->FreeBackingStore();
       it = array_buffers_.erase(it);
     } else {
       UNREACHABLE();
@@ -69,21 +57,16 @@ void LocalArrayBufferTracker::Process(Callback callback) {
         static_cast<intptr_t>(freed_memory));
   }
   retained_size_ = new_retained_size;
-
-  // Pass the backing stores that need to be freed to the main thread for later
-  // distribution.
-  // ArrayBufferCollector takes ownership of this pointer.
-  heap_->array_buffer_collector()->AddGarbageAllocations(
-      backing_stores_to_free);
 }
 
-void ArrayBufferTracker::PrepareToFreeDeadInNewSpace(Heap* heap) {
+void ArrayBufferTracker::FreeDeadInNewSpace(Heap* heap) {
   DCHECK_EQ(heap->gc_state(), Heap::HeapState::SCAVENGE);
   for (Page* page : PageRange(heap->new_space()->FromSpaceStart(),
                               heap->new_space()->FromSpaceEnd())) {
     bool empty = ProcessBuffers(page, kUpdateForwardedRemoveOthers);
     CHECK(empty);
   }
+  heap->account_external_memory_concurrently_freed();
 }
 
 size_t ArrayBufferTracker::RetainedInNewSpace(Heap* heap) {

src/heap/array-buffer-tracker.h

@@ -34,9 +34,9 @@ class ArrayBufferTracker : public AllStatic {
   inline static void RegisterNew(Heap* heap, JSArrayBuffer* buffer);
   inline static void Unregister(Heap* heap, JSArrayBuffer* buffer);
 
-  // Identifies all backing store pointers for dead JSArrayBuffers in new space.
+  // Frees all backing store pointers for dead JSArrayBuffers in new space.
   // Does not take any locks and can only be called during Scavenge.
-  static void PrepareToFreeDeadInNewSpace(Heap* heap);
+  static void FreeDeadInNewSpace(Heap* heap);
 
   // Number of array buffer bytes retained from new space.
   static size_t RetainedInNewSpace(Heap* heap);

src/heap/heap.cc

@@ -23,7 +23,6 @@
 #include "src/deoptimizer.h"
 #include "src/feedback-vector.h"
 #include "src/global-handles.h"
-#include "src/heap/array-buffer-collector.h"
 #include "src/heap/array-buffer-tracker-inl.h"
 #include "src/heap/barrier.h"
 #include "src/heap/code-stats.h"
@@ -192,7 +191,6 @@ Heap::Heap()
       last_gc_time_(0.0),
       mark_compact_collector_(nullptr),
       minor_mark_compact_collector_(nullptr),
-      array_buffer_collector_(nullptr),
       memory_allocator_(nullptr),
       store_buffer_(nullptr),
       incremental_marking_(nullptr),
@@ -2061,10 +2059,8 @@ void Heap::Scavenge() {
 
   {
     TRACE_GC(tracer(), GCTracer::Scope::SCAVENGER_PROCESS_ARRAY_BUFFERS);
-    ArrayBufferTracker::PrepareToFreeDeadInNewSpace(this);
+    ArrayBufferTracker::FreeDeadInNewSpace(this);
   }
-  array_buffer_collector()->FreeAllocationsOnBackgroundThread();
-
   RememberedSet<OLD_TO_NEW>::IterateMemoryChunks(this, [](MemoryChunk* chunk) {
     if (chunk->SweepingDone()) {
       RememberedSet<OLD_TO_NEW>::FreeEmptyBuckets(chunk);
@@ -5509,7 +5505,6 @@ bool Heap::SetUp() {
 
   tracer_ = new GCTracer(this);
   minor_mark_compact_collector_ = new MinorMarkCompactCollector(this);
-  array_buffer_collector_ = new ArrayBufferCollector(this);
   gc_idle_time_handler_ = new GCIdleTimeHandler();
   memory_reducer_ = new MemoryReducer(this);
   if (V8_UNLIKELY(FLAG_gc_stats)) {
@@ -5658,11 +5653,6 @@ void Heap::TearDown() {
     minor_mark_compact_collector_ = nullptr;
   }
 
-  if (array_buffer_collector_ != nullptr) {
-    delete array_buffer_collector_;
-    array_buffer_collector_ = nullptr;
-  }
-
   delete incremental_marking_;
   incremental_marking_ = nullptr;
 

src/heap/heap.h

@@ -388,7 +388,6 @@ using v8::MemoryPressureLevel;
   } while (false)
 
 class AllocationObserver;
-class ArrayBufferCollector;
 class ArrayBufferTracker;
 class ConcurrentMarking;
 class GCIdleTimeAction;
@@ -1010,10 +1009,6 @@ class Heap {
     return minor_mark_compact_collector_;
   }
 
-  ArrayBufferCollector* array_buffer_collector() {
-    return array_buffer_collector_;
-  }
-
   // ===========================================================================
   // Root set access. ==========================================================
   // ===========================================================================
@@ -2412,8 +2407,6 @@ class Heap {
   MarkCompactCollector* mark_compact_collector_;
   MinorMarkCompactCollector* minor_mark_compact_collector_;
 
-  ArrayBufferCollector* array_buffer_collector_;
-
   MemoryAllocator* memory_allocator_;
 
   StoreBuffer* store_buffer_;

src/heap/mark-compact.cc

@@ -14,7 +14,6 @@
 #include "src/execution.h"
 #include "src/frames-inl.h"
 #include "src/global-handles.h"
-#include "src/heap/array-buffer-collector.h"
 #include "src/heap/array-buffer-tracker-inl.h"
 #include "src/heap/concurrent-marking.h"
 #include "src/heap/gc-tracer.h"
@@ -4293,7 +4292,6 @@ void MarkCompactCollector::UpdatePointersAfterEvacuation() {
         updating_job.AddTask(new PointersUpdatingTask(isolate()));
       }
       updating_job.Run();
-      heap()->array_buffer_collector()->FreeAllocationsOnBackgroundThread();
     }
   }
 
@@ -4351,7 +4349,6 @@ void MinorMarkCompactCollector::UpdatePointersAfterEvacuation() {
     TRACE_GC(heap()->tracer(),
              GCTracer::Scope::MINOR_MC_EVACUATE_UPDATE_POINTERS_SLOTS);
     updating_job.Run();
-    heap()->array_buffer_collector()->FreeAllocationsOnBackgroundThread();
   }
 
   {

src/objects.cc

@@ -18858,7 +18858,7 @@ void JSArrayBuffer::FreeBackingStore() {
   using AllocationMode = ArrayBuffer::Allocator::AllocationMode;
   const size_t length = allocation_length();
   const AllocationMode mode = allocation_mode();
-  FreeBackingStore(GetIsolate(), {allocation_base(), length, mode});
+  GetIsolate()->array_buffer_allocator()->Free(allocation_base(), length, mode);
 
   // Zero out the backing store and allocation base to avoid dangling
   // pointers.
@@ -18870,12 +18870,6 @@ void JSArrayBuffer::FreeBackingStore() {
   set_allocation_length(0);
 }
 
-// static
-void JSArrayBuffer::FreeBackingStore(Isolate* isolate, Allocation allocation) {
-  isolate->array_buffer_allocator()->Free(allocation.allocation_base,
-                                          allocation.length, allocation.mode);
-}
-
 void JSArrayBuffer::Setup(Handle<JSArrayBuffer> array_buffer, Isolate* isolate,
                           bool is_external, void* data, size_t allocated_length,
                           SharedFlag shared) {

src/objects/js-array.h

@@ -179,19 +179,7 @@ class JSArrayBuffer : public JSObject {
 
   inline ArrayBuffer::Allocator::AllocationMode allocation_mode() const;
 
-  struct Allocation {
-    using AllocationMode = ArrayBuffer::Allocator::AllocationMode;
-
-    Allocation(void* allocation_base, size_t length, AllocationMode mode)
-        : allocation_base(allocation_base), length(length), mode(mode) {}
-
-    void* allocation_base;
-    size_t length;
-    AllocationMode mode;
-  };
-
   void FreeBackingStore();
-  static void FreeBackingStore(Isolate* isolate, Allocation allocation);
 
   V8_EXPORT_PRIVATE static void Setup(
       Handle<JSArrayBuffer> array_buffer, Isolate* isolate, bool is_external,

src/v8.gyp

@@ -990,8 +990,6 @@
         'handles.cc',
         'handles.h',
         'heap-symbols.h',
-        'heap/array-buffer-collector.cc',
-        'heap/array-buffer-collector.h',
         'heap/array-buffer-tracker-inl.h',
         'heap/array-buffer-tracker.cc',
         'heap/array-buffer-tracker.h',