Bug 1277709 - Make threadsafe reference counting use the minimum memory sychronization needed. r=froydnj
☠☠ backed out by 2ba7fac730ed ☠ ☠
authorL. David Baron <dbaron@dbaron.org>
Mon, 03 Apr 2017 20:43:29 -0700
changeset 399205 a52e75fdda073ca38e2a88b91ad7070c4138d702
parent 399204 8a50c2d76afcdfa17697812b57de60304125903b
child 399206 8f8e8cd713ad1d08e756c329b45004fa9cea1946
push id7391
push usermtabara@mozilla.com
push dateMon, 12 Jun 2017 13:08:53 +0000
treeherdermozilla-beta@2191d7f87e2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1277709
milestone55.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 1277709 - Make threadsafe reference counting use the minimum memory sychronization needed. r=froydnj This uses std::atomic rather than mozilla::Atomic since mozilla::Atomic does not support using different memory synchronization for different atomic operations on the same variable. The added comments could use careful review since, while they reflect my understanding of the issue, I don't consider myself an expert on the topic. MozReview-Commit-ID: 7xByCXt17Dr
modules/libpref/Preferences.h
xpcom/base/nsISupportsImpl.h
--- a/modules/libpref/Preferences.h
+++ b/modules/libpref/Preferences.h
@@ -12,16 +12,17 @@
 
 #include "nsIPrefService.h"
 #include "nsIPrefBranch.h"
 #include "nsIPrefBranchInternal.h"
 #include "nsIObserver.h"
 #include "nsCOMPtr.h"
 #include "nsTArray.h"
 #include "nsWeakReference.h"
+#include "mozilla/Atomics.h"
 #include "mozilla/MemoryReporting.h"
 
 class nsIFile;
 class nsAdoptingString;
 class nsAdoptingCString;
 
 #ifndef have_PrefChangedFunc_typedef
 typedef void (*PrefChangedFunc)(const char *, void *);
--- a/xpcom/base/nsISupportsImpl.h
+++ b/xpcom/base/nsISupportsImpl.h
@@ -15,17 +15,17 @@
 
 
 #if !defined(XPCOM_GLUE_AVOID_NSPR)
 #include "prthread.h" /* needed for thread-safety checks */
 #endif // !XPCOM_GLUE_AVOID_NSPR
 
 #include "nsDebug.h"
 #include "nsXPCOM.h"
-#include "mozilla/Atomics.h"
+#include <atomic>
 #include "mozilla/Attributes.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Compiler.h"
 #include "mozilla/Likely.h"
 #include "mozilla/MacroArgs.h"
 #include "mozilla/MacroForEach.h"
 #include "mozilla/TypeTraits.h"
 
@@ -302,34 +302,65 @@ class ThreadSafeAutoRefCnt
 public:
   ThreadSafeAutoRefCnt() : mValue(0) {}
   explicit ThreadSafeAutoRefCnt(nsrefcnt aValue) : mValue(aValue) {}
 
   ThreadSafeAutoRefCnt(const ThreadSafeAutoRefCnt&) = delete;
   void operator=(const ThreadSafeAutoRefCnt&) = delete;
 
   // only support prefix increment/decrement
-  MOZ_ALWAYS_INLINE nsrefcnt operator++() { return ++mValue; }
-  MOZ_ALWAYS_INLINE nsrefcnt operator--() { return --mValue; }
+  MOZ_ALWAYS_INLINE nsrefcnt operator++()
+  {
+    // Memory synchronization is not required when incrementing a
+    // reference count.  The first increment of a reference count on a
+    // thread is not important, since the first use of the object on a
+    // thread can happen before it.  What is important is the transfer
+    // of the pointer to that thread, which may happen prior to the
+    // first increment on that thread.  The necessary memory
+    // synchronization is done by the mechanism that transfers the
+    // pointer between threads.
+    return mValue.fetch_add(1, std::memory_order_relaxed) + 1;
+  }
+  MOZ_ALWAYS_INLINE nsrefcnt operator--()
+  {
+    // Since this may be the last release on this thread, we need
+    // release semantics so that prior writes on this thread are visible
+    // to the thread that destroys the object when it reads mValue with
+    // acquire semantics.
+    nsrefcnt result = mValue.fetch_sub(1, std::memory_order_release) - 1;
+    if (result == 0) {
+      // We're going to destroy the object on this thread, so we need
+      // acquire semantics to synchronize with the memory released by
+      // the last release on other threads, that is, to ensure that
+      // writes prior to that release are now visible on this thread.
+      result = mValue.load(std::memory_order_acquire);
+    }
+    return result;
+  }
 
   MOZ_ALWAYS_INLINE nsrefcnt operator=(nsrefcnt aValue)
   {
-    return (mValue = aValue);
+    // Use release semantics since we're not sure what the caller is
+    // doing.
+    mValue.store(aValue, std::memory_order_release);
+    return aValue;
   }
-  MOZ_ALWAYS_INLINE operator nsrefcnt() const { return mValue; }
-  MOZ_ALWAYS_INLINE nsrefcnt get() const { return mValue; }
+  MOZ_ALWAYS_INLINE operator nsrefcnt() const { return get(); }
+  MOZ_ALWAYS_INLINE nsrefcnt get() const
+  {
+    // Use acquire semantics since we're not sure what the caller is
+    // doing.
+    return mValue.load(std::memory_order_acquire);
+  }
 
   static const bool isThreadSafe = true;
 private:
   nsrefcnt operator++(int) = delete;
   nsrefcnt operator--(int) = delete;
-  // In theory, RelaseAcquire consistency (but no weaker) is sufficient for
-  // the counter. Making it weaker could speed up builds on ARM (but not x86),
-  // but could break pre-existing code that assumes sequential consistency.
-  Atomic<nsrefcnt> mValue;
+  std::atomic<nsrefcnt> mValue;
 };
 } // namespace mozilla
 
 ///////////////////////////////////////////////////////////////////////////////
 
 /**
  * Declare the reference count variable and the implementations of the
  * AddRef and QueryInterface methods.