[rab/gsab] Simplify BackingStore::ResizeInPlace

The logic is different for shared and non-shared memory, so it's
cleaner to have different functions for them.

Bug: v8:11111
Change-Id: I95d43b54c207b8059ea59d6d0f873623de946be3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2903152
Commit-Queue: Marja Hölttä <marja@chromium.org>
Reviewed-by: Shu-yu Guo <syg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#74656}

src/builtins/builtins-arraybuffer.cc

@@ -420,23 +420,41 @@ static Object ResizeHelper(BuiltinArguments args, Isolate* isolate,
 
   // TODO(v8:11111): Wasm integration.
 
-  // [RAB] Let oldBlock be O.[[ArrayBufferData]].
-  // [RAB] Let newBlock be ? CreateByteDataBlock(newByteLength).
-  // [RAB] Let copyLength be min(newByteLength, O.[[ArrayBufferByteLength]]).
-  // [RAB] Perform CopyDataBlockBytes(newBlock, 0, oldBlock, 0, copyLength).
-  // [RAB] NOTE: Neither creation of the new Data Block nor copying from the old
-  // Data Block are observable. Implementations reserve the right to implement
-  // this method as in-place growth or shrinkage.
-  if (!array_buffer->GetBackingStore()->ResizeInPlace(
-          isolate, new_byte_length, new_committed_pages * page_size,
-          !is_shared)) {
-    THROW_NEW_ERROR_RETURN_FAILURE(
-        isolate, NewRangeError(MessageTemplate::kInvalidArrayBufferLength));
-  }
-  // [RAB] Set O.[[ArrayBufferByteLength]] to newLength.
   if (!is_shared) {
+    // [RAB] Let oldBlock be O.[[ArrayBufferData]].
+    // [RAB] Let newBlock be ? CreateByteDataBlock(newByteLength).
+    // [RAB] Let copyLength be min(newByteLength, O.[[ArrayBufferByteLength]]).
+    // [RAB] Perform CopyDataBlockBytes(newBlock, 0, oldBlock, 0, copyLength).
+    // [RAB] NOTE: Neither creation of the new Data Block nor copying from the
+    // old Data Block are observable. Implementations reserve the right to
+    // implement this method as in-place growth or shrinkage.
+    if (array_buffer->GetBackingStore()->ResizeInPlace(
+            isolate, new_byte_length, new_committed_pages * page_size) !=
+        BackingStore::ResizeOrGrowResult::kSuccess) {
+      THROW_NEW_ERROR_RETURN_FAILURE(
+          isolate, NewRangeError(MessageTemplate::kOutOfMemory,
+                                 isolate->factory()->NewStringFromAsciiChecked(
+                                     kMethodName)));
+    }
+    // [RAB] Set O.[[ArrayBufferByteLength]] to newLength.
     array_buffer->set_byte_length(new_byte_length);
   } else {
+    // [GSAB] (Detailed description of the algorithm omitted.)
+    auto result = array_buffer->GetBackingStore()->GrowInPlace(
+        isolate, new_byte_length, new_committed_pages * page_size);
+    if (result == BackingStore::ResizeOrGrowResult::kFailure) {
+      THROW_NEW_ERROR_RETURN_FAILURE(
+          isolate, NewRangeError(MessageTemplate::kOutOfMemory,
+                                 isolate->factory()->NewStringFromAsciiChecked(
+                                     kMethodName)));
+    }
+    if (result == BackingStore::ResizeOrGrowResult::kRace) {
+      THROW_NEW_ERROR_RETURN_FAILURE(
+          isolate,
+          NewRangeError(
+              MessageTemplate::kInvalidArrayBufferResizeLength,
+              isolate->factory()->NewStringFromAsciiChecked(kMethodName)));
+    }
     // Invariant: byte_length for a GSAB is 0 (it needs to be read from the
     // BackingStore).
     CHECK_EQ(0, array_buffer->byte_length());

src/common/message-template.h

@@ -356,6 +356,7 @@ namespace internal {
   T(LetInLexicalBinding, "let is disallowed as a lexically bound name")        \
   T(LocaleMatcher, "Illegal value for localeMatcher:%")                        \
   T(NormalizationForm, "The normalization form should be one of %.")           \
+  T(OutOfMemory, "%: Out of memory")                                           \
   T(ParameterOfFunctionOutOfRange,                                             \
     "Paramenter % of function %() is % and out of range")                      \
   T(ZeroDigitNumericSeparator,                                                 \

src/objects/backing-store.cc

@@ -623,11 +623,52 @@ void BackingStore::UpdateSharedWasmMemoryObjects(Isolate* isolate) {
 }
 #endif  // V8_ENABLE_WEBASSEMBLY
 
-// Commit already reserved memory.
-bool BackingStore::ResizeInPlace(Isolate* isolate, size_t new_byte_length,
-                                 size_t new_committed_length,
-                                 bool allow_shrinking) {
+// Commit already reserved memory (for RAB backing stores (not shared)).
+BackingStore::ResizeOrGrowResult BackingStore::ResizeInPlace(
+    Isolate* isolate, size_t new_byte_length, size_t new_committed_length) {
   DCHECK_LE(new_byte_length, new_committed_length);
+  DCHECK(!is_shared());
+
+  if (new_byte_length < byte_length_) {
+    // TOOO(v8:11111): Figure out a strategy for shrinking - when do we
+    // un-commit the memory?
+
+    // Zero the memory so that in case the buffer is grown later, we have
+    // zeroed the contents already.
+    memset(reinterpret_cast<byte*>(buffer_start_) + new_byte_length, 0,
+           byte_length_ - new_byte_length);
+
+    // Changing the byte length wouldn't strictly speaking be needed, since
+    // the JSArrayBuffer already stores the updated length. This is to keep
+    // the BackingStore and JSArrayBuffer in sync.
+    byte_length_ = new_byte_length;
+    return kSuccess;
+  }
+  if (new_byte_length == byte_length_) {
+    // i::SetPermissions with size 0 fails on some platforms, so special
+    // handling for the case byte_length_ == new_byte_length == 0 is required.
+    return kSuccess;
+  }
+
+  // Try to adjust the permissions on the memory.
+  if (!i::SetPermissions(GetPlatformPageAllocator(), buffer_start_,
+                         new_committed_length, PageAllocator::kReadWrite)) {
+    return kFailure;
+  }
+
+  // Do per-isolate accounting for non-shared backing stores.
+  DCHECK(free_on_destruct_);
+  reinterpret_cast<v8::Isolate*>(isolate)
+      ->AdjustAmountOfExternalAllocatedMemory(new_byte_length - byte_length_);
+  byte_length_ = new_byte_length;
+  return kSuccess;
+}
+
+// Commit already reserved memory (for GSAB backing stores (shared)).
+BackingStore::ResizeOrGrowResult BackingStore::GrowInPlace(
+    Isolate* isolate, size_t new_byte_length, size_t new_committed_length) {
+  DCHECK_LE(new_byte_length, new_committed_length);
+  DCHECK(is_shared());
   // See comment in GrowWasmMemoryInPlace.
   // GrowableSharedArrayBuffer.prototype.grow can be called from several
   // threads. If two threads try to grow() in a racy way, the spec allows the
@@ -638,36 +679,21 @@ bool BackingStore::ResizeInPlace(Isolate* isolate, size_t new_byte_length,
   size_t old_byte_length = byte_length_.load(std::memory_order_seq_cst);
   while (true) {
     if (new_byte_length < old_byte_length) {
-      // TOOO(v8:11111): Figure out a strategy for shrinking - when do we
-      // un-commit the memory?
-      if (allow_shrinking) {
-        // This branch is only relevant for RABs and they are not shared between
-        // threads.
-        DCHECK(!is_shared());
-
-        // Zero the memory so that in case the buffer is grown later, we have
-        // zeroed the contents already.
-        memset(reinterpret_cast<byte*>(buffer_start_) + new_byte_length, 0,
-               old_byte_length - new_byte_length);
-
-        // Changing the byte length wouldn't strictly speaking be needed, since
-        // the JSArrayBuffer already stores the updated length. This is to keep
-        // the BackingStore and JSArrayBuffer in sync.
-        byte_length_ = new_byte_length;
-        return true;
-      }
-      return false;
+      // The caller checks for the new_byte_length < old_byte_length_ case. This
+      // can only happen if another thread grew the memory after that.
+      return kRace;
     }
-    if (new_byte_length == 0) {
-      DCHECK_EQ(0, old_byte_length);
-      // i::SetPermissions with size 0 fails on some platforms.
-      return true;
+    if (new_byte_length == old_byte_length) {
+      // i::SetPermissions with size 0 fails on some platforms, so special
+      // handling for the case old_byte_length == new_byte_length == 0 is
+      // required.
+      return kSuccess;
     }
 
     // Try to adjust the permissions on the memory.
     if (!i::SetPermissions(GetPlatformPageAllocator(), buffer_start_,
                            new_committed_length, PageAllocator::kReadWrite)) {
-      return false;
+      return kFailure;
     }
 
     // compare_exchange_weak updates old_byte_length.
@@ -677,14 +703,7 @@ bool BackingStore::ResizeInPlace(Isolate* isolate, size_t new_byte_length,
       break;
     }
   }
-  DCHECK(free_on_destruct_);
-  if (!is_shared_) {
-    // Only do per-isolate accounting for non-shared backing stores.
-    reinterpret_cast<v8::Isolate*>(isolate)
-        ->AdjustAmountOfExternalAllocatedMemory(new_byte_length -
-                                                old_byte_length);
-  }
-  return true;
+  return kSuccess;
 }
 
 std::unique_ptr<BackingStore> BackingStore::WrapAllocation(

src/objects/backing-store.h

@@ -97,8 +97,12 @@ class V8_EXPORT_PRIVATE BackingStore : public BackingStoreBase {
   bool has_guard_regions() const { return has_guard_regions_; }
   bool free_on_destruct() const { return free_on_destruct_; }
 
-  bool ResizeInPlace(Isolate* isolate, size_t new_byte_length,
-                     size_t new_committed_length, bool allow_shrinking);
+  enum ResizeOrGrowResult { kSuccess, kFailure, kRace };
+
+  ResizeOrGrowResult ResizeInPlace(Isolate* isolate, size_t new_byte_length,
+                                   size_t new_committed_length);
+  ResizeOrGrowResult GrowInPlace(Isolate* isolate, size_t new_byte_length,
+                                 size_t new_committed_length);
 
   // Wrapper around ArrayBuffer::Allocator::Reallocate.
   bool Reallocate(Isolate* isolate, size_t new_byte_length);

test/cctest/interpreter/bytecode_expectations/PrivateAccessorAccess.golden

@@ -83,7 +83,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -114,7 +114,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(278),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -145,7 +145,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   48 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -176,7 +176,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   41 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(278),
                 B(Star4),
                 B(LdaConstant), U8(0),
                 B(Star5),

test/cctest/interpreter/bytecode_expectations/PrivateMethodAccess.golden

@@ -56,7 +56,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   49 S> */ B(Wide), B(LdaSmi), I16(276),
+  /*   49 S> */ B(Wide), B(LdaSmi), I16(277),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -88,7 +88,7 @@ bytecodes: [
                 B(Mov), R(this), R(0),
                 B(Mov), R(context), R(2),
   /*   44 E> */ B(CallRuntime), U16(Runtime::kAddPrivateBrand), R(0), U8(3),
-  /*   49 S> */ B(Wide), B(LdaSmi), I16(276),
+  /*   49 S> */ B(Wide), B(LdaSmi), I16(277),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),

test/cctest/interpreter/bytecode_expectations/StaticPrivateMethodAccess.golden

@@ -24,7 +24,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(1),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(274),
+                B(Wide), B(LdaSmi), I16(275),
                 B(Star2),
                 B(LdaConstant), U8(0),
                 B(Star3),
@@ -55,7 +55,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   56 S> */ B(Wide), B(LdaSmi), I16(276),
+  /*   56 S> */ B(Wide), B(LdaSmi), I16(277),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -82,7 +82,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   56 S> */ B(Wide), B(LdaSmi), I16(276),
+  /*   56 S> */ B(Wide), B(LdaSmi), I16(277),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -121,7 +121,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(274),
+                B(Wide), B(LdaSmi), I16(275),
                 B(Star2),
                 B(LdaConstant), U8(0),
                 B(Star3),
@@ -143,7 +143,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(1),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(275),
+                B(Wide), B(LdaSmi), I16(276),
                 B(Star3),
                 B(LdaConstant), U8(0),
                 B(Star4),
@@ -158,7 +158,7 @@ bytecodes: [
                 B(TestReferenceEqual), R(this),
                 B(Mov), R(this), R(0),
                 B(JumpIfTrue), U8(16),
-                B(Wide), B(LdaSmi), I16(274),
+                B(Wide), B(LdaSmi), I16(275),
                 B(Star2),
                 B(LdaConstant), U8(0),
                 B(Star3),
@@ -188,7 +188,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   60 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   60 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -214,7 +214,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   53 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   53 S> */ B(Wide), B(LdaSmi), I16(278),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -240,7 +240,7 @@ frame size: 2
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   60 S> */ B(Wide), B(LdaSmi), I16(278),
+  /*   60 S> */ B(Wide), B(LdaSmi), I16(279),
                 B(Star0),
                 B(LdaConstant), U8(0),
                 B(Star1),
@@ -266,7 +266,7 @@ frame size: 3
 parameter count: 1
 bytecode array length: 14
 bytecodes: [
-  /*   46 S> */ B(Wide), B(LdaSmi), I16(277),
+  /*   46 S> */ B(Wide), B(LdaSmi), I16(278),
                 B(Star1),
                 B(LdaConstant), U8(0),
                 B(Star2),