Bug 1469290 - Avoid extra locks in NS_ReadInputStreamToBuffer, r=valentin
authorAndrea Marchesini <amarchesini@mozilla.com>
Mon, 18 Jun 2018 19:41:29 -0400
changeset 479661 f194f3fb2e6e437df86a910fa5f102e11fe3d188
parent 479660 289ce1ae93b6c5f08f3e65c1997312bd901b101c
child 479662 81bb56f69788224c30d62dc42e60004ecb5fc860
push id1757
push userffxbld-merge
push dateFri, 24 Aug 2018 17:02:43 +0000
treeherdermozilla-release@736023aebdb1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin
bugs1469290
milestone62.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 1469290 - Avoid extra locks in NS_ReadInputStreamToBuffer, r=valentin
netwerk/base/nsNetUtil.cpp
--- a/netwerk/base/nsNetUtil.cpp
+++ b/netwerk/base/nsNetUtil.cpp
@@ -1517,43 +1517,41 @@ NS_NewBufferedInputStream(nsIInputStream
     }
     return rv;
 }
 
 namespace {
 
 #define BUFFER_SIZE 8192
 
-class BufferWriter final : public Runnable
-                         , public nsIInputStreamCallback
+class BufferWriter final : public nsIInputStreamCallback
 {
 public:
-    NS_DECL_ISUPPORTS_INHERITED
+    NS_DECL_THREADSAFE_ISUPPORTS
 
     BufferWriter(nsIInputStream* aInputStream,
                  void* aBuffer, int64_t aCount)
-        : Runnable("BufferWriterRunnable")
-        , mMonitor("BufferWriter.mMonitor")
+        : mMonitor("BufferWriter.mMonitor")
         , mInputStream(aInputStream)
         , mBuffer(aBuffer)
         , mCount(aCount)
         , mWrittenData(0)
         , mBufferType(aBuffer ? eExternal : eInternal)
-        , mAsyncResult(NS_OK)
         , mBufferSize(0)
-        , mCompleted(false)
     {
         MOZ_ASSERT(aInputStream);
         MOZ_ASSERT(aCount == -1 || aCount > 0);
         MOZ_ASSERT_IF(mBuffer, aCount > 0);
     }
 
     nsresult
     Write()
     {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
+
         // Let's make the inputStream buffered if it's not.
         if (!NS_InputStreamIsBuffered(mInputStream)) {
             nsCOMPtr<nsIInputStream> bufferedStream;
             nsresult rv =
                 NS_NewBufferedInputStream(getter_AddRefs(bufferedStream),
                                           mInputStream.forget(), BUFFER_SIZE);
             NS_ENSURE_SUCCESS(rv, rv);
 
@@ -1570,26 +1568,26 @@ public:
         mInputStream = nullptr;
 
         return WriteAsync();
     }
 
     uint64_t
     WrittenData() const
     {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
         return mWrittenData;
     }
 
     void*
     StealBuffer()
     {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
         MOZ_ASSERT(mBufferType == eInternal);
 
-        MonitorAutoLock lock(mMonitor);
-
         void* buffer = mBuffer;
 
         mBuffer = nullptr;
         mBufferSize = 0;
 
         return buffer;
     }
 
@@ -1603,16 +1601,18 @@ private:
         if (mTaskQueue) {
             mTaskQueue->BeginShutdown();
         }
     }
 
     nsresult
     WriteSync()
     {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
+
         uint64_t length = (uint64_t)mCount;
 
         if (mCount == -1) {
             nsresult rv = mInputStream->Available(&length);
             NS_ENSURE_SUCCESS(rv, rv);
 
             if (length == 0) {
                 // nothing to read.
@@ -1635,135 +1635,115 @@ private:
 
         mWrittenData = writtenData;
         return NS_OK;
     }
 
     nsresult
     WriteAsync()
     {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
+
         if (mCount > 0 && mBufferType == eInternal) {
             mBuffer = malloc(mCount);
             if (NS_WARN_IF(!mBuffer)) {
                 return NS_ERROR_OUT_OF_MEMORY;
             }
         }
 
-        nsCOMPtr<nsIEventTarget> target =
-            do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
-        if (!target) {
-            return NS_ERROR_FAILURE;
-        }
-
-        mTaskQueue = new TaskQueue(target.forget());
-
-        MonitorAutoLock lock(mMonitor);
-
-        nsCOMPtr<nsIRunnable> runnable = this;
-        nsresult rv = mTaskQueue->Dispatch(runnable.forget());
-        NS_ENSURE_SUCCESS(rv, rv);
-
-        lock.Wait();
-
-        mCompleted = true;
-        return mAsyncResult;
-    }
-
-    // This method runs on the I/O Thread when the owning thread is blocked by
-    // the mMonitor. It is called multiple times until mCount is greater than 0
-    // or until there is something to read in the stream.
-    NS_IMETHOD
-    Run() override
-    {
-        MOZ_ASSERT(mAsyncInputStream);
-        MOZ_ASSERT(!mInputStream);
-
-        MonitorAutoLock lock(mMonitor);
-
         while (true) {
-            if (mCompleted) {
-                return NS_OK;
-            }
-
-            if (mCount == 0) {
-                OperationCompleted(lock, NS_OK);
-                return NS_OK;
-            }
-
             if (mCount == -1 && !MaybeExpandBufferSize()) {
-                OperationCompleted(lock, NS_ERROR_OUT_OF_MEMORY);
-                return NS_OK;
+                return NS_ERROR_OUT_OF_MEMORY;
             }
 
             uint64_t offset = mWrittenData;
             uint64_t length = mCount == -1 ? BUFFER_SIZE : mCount;
 
-            // Let's try to read it directly.
+            // Let's try to read data directly.
             uint32_t writtenData;
             nsresult rv = mAsyncInputStream->ReadSegments(NS_CopySegmentToBuffer,
                                                          static_cast<char*>(mBuffer) + offset,
                                                          length, &writtenData);
 
-            // Operation completed.
+            // Operation completed. Nothing more to read.
             if (NS_SUCCEEDED(rv) && writtenData == 0) {
-                OperationCompleted(lock, NS_OK);
                 return NS_OK;
             }
 
             // If we succeeded, let's try to read again.
             if (NS_SUCCEEDED(rv)) {
                 mWrittenData += writtenData;
                 if (mCount != -1) {
                     MOZ_ASSERT(mCount >= writtenData);
                     mCount -= writtenData;
                 }
 
                 continue;
             }
 
             // Async wait...
             if (rv == NS_BASE_STREAM_WOULD_BLOCK) {
+                rv = MaybeCreateTaskQueue();
+                if (NS_WARN_IF(NS_FAILED(rv))) {
+                    return rv;
+                }
+
+                MonitorAutoLock lock(mMonitor);
+
                 rv = mAsyncInputStream->AsyncWait(this, 0, length, mTaskQueue);
                 if (NS_WARN_IF(NS_FAILED(rv))) {
-                    OperationCompleted(lock, rv);
+                    return rv;
                 }
-                return NS_OK;
+
+                lock.Wait();
+                continue;
             }
 
-            // Error.
-            OperationCompleted(lock, rv);
-            return NS_OK;
+            // Otherwise, let's propagate the error.
+            return rv;
         }
 
         MOZ_ASSERT_UNREACHABLE("We should not be here");
         return NS_ERROR_FAILURE;
     }
 
+    nsresult
+    MaybeCreateTaskQueue()
+    {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
+
+        if (!mTaskQueue) {
+            nsCOMPtr<nsIEventTarget> target =
+                do_GetService(NS_STREAMTRANSPORTSERVICE_CONTRACTID);
+            if (!target) {
+                return NS_ERROR_FAILURE;
+            }
+
+            mTaskQueue = new TaskQueue(target.forget());
+        }
+
+        return NS_OK;
+    }
+
     NS_IMETHOD
     OnInputStreamReady(nsIAsyncInputStream* aStream) override
     {
-        MOZ_ASSERT(aStream == mAsyncInputStream);
-        // The stream is ready, let's read it again.
-        return Run();
-    }
-
-    // This function is called from the I/O thread and it will unblock the
-    // owning thread.
-    void
-    OperationCompleted(MonitorAutoLock& aLock, nsresult aRv)
-    {
-        mAsyncResult = aRv;
-
-        // This will unlock the owning thread.
-        aLock.Notify();
+        MOZ_ASSERT(!NS_IsMainThread());
+
+        // We have something to read. Let's unlock the main-thread.
+        MonitorAutoLock lock(mMonitor);
+        lock.Notify();
+        return NS_OK;
     }
 
     bool
     MaybeExpandBufferSize()
     {
+        NS_ASSERT_OWNINGTHREAD(BufferWriter);
+
         MOZ_ASSERT(mCount == -1);
 
         if (mBufferSize >= mWrittenData + BUFFER_SIZE) {
             // The buffer is big enough.
             return true;
         }
 
         CheckedUint32 bufferSize =
@@ -1783,16 +1763,18 @@ private:
             return false;
         }
 
         mBuffer = buffer;
         mBufferSize = bufferSize.value();
         return true;
     }
 
+    // All the members of this class are touched on the owning thread only. The
+    // monitor is only used to communicate when there is more data to read.
     Monitor mMonitor;
 
     nsCOMPtr<nsIInputStream> mInputStream;
     nsCOMPtr<nsIAsyncInputStream> mAsyncInputStream;
 
     RefPtr<TaskQueue> mTaskQueue;
 
     void* mBuffer;
@@ -1804,22 +1786,20 @@ private:
         // in the DTOR if not stolen. The buffer can be reallocated.
         eInternal,
 
         // The buffer is not owned by this object and it cannot be reallocated.
         eExternal,
     } mBufferType;
 
     // The following set if needed for the async read.
-    nsresult mAsyncResult;
     uint64_t mBufferSize;
-    Atomic<bool> mCompleted;
 };
 
-NS_IMPL_ISUPPORTS_INHERITED(BufferWriter, Runnable, nsIInputStreamCallback)
+NS_IMPL_ISUPPORTS(BufferWriter, nsIInputStreamCallback)
 
 } // anonymous namespace
 
 nsresult
 NS_ReadInputStreamToBuffer(nsIInputStream* aInputStream,
                            void** aDest,
                            int64_t aCount,
                            uint64_t* aWritten)