Bug 1591047 part 1 - Don't lock when acquiring length of shared Wasm memory. r=lth
authorRyan Hunt <rhunt@eqrion.net>
Fri, 01 Nov 2019 14:46:35 +0000
changeset 500155 911c7062f83fe4028f89bd2e70c1f6fb19210d23
parent 500154 04603499b2ea9c0668ce8da28992d1579c91518e
child 500156 2a4f822999200ebbe1600ace0c0670dd813f7a20
push id114164
push useraiakab@mozilla.com
push dateTue, 05 Nov 2019 10:06:15 +0000
treeherdermozilla-inbound@4d585c7edc76 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslth
bugs1591047
milestone72.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 1591047 part 1 - Don't lock when acquiring length of shared Wasm memory. r=lth This commit drops the requirement to acquire the shared memory lock when we are only acquiring the length. The length_ field is changed to be an Atomic<SeqCst> in the process. We still need to acquire the lock when growing the memory in order to atomically compute the new length and commit the new pages. Differential Revision: https://phabricator.services.mozilla.com/D50374
js/src/vm/SharedArrayObject.cpp
js/src/vm/SharedArrayObject.h
js/src/wasm/WasmJS.cpp
--- a/js/src/vm/SharedArrayObject.cpp
+++ b/js/src/vm/SharedArrayObject.cpp
@@ -110,23 +110,23 @@ bool SharedArrayRawBuffer::wasmGrowToSiz
   }
 
   uint32_t delta = newLength - length_;
   MOZ_ASSERT(delta % wasm::PageSize == 0);
 
   uint8_t* dataEnd = dataPointerShared().unwrap(/* for resize */) + length_;
   MOZ_ASSERT(uintptr_t(dataEnd) % gc::SystemPageSize() == 0);
 
-  // The ordering of committing memory and changing length does not matter
-  // since all clients take the lock.
-
   if (!CommitBufferMemory(dataEnd, delta)) {
     return false;
   }
 
+  // We rely on CommitBufferMemory (and therefore memmap/VirtualAlloc) to only
+  // return once it has committed memory for all threads. We only update with a
+  // new length once this has occurred.
   length_ = newLength;
 
   return true;
 }
 
 bool SharedArrayRawBuffer::addReference() {
   MOZ_RELEASE_ASSERT(refcount_ > 0);
 
--- a/js/src/vm/SharedArrayObject.h
+++ b/js/src/vm/SharedArrayObject.h
@@ -44,18 +44,18 @@ class FutexWaiter;
  * it may grow toward maxSize_.  See extensive comments above WasmArrayRawBuffer
  * in ArrayBufferObject.cpp.
  *
  * length_ only grows when the lock is held.
  */
 class SharedArrayRawBuffer {
  private:
   mozilla::Atomic<uint32_t, mozilla::ReleaseAcquire> refcount_;
-  Mutex lock_;
-  uint32_t length_;
+  mozilla::Atomic<uint32_t, mozilla::SequentiallyConsistent> length_;
+  Mutex growLock_;
   uint32_t maxSize_;
   size_t mappedSize_;  // Does not include the page for the header
   bool preparedForWasm_;
 
   // A list of structures representing tasks waiting on some
   // location within this buffer.
   FutexWaiter* waiters_;
 
@@ -64,35 +64,37 @@ class SharedArrayRawBuffer {
     MOZ_ASSERT(p.asValue() % gc::SystemPageSize() == 0);
     return p.unwrap(/* we trust you won't abuse it */);
   }
 
  protected:
   SharedArrayRawBuffer(uint8_t* buffer, uint32_t length, uint32_t maxSize,
                        size_t mappedSize, bool preparedForWasm)
       : refcount_(1),
-        lock_(mutexid::SharedArrayGrow),
         length_(length),
+        growLock_(mutexid::SharedArrayGrow),
         maxSize_(maxSize),
         mappedSize_(mappedSize),
         preparedForWasm_(preparedForWasm),
         waiters_(nullptr) {
     MOZ_ASSERT(buffer == dataPointerShared());
   }
 
  public:
   class Lock;
   friend class Lock;
 
   class MOZ_STACK_CLASS Lock {
     SharedArrayRawBuffer* buf;
 
    public:
-    explicit Lock(SharedArrayRawBuffer* buf) : buf(buf) { buf->lock_.lock(); }
-    ~Lock() { buf->lock_.unlock(); }
+    explicit Lock(SharedArrayRawBuffer* buf) : buf(buf) {
+      buf->growLock_.lock();
+    }
+    ~Lock() { buf->growLock_.unlock(); }
   };
 
   // max must be Something for wasm, Nothing for other uses
   static SharedArrayRawBuffer* Allocate(
       uint32_t length, const mozilla::Maybe<uint32_t>& maxSize,
       const mozilla::Maybe<size_t>& mappedSize);
 
   // This may be called from multiple threads.  The caller must take
@@ -104,17 +106,17 @@ class SharedArrayRawBuffer {
   void setWaiters(FutexWaiter* waiters) { waiters_ = waiters; }
 
   SharedMem<uint8_t*> dataPointerShared() const {
     uint8_t* ptr =
         reinterpret_cast<uint8_t*>(const_cast<SharedArrayRawBuffer*>(this));
     return SharedMem<uint8_t*>::shared(ptr + sizeof(SharedArrayRawBuffer));
   }
 
-  uint32_t byteLength(const Lock&) const { return length_; }
+  uint32_t volatileByteLength() const { return length_; }
 
   uint32_t maxSize() const { return maxSize_; }
 
   size_t mappedSize() const { return mappedSize_; }
 
   bool isWasm() const { return preparedForWasm_; }
 
   void tryGrowMaxSizeInPlace(uint32_t deltaMaxSize);
--- a/js/src/wasm/WasmJS.cpp
+++ b/js/src/wasm/WasmJS.cpp
@@ -1815,18 +1815,17 @@ ArrayBufferObjectMaybeShared& WasmMemory
 
 SharedArrayRawBuffer* WasmMemoryObject::sharedArrayRawBuffer() const {
   MOZ_ASSERT(isShared());
   return buffer().as<SharedArrayBufferObject>().rawBufferObject();
 }
 
 uint32_t WasmMemoryObject::volatileMemoryLength() const {
   if (isShared()) {
-    SharedArrayRawBuffer::Lock lock(sharedArrayRawBuffer());
-    return sharedArrayRawBuffer()->byteLength(lock);
+    return sharedArrayRawBuffer()->volatileByteLength();
   }
   return buffer().byteLength();
 }
 
 bool WasmMemoryObject::isShared() const {
   return buffer().is<SharedArrayBufferObject>();
 }
 
@@ -1899,18 +1898,18 @@ bool WasmMemoryObject::addMovingGrowObse
 }
 
 /* static */
 uint32_t WasmMemoryObject::growShared(HandleWasmMemoryObject memory,
                                       uint32_t delta) {
   SharedArrayRawBuffer* rawBuf = memory->sharedArrayRawBuffer();
   SharedArrayRawBuffer::Lock lock(rawBuf);
 
-  MOZ_ASSERT(rawBuf->byteLength(lock) % PageSize == 0);
-  uint32_t oldNumPages = rawBuf->byteLength(lock) / PageSize;
+  MOZ_ASSERT(rawBuf->volatileByteLength() % PageSize == 0);
+  uint32_t oldNumPages = rawBuf->volatileByteLength() / PageSize;
 
   CheckedInt<uint32_t> newSize = oldNumPages;
   newSize += delta;
   newSize *= PageSize;
   if (!newSize.isValid()) {
     return -1;
   }