Revert "[wasm] Lazy update instances on a shared Memory.Grow"

This reverts commit 80f06d6f.

Reason for revert: failing grow-memory tests

Original change's description:
> [wasm] Lazy update instances on a shared Memory.Grow
> 
>  - Introduce a GROW_SHARED_MEMORY interrupt, and handler
>  - Memory objects for isolates are updated on a stack check, add
>    tracking for isolates that hit the stack check
>  - When enough memory is not reserved ahead of time, fail to grow
>  - Add tracking for externalized buffers in the MemoryTracker so
>    that the MemoryTracker will know when backing_stores can be freed.
>  - For shared buffer, do not always allocate a new buffer when
>    growing an externalized buffer
> 
> 
> Change-Id: I9cf1be19f2f165fa6ea4096869f7d6365304c8c4
> Bug: v8:8564
> Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1472430
> Commit-Queue: Deepti Gandluri <gdeepti@chromium.org>
> Reviewed-by: Ben Smith <binji@chromium.org>
> Reviewed-by: Andreas Haas <ahaas@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#60064}

TBR=binji@chromium.org,titzer@chromium.org,gdeepti@chromium.org,ahaas@chromium.org

Change-Id: I2ed0b59bcbb285b701172b401d606963261d375c
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: v8:8564
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/1506355Reviewed-by: Bill Budge <bbudge@chromium.org>
Commit-Queue: Bill Budge <bbudge@chromium.org>
Cr-Commit-Position: refs/heads/master@{#60068}

src/execution.cc

@@ -640,12 +640,6 @@ Object StackGuard::HandleInterrupts() {
     isolate_->heap()->HandleGCRequest();
   }
 
-  if (CheckAndClearInterrupt(GROW_SHARED_MEMORY)) {
-    TRACE_INTERRUPT("GROW_SHARED_MEMORY");
-    isolate_->wasm_engine()->memory_tracker()->UpdateSharedMemoryInstances(
-        isolate_);
-  }
-
   if (CheckAndClearInterrupt(TERMINATE_EXECUTION)) {
     TRACE_INTERRUPT("TERMINATE_EXECUTION");
     return isolate_->TerminateExecution();

src/execution.h

@@ -91,13 +91,12 @@ class V8_EXPORT_PRIVATE StackGuard final {
   // it has been set up.
   void ClearThread(const ExecutionAccess& lock);
 
-#define INTERRUPT_LIST(V)                                         \
-  V(TERMINATE_EXECUTION, TerminateExecution, 0)                   \
-  V(GC_REQUEST, GC, 1)                                            \
-  V(INSTALL_CODE, InstallCode, 2)                                 \
-  V(API_INTERRUPT, ApiInterrupt, 3)                               \
-  V(DEOPT_MARKED_ALLOCATION_SITES, DeoptMarkedAllocationSites, 4) \
-  V(GROW_SHARED_MEMORY, GrowSharedMemory, 5)
+#define INTERRUPT_LIST(V)                       \
+  V(TERMINATE_EXECUTION, TerminateExecution, 0) \
+  V(GC_REQUEST, GC, 1)                          \
+  V(INSTALL_CODE, InstallCode, 2)               \
+  V(API_INTERRUPT, ApiInterrupt, 3)             \
+  V(DEOPT_MARKED_ALLOCATION_SITES, DeoptMarkedAllocationSites, 4)
 
 #define V(NAME, Name, id)                                                    \
   inline bool Check##Name() { return CheckInterrupt(NAME); }                 \

src/isolate.cc

@@ -2922,8 +2922,6 @@ void Isolate::Deinit() {
     optimizing_compile_dispatcher_ = nullptr;
   }
 
-  wasm_engine()->memory_tracker()->DeleteSharedMemoryObjectsOnIsolate(this);
-
   heap_.mark_compact_collector()->EnsureSweepingCompleted();
   heap_.memory_allocator()->unmapper()->EnsureUnmappingCompleted();
 

src/objects/js-array-buffer-inl.h

@@ -77,7 +77,11 @@ void* JSArrayBuffer::allocation_base() const {
 }
 
 bool JSArrayBuffer::is_wasm_memory() const {
-  return IsWasmMemoryBit::decode(bit_field());
+  bool const is_wasm_memory = IsWasmMemoryBit::decode(bit_field());
+  DCHECK_EQ(is_wasm_memory,
+            GetIsolate()->wasm_engine()->memory_tracker()->IsWasmMemory(
+                backing_store()));
+  return is_wasm_memory;
 }
 
 void JSArrayBuffer::set_is_wasm_memory(bool is_wasm_memory) {

src/objects/js-array-buffer.cc

@@ -69,7 +69,11 @@ void JSArrayBuffer::FreeBackingStore(Isolate* isolate, Allocation allocation) {
   if (allocation.is_wasm_memory) {
     wasm::WasmMemoryTracker* memory_tracker =
         isolate->wasm_engine()->memory_tracker();
-    memory_tracker->FreeMemoryIfIsWasmMemory(isolate, allocation.backing_store);
+    if (!memory_tracker->FreeMemoryIfIsWasmMemory(isolate,
+                                                  allocation.backing_store)) {
+      CHECK(FreePages(GetPlatformPageAllocator(), allocation.allocation_base,
+                      allocation.length));
+    }
   } else {
     isolate->array_buffer_allocator()->Free(allocation.allocation_base,
                                             allocation.length);

src/value-serializer.cc

@@ -922,9 +922,6 @@ Maybe<bool> ValueSerializer::WriteWasmMemory(Handle<WasmMemoryObject> object) {
     return Nothing<bool>();
   }
 
-  isolate_->wasm_engine()->memory_tracker()->RegisterWasmMemoryAsShared(
-      object, isolate_);
-
   WriteTag(SerializationTag::kWasmMemoryTransfer);
   WriteZigZag<int32_t>(object->maximum_pages());
   return WriteJSReceiver(Handle<JSReceiver>(object->array_buffer(), isolate_));
@@ -1869,9 +1866,6 @@ MaybeHandle<WasmMemoryObject> ValueDeserializer::ReadWasmMemory() {
   Handle<WasmMemoryObject> result =
       WasmMemoryObject::New(isolate_, buffer, maximum_pages);
 
-  isolate_->wasm_engine()->memory_tracker()->RegisterWasmMemoryAsShared(
-      result, isolate_);
-
   AddObjectWithID(id, result);
   return result;
 }

src/wasm/wasm-memory.h

@@ -7,7 +7,6 @@
 
 #include <atomic>
 #include <unordered_map>
-#include <unordered_set>
 
 #include "src/base/platform/mutex.h"
 #include "src/flags.h"
@@ -40,27 +39,11 @@ class WasmMemoryTracker {
                           size_t allocation_length, void* buffer_start,
                           size_t buffer_length);
 
-  struct SharedMemoryObjectState {
-    Handle<WasmMemoryObject> memory_object;
-    Isolate* isolate;
-
-    SharedMemoryObjectState() = default;
-    SharedMemoryObjectState(Handle<WasmMemoryObject> memory_object,
-                            Isolate* isolate)
-        : memory_object(memory_object), isolate(isolate) {}
-  };
-
   struct AllocationData {
     void* allocation_base = nullptr;
     size_t allocation_length = 0;
     void* buffer_start = nullptr;
     size_t buffer_length = 0;
-    bool is_shared = false;
-
-    // Track Wasm Memory instances across isolates, this is populated on
-    // PostMessage using persistent handles for memory objects.
-    std::vector<WasmMemoryTracker::SharedMemoryObjectState>
-        memory_object_vector;
 
    private:
     AllocationData() = default;
@@ -98,11 +81,11 @@ class WasmMemoryTracker {
   // Decreases the amount of reserved address space.
   void ReleaseReservation(size_t num_bytes);
 
+  // Removes an allocation from the tracker.
+  AllocationData ReleaseAllocation(Isolate* isolate, const void* buffer_start);
 
   bool IsWasmMemory(const void* buffer_start);
 
-  bool IsWasmSharedMemory(const void* buffer_start);
-
   // Returns whether the given buffer is a Wasm memory with guard regions large
   // enough to safely use trap handlers.
   bool HasFullGuardRegions(const void* buffer_start);
@@ -116,26 +99,6 @@ class WasmMemoryTracker {
   // free the buffer manually.
   bool FreeMemoryIfIsWasmMemory(Isolate* isolate, const void* buffer_start);
 
-  // When WebAssembly.Memory is transferred over PostMessage, register the
-  // allocation as shared and track the memory objects that will need
-  // updating if memory is resized.
-  void RegisterWasmMemoryAsShared(Handle<WasmMemoryObject> object,
-                                  Isolate* isolate);
-
-  // This method is called when the underlying backing store is grown, but
-  // instances that share the backing_store have not yet been updated.
-  void SetPendingUpdateOnGrow(Handle<JSArrayBuffer> old_buffer,
-                              size_t new_size);
-
-  // Interrupt handler for GROW_SHARED_MEMORY interrupt. Update memory objects
-  // and instances that share the memory objects  after a Grow call.
-  void UpdateSharedMemoryInstances(Isolate* isolate);
-
-  // Due to timing of when buffers are garbage collected, vs. when isolate
-  // object handles are destroyed, it is possible to leak global handles. To
-  // avoid this, cleanup any global handles on isolate destruction if any exist.
-  void DeleteSharedMemoryObjectsOnIsolate(Isolate* isolate);
-
   // Allocation results are reported to UMA
   //
   // See wasm_memory_allocation_result in counters.h
@@ -151,70 +114,8 @@ class WasmMemoryTracker {
   };
 
  private:
-  // Helper methods to free memory only if not shared by other isolates, memory
-  // objects.
-  void FreeMemoryIfNotShared_Locked(Isolate* isolate,
-                                    const void* backing_store);
-  bool CanFreeSharedMemory_Locked(const void* backing_store);
-  void RemoveSharedBufferState_Locked(Isolate* isolate,
-                                      const void* backing_store);
-
-  // Registers the allocation as shared, and tracks all the memory objects
-  // associates with this allocation across isolates.
-  void RegisterSharedWasmMemory_Locked(Handle<WasmMemoryObject> object,
-                                       Isolate* isolate);
-
-  // Map the new size after grow to the buffer backing store, so that instances
-  // and memory objects that share the WebAssembly.Memory across isolates can
-  // be updated..
-  void AddBufferToGrowMap_Locked(Handle<JSArrayBuffer> old_buffer,
-                                 size_t new_size);
-
-  // Trigger a GROW_SHARED_MEMORY interrupt on all the isolates that have memory
-  // objects that share this buffer.
-  void TriggerSharedGrowInterruptOnAllIsolates_Locked(
-      Handle<JSArrayBuffer> old_buffer);
-
-  // When isolates hit a stack check, update the memory objects associated with
-  // that isolate.
-  void UpdateSharedMemoryStateOnInterrupt_Locked(Isolate* isolate,
-                                                 void* backing_store,
-                                                 size_t new_size);
-
-  // Check if all the isolates that share a backing_store have hit a stack
-  // check. If a stack check is hit, and the backing store is pending grow,
-  // this isolate will have updated memory objects.
-  bool AreAllIsolatesUpdated_Locked(const void* backing_store);
-
-  // If a grow call is made to a buffer with a pending grow, and all the
-  // isolates that share this buffer have not hit a StackCheck, clear the set of
-  // already updated instances so they can be updated with the new size on the
-  // most recent grow call.
-  void ClearUpdatedInstancesOnPendingGrow_Locked(const void* backing_store);
-
-  // Helper functions to update memory objects on grow, and maintain state for
-  // which isolates hit a stack check.
-  void UpdateMemoryObjectsForIsolate_Locked(Isolate* isolate,
-                                            void* backing_store,
-                                            size_t new_size);
-  bool MemoryObjectsNeedUpdate_Locked(Isolate* isolate,
-                                      const void* backing_store);
-
-  // Destroy global handles to memory objects, and remove backing store from
-  // isolates_per_buffer on Free.
-  void DestroyMemoryObjectsAndRemoveIsolateEntry_Locked(
-      Isolate* isolate, const void* backing_store);
-  void DestroyMemoryObjectsAndRemoveIsolateEntry_Locked(
-      const void* backing_store);
-
-  void RemoveIsolateFromBackingStore_Locked(Isolate* isolate,
-                                            const void* backing_store);
-
-  // Removes an allocation from the tracker.
-  AllocationData ReleaseAllocation_Locked(Isolate* isolate,
-                                          const void* buffer_start);
-
   void AddAddressSpaceSample(Isolate* isolate);
+
   // Clients use a two-part process. First they "reserve" the address space,
   // which signifies an intent to actually allocate it. This determines whether
   // doing the allocation would put us over our limit. Once there is a
@@ -231,36 +132,10 @@ class WasmMemoryTracker {
 
   size_t allocated_address_space_ = 0;
 
-  //////////////////////////////////////////////////////////////////////////////
-  // Protected by {mutex_}:
-
   // Track Wasm memory allocation information. This is keyed by the start of the
   // buffer, rather than by the start of the allocation.
   std::unordered_map<const void*, AllocationData> allocations_;
 
-  // Maps each buffer to the isolates that share the backing store.
-  std::unordered_map<const void*, std::unordered_set<Isolate*>>
-      isolates_per_buffer_;
-
-  // Maps which isolates have had a grow interrupt handled on the buffer. This
-  // is maintained to ensure that the instances are updated with the right size
-  // on Grow.
-  std::unordered_map<const void*, std::unordered_set<Isolate*>>
-      isolates_updated_on_grow_;
-
-  // Maps backing stores(void*) to the size of the underlying memory in
-  // (size_t). An entry to this map is made on a grow call to the corresponding
-  // backing store. On consecutive grow calls to the same backing store,
-  // the size entry is updated. This entry is made right after the mprotect
-  // call to change the protections on a backing_store, so the memory objects
-  // have not been updated yet. The backing store entry in this map is erased
-  // when all the memory objects, or instances that share this backing store
-  // have their bounds updated.
-  std::unordered_map<void*, size_t> grow_update_map_;
-
-  // End of fields protected by {mutex_}.
-  //////////////////////////////////////////////////////////////////////////////
-
   DISALLOW_COPY_AND_ASSIGN(WasmMemoryTracker);
 };
 

src/wasm/wasm-objects.cc

@@ -910,30 +910,12 @@ void WasmTableObject::ClearDispatchTables(Isolate* isolate,
 }
 
 namespace {
-bool AdjustBufferPermissions(Isolate* isolate, Handle<JSArrayBuffer> old_buffer,
-                             size_t new_size) {
-  if (new_size > old_buffer->allocation_length()) return false;
-  void* old_mem_start = old_buffer->backing_store();
-  size_t old_size = old_buffer->byte_length();
-  if (old_size != new_size) {
-    DCHECK_NOT_NULL(old_mem_start);
-    DCHECK_GE(new_size, old_size);
-    // If adjusting permissions fails, propagate error back to return
-    // failure to grow.
-    if (!i::SetPermissions(GetPlatformPageAllocator(), old_mem_start, new_size,
-                           PageAllocator::kReadWrite)) {
-      return false;
-    }
-    reinterpret_cast<v8::Isolate*>(isolate)
-        ->AdjustAmountOfExternalAllocatedMemory(new_size - old_size);
-  }
-  return true;
-}
-
 MaybeHandle<JSArrayBuffer> MemoryGrowBuffer(Isolate* isolate,
                                             Handle<JSArrayBuffer> old_buffer,
                                             size_t new_size) {
   CHECK_EQ(0, new_size % wasm::kWasmPageSize);
+  size_t old_size = old_buffer->byte_length();
+  void* old_mem_start = old_buffer->backing_store();
   // Reusing the backing store from externalized buffers causes problems with
   // Blink's array buffers. The connection between the two is lost, which can
   // lead to Blink not knowing about the other reference to the buffer and
@@ -950,12 +932,10 @@ MaybeHandle<JSArrayBuffer> MemoryGrowBuffer(Isolate* isolate,
     // If the old buffer had full guard regions, we can only safely use the new
     // buffer if it also has full guard regions. Otherwise, we'd have to
     // recompile all the instances using this memory to insert bounds checks.
-    void* old_mem_start = old_buffer->backing_store();
     if (memory_tracker->HasFullGuardRegions(old_mem_start) &&
         !memory_tracker->HasFullGuardRegions(new_buffer->backing_store())) {
       return {};
     }
-    size_t old_size = old_buffer->byte_length();
     if (old_size == 0) return new_buffer;
     memcpy(new_buffer->backing_store(), old_mem_start, old_size);
     DCHECK(old_buffer.is_null() || !old_buffer->is_shared());
@@ -963,7 +943,18 @@ MaybeHandle<JSArrayBuffer> MemoryGrowBuffer(Isolate* isolate,
     i::wasm::DetachMemoryBuffer(isolate, old_buffer, free_memory);
     return new_buffer;
   } else {
-    if (!AdjustBufferPermissions(isolate, old_buffer, new_size)) return {};
+    if (old_size != new_size) {
+      DCHECK_NOT_NULL(old_buffer->backing_store());
+      // If adjusting permissions fails, propagate error back to return
+      // failure to grow.
+      if (!i::SetPermissions(GetPlatformPageAllocator(), old_mem_start,
+                             new_size, PageAllocator::kReadWrite)) {
+        return {};
+      }
+      DCHECK_GE(new_size, old_size);
+      reinterpret_cast<v8::Isolate*>(isolate)
+          ->AdjustAmountOfExternalAllocatedMemory(new_size - old_size);
+    }
     // NOTE: We must allocate a new array buffer here because the spec
     // assumes that ArrayBuffers do not change size.
     void* backing_store = old_buffer->backing_store();
@@ -1083,32 +1074,15 @@ void WasmMemoryObject::AddInstance(Isolate* isolate,
   SetInstanceMemory(instance, buffer);
 }
 
-void WasmMemoryObject::update_instances(Isolate* isolate,
-                                        Handle<JSArrayBuffer> buffer) {
-  if (has_instances()) {
-    Handle<WeakArrayList> instances(this->instances(), isolate);
-    for (int i = 0; i < instances->length(); i++) {
-      MaybeObject elem = instances->Get(i);
-      HeapObject heap_object;
-      if (elem->GetHeapObjectIfWeak(&heap_object)) {
-        Handle<WasmInstanceObject> instance(
-            WasmInstanceObject::cast(heap_object), isolate);
-        SetInstanceMemory(instance, buffer);
-      } else {
-        DCHECK(elem->IsCleared());
-      }
-    }
-  }
-  set_array_buffer(*buffer);
-}
-
 // static
 int32_t WasmMemoryObject::Grow(Isolate* isolate,
                                Handle<WasmMemoryObject> memory_object,
                                uint32_t pages) {
   TRACE_EVENT0(TRACE_DISABLED_BY_DEFAULT("v8.wasm"), "GrowMemory");
   Handle<JSArrayBuffer> old_buffer(memory_object->array_buffer(), isolate);
-  if (!old_buffer->is_growable()) return -1;
+  // TODO(gdeepti): Remove check for is_shared when Growing Shared memory
+  // is supported.
+  if (!old_buffer->is_growable() || old_buffer->is_shared()) return -1;
 
   // Checks for maximum memory size, compute new size.
   uint32_t maximum_pages = wasm::max_mem_pages();
@@ -1128,45 +1102,28 @@ int32_t WasmMemoryObject::Grow(Isolate* isolate,
   size_t new_size =
       static_cast<size_t>(old_pages + pages) * wasm::kWasmPageSize;
 
-  // Memory is grown, but the memory objects and instances are not yet updated.
-  // Handle this in the interrupt handler so that it's safe for all the isolates
-  // that share this buffer to be updated safely.
+  // Grow the buffer.
   Handle<JSArrayBuffer> new_buffer;
-  if (old_buffer->is_shared()) {
-    // Adjust protections for the buffer.
-    if (!AdjustBufferPermissions(isolate, old_buffer, new_size)) {
-      return -1;
-    }
-    wasm::WasmMemoryTracker* const memory_tracker =
-        isolate->wasm_engine()->memory_tracker();
-    void* backing_store = old_buffer->backing_store();
-    if (memory_tracker->IsWasmSharedMemory(backing_store)) {
-      // This memory is shared between different isolates.
-      DCHECK(old_buffer->is_shared());
-      // Update pending grow state, and trigger a grow interrupt on all the
-      // isolates that share this buffer.
-      isolate->wasm_engine()->memory_tracker()->SetPendingUpdateOnGrow(
-          old_buffer, new_size);
-      // Handle interrupts for this isolate so that the instances with this
-      // isolate are updated.
-      isolate->stack_guard()->HandleInterrupts();
-      // Failure to allocate, or adjust pemissions already handled here, and
-      // updates to instances handled in the interrupt handler safe to return.
-      return static_cast<uint32_t>(old_size / wasm::kWasmPageSize);
-    }
-    // SharedArrayBuffer, but not shared across isolates. Setup a new buffer
-    // with updated permissions and update the instances.
-    new_buffer = wasm::SetupArrayBuffer(isolate, backing_store, new_size,
-                                        old_buffer->is_external());
-    memory_object->update_instances(isolate, new_buffer);
-  } else {
-    if (!MemoryGrowBuffer(isolate, old_buffer, new_size)
-             .ToHandle(&new_buffer)) {
-      return -1;
-    }
+  if (!MemoryGrowBuffer(isolate, old_buffer, new_size).ToHandle(&new_buffer)) {
+    return -1;
   }
+
   // Update instances if any.
-  memory_object->update_instances(isolate, new_buffer);
+  if (memory_object->has_instances()) {
+    Handle<WeakArrayList> instances(memory_object->instances(), isolate);
+    for (int i = 0; i < instances->length(); i++) {
+      MaybeObject elem = instances->Get(i);
+      HeapObject heap_object;
+      if (elem->GetHeapObjectIfWeak(&heap_object)) {
+        Handle<WasmInstanceObject> instance(
+            WasmInstanceObject::cast(heap_object), isolate);
+        SetInstanceMemory(instance, new_buffer);
+      } else {
+        DCHECK(elem->IsCleared());
+      }
+    }
+  }
+  memory_object->set_array_buffer(*new_buffer);
   return static_cast<uint32_t>(old_size / wasm::kWasmPageSize);
 }
 

src/wasm/wasm-objects.h

@@ -330,8 +330,6 @@ class WasmMemoryObject : public JSObject {
       Isolate* isolate, uint32_t initial, uint32_t maximum,
       bool is_shared_memory);
 
-  void update_instances(Isolate* isolate, Handle<JSArrayBuffer> buffer);
-
   static int32_t Grow(Isolate*, Handle<WasmMemoryObject>, uint32_t pages);
 
   OBJECT_CONSTRUCTORS(WasmMemoryObject, JSObject);