Bug 1529298 - Now that |ownsData()| is always true, inline that true value into all callers. r=sfink
authorJeff Walden <jwalden@mit.edu>
Wed, 20 Feb 2019 13:33:17 -0800
changeset 461624 650bd5a18809655323834c7b56d84ad6c512d752
parent 461623 ef4c27821811d19a0dc64e4d9b4d2e7841bb1370
child 461625 694fe0c4379319644cd9f64781fcb71a75558ed5
push id35626
push usercsabou@mozilla.com
push dateThu, 28 Feb 2019 11:31:08 +0000
treeherdermozilla-central@2ea0c1db7e60 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssfink
bugs1529298
milestone67.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1529298 - Now that |ownsData()| is always true, inline that true value into all callers. r=sfink
js/src/vm/ArrayBufferObject.cpp
js/src/vm/ArrayBufferObject.h
--- a/js/src/vm/ArrayBufferObject.cpp
+++ b/js/src/vm/ArrayBufferObject.cpp
@@ -522,26 +522,22 @@ static uint8_t* NewCopiedBufferContents(
     buffer->setNewData(cx->runtime()->defaultFreeOp(), newContents);
   }
 
   buffer->setByteLength(0);
   buffer->setIsDetached();
 }
 
 void ArrayBufferObject::setNewData(FreeOp* fop, BufferContents newContents) {
-  if (ownsData()) {
-    // XXX As data kinds gradually transition to *all* being owned, this
-    //     assertion could become troublesome.  But right *now*, it isn't --
-    //     the call just above in ABO::detach occurs only if this assertion is
-    //     true, and the call in ABO::changeContents happens only if
-    //     ABO::prepareForAsmJS calls ABO::changeContents, but *that* call is
-    //     guarded by an owns-data check.  So no need to touch this right now.
-    MOZ_ASSERT(newContents.data() != dataPointer());
-    releaseData(fop);
-  }
+  // XXX All callers of this check are changing data pointer, so this assertion
+  //     is fine.  But we might consider just inlining these steps into the
+  //     callers, making the release-operation for changing data pointer
+  //     explicit everywhere it occurs.
+  MOZ_ASSERT(newContents.data() != dataPointer());
+  releaseData(fop);
 
   setDataPointer(newContents);
 }
 
 // This is called *only* from changeContents(), below.
 // By construction, every view parameter will be mapping unshared memory (an
 // ArrayBuffer). Hence no reason to worry about shared memory here.
 
@@ -968,33 +964,17 @@ bool js::CreateWasmBuffer(JSContext* cx,
   if (buffer->hasUserOwnedData()) {
     MOZ_ASSERT(!buffer->isPreparedForAsmJS());
     return false;
   }
 
   MOZ_ASSERT(buffer->isMalloced() || buffer->isMapped() ||
              buffer->isExternal());
 
-  // Buffers already prepared for asm.js need no further work.
-  if (buffer->isPreparedForAsmJS()) {
-    return true;
-  }
-
-  // XXX User-owned, no-data, and is-inline were excluded above, so
-  //     |ownsData()| having changed semantics in this patch doesn't matter
-  //     here.
-  if (!buffer->ownsData()) {
-    uint8_t* data = NewCopiedBufferContents(cx, buffer);
-    if (!data) {
-      return false;
-    }
-
-    buffer->changeContents(cx, BufferContents::createMalloced(data));
-  }
-
+  // It's fine if this uselessly sets the flag a second time.
   buffer->setIsPreparedForAsmJS();
   return true;
 }
 
 ArrayBufferObject::BufferContents ArrayBufferObject::createMappedContents(
     int fd, size_t offset, size_t length) {
   void* data =
       gc::AllocateMappedContent(fd, offset, length, ARRAY_BUFFER_ALIGNMENT);
@@ -1014,18 +994,16 @@ SharedMem<uint8_t*> ArrayBufferObject::d
 }
 
 ArrayBufferObject::FreeInfo* ArrayBufferObject::freeInfo() const {
   MOZ_ASSERT(isExternal());
   return reinterpret_cast<FreeInfo*>(inlineDataPointer());
 }
 
 void ArrayBufferObject::releaseData(FreeOp* fop) {
-  MOZ_ASSERT(ownsData());
-
   switch (bufferKind()) {
     case INLINE_DATA:
       // Inline data doesn't require releasing.
       break;
     case MALLOCED:
       fop->free_(dataPointer());
       break;
     case NO_DATA:
@@ -1389,31 +1367,29 @@ ArrayBufferObject* ArrayBufferObject::cr
   return buffer;
 }
 
 /* static */ uint8_t* ArrayBufferObject::stealMallocedContents(
     JSContext* cx, Handle<ArrayBufferObject*> buffer) {
   CheckStealPreconditions(buffer, cx);
 
   switch (buffer->bufferKind()) {
-    case MALLOCED:
-      if (buffer->ownsData()) {
-        uint8_t* stolenData = buffer->dataPointer();
-        MOZ_ASSERT(stolenData);
+    case MALLOCED: {
+      uint8_t* stolenData = buffer->dataPointer();
+      MOZ_ASSERT(stolenData);
 
-        // Overwrite the old data pointer *without* releasing the contents
-        // being stolen.
-        BufferContents newContents = BufferContents::createNoData();
-        buffer->setDataPointer(newContents);
+      // Overwrite the old data pointer *without* releasing the contents
+      // being stolen.
+      BufferContents newContents = BufferContents::createNoData();
+      buffer->setDataPointer(newContents);
 
-        // Detach |buffer| now that doing so won't free |stolenData|.
-        ArrayBufferObject::detach(cx, buffer, newContents);
-        return stolenData;
-      }
-      MOZ_FALLTHROUGH;
+      // Detach |buffer| now that doing so won't free |stolenData|.
+      ArrayBufferObject::detach(cx, buffer, newContents);
+      return stolenData;
+    }
 
     case INLINE_DATA:
     case NO_DATA:
     case USER_OWNED:
     case MAPPED:
     case EXTERNAL: {
       // We can't use these data types directly.  Make a copy to return.
       uint8_t* copiedData = NewCopiedBufferContents(cx, buffer);
@@ -1494,21 +1470,16 @@ ArrayBufferObject::extractStructuredClon
 
   MOZ_ASSERT_UNREACHABLE("garbage kind computed");
   return BufferContents::createFailed();
 }
 
 /* static */ void ArrayBufferObject::addSizeOfExcludingThis(
     JSObject* obj, mozilla::MallocSizeOf mallocSizeOf, JS::ClassInfo* info) {
   ArrayBufferObject& buffer = AsArrayBuffer(obj);
-
-  if (!buffer.ownsData()) {
-    return;
-  }
-
   switch (buffer.bufferKind()) {
     case INLINE_DATA:
       // Inline data's size should be reported by this object's size-class
       // reporting.
       break;
     case MALLOCED:
       if (buffer.isPreparedForAsmJS()) {
         info->objectsMallocHeapElementsAsmJS +=
@@ -1537,21 +1508,17 @@ ArrayBufferObject::extractStructuredClon
       MOZ_CRASH("external buffers not currently supported");
       break;
     case BAD1:
       MOZ_CRASH("bad bufferKind()");
   }
 }
 
 /* static */ void ArrayBufferObject::finalize(FreeOp* fop, JSObject* obj) {
-  ArrayBufferObject& buffer = obj->as<ArrayBufferObject>();
-
-  if (buffer.ownsData()) {
-    buffer.releaseData(fop);
-  }
+  obj->as<ArrayBufferObject>().releaseData(fop);
 }
 
 /* static */ void ArrayBufferObject::copyData(
     Handle<ArrayBufferObject*> toBuffer, uint32_t toIndex,
     Handle<ArrayBufferObject*> fromBuffer, uint32_t fromIndex, uint32_t count) {
   MOZ_ASSERT(toBuffer->byteLength() >= count);
   MOZ_ASSERT(toBuffer->byteLength() >= toIndex + count);
   MOZ_ASSERT(fromBuffer->byteLength() >= fromIndex);
@@ -1766,56 +1733,18 @@ JS_FRIEND_API bool JS_DetachArrayBuffer(
   Rooted<ArrayBufferObject*> buffer(cx, &obj->as<ArrayBufferObject>());
 
   if (buffer->isWasm() || buffer->isPreparedForAsmJS()) {
     JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                               JSMSG_WASM_NO_TRANSFER);
     return false;
   }
 
-  using BufferContents = ArrayBufferObject::BufferContents;
-
-  // This patch makes NO_DATA, INLINE_DATA, and USER_OWNED all be owned.  I
-  // argue this change should be safe.  First, the easy NO_DATA case:
-  //
-  //   * NO_DATA in ABO::detach will see that no data pointer change occurs, so
-  //     it won't call ABO::setNewData and will leave this exactly as it was
-  //     (save for view-notification and byte length and detachment changing).
-  //     This is trivially fine.
-  //
-  // Making INLINE_DATA and USER_OWNED be owned, if we keep doing the same
-  // steps, *will* change stuff.  These pre-states
-  //
-  //   (user-owned pointer, OwnsData, USER_OWNED | remaining)
-  //   (inline data pointer, OwnsData, INLINE_DATA | remaining)
-  //
-  // will change to
-  //
-  //   (nullptr, OwnsData, NO_DATA | remaining)
-  //
-  // This changes the behavior of JS_GetArrayBufferData, but the calling code
-  // already has to be cognizant of byte length if it might index into that
-  // pointer, so all that changes is the pointer's plain identity, and it seems
-  // okay to say "don't do that".  Ownership state is unchanged.  And the
-  // kind-change shouldn't matter because we only test
-  // |buffer->hasUserOwnedData()| in ABO::prepareForAsmJS -- but only *after*
-  // anything length-zero would have been filtered out, so no change there.
-  // Nothing tests for |isInlineData()| except assertions.
-  //
-  // With |ownsData()| becoming increasingly true for everything (at present
-  // there's only one way to have |!ownsData()|, soon this can just be
-  // |BufferContents::createNoData()| in all cases.  (And, of course, this
-  // comment and a few others like it can then die.  XXX)
-  BufferContents newContents =
-      buffer->ownsData()
-          ? BufferContents::createNoData()
-          : buffer->contents();
-
-  ArrayBufferObject::detach(cx, buffer, newContents);
-
+  ArrayBufferObject::detach(cx, buffer,
+                            ArrayBufferObject::BufferContents::createNoData());
   return true;
 }
 
 JS_FRIEND_API bool JS_IsDetachedArrayBufferObject(JSObject* obj) {
   ArrayBufferObject* aobj = obj->maybeUnwrapIf<ArrayBufferObject>();
   if (!aobj) {
     return false;
   }
--- a/js/src/vm/ArrayBufferObject.h
+++ b/js/src/vm/ArrayBufferObject.h
@@ -441,18 +441,16 @@ class ArrayBufferObject : public ArrayBu
 
   static BufferContents createMappedContents(int fd, size_t offset,
                                              size_t length);
 
   static size_t offsetOfDataSlot() { return getFixedSlotOffset(DATA_SLOT); }
 
   void setHasTypedObjectViews() { setFlags(flags() | TYPED_OBJECT_VIEWS); }
 
-  bool ownsData() const { return flags() & OWNS_DATA; }
-
  protected:
   void setDataPointer(BufferContents contents);
   void setByteLength(uint32_t length);
 
   uint32_t flags() const;
   void setFlags(uint32_t flags);
 
   void setOwnsData(OwnsState owns) {