Bug 1502207 Part 4 - Relax ordering requirements for atomic accesses to be specific to the atomic value itself, r=froydnj.
authorBrian Hackett <bhackett1024@gmail.com>
Thu, 25 Oct 2018 11:46:07 -1000
changeset 443266 b31239e8fbe3b47c84aa1ca7dcc28ef0759b37d7
parent 443265 e911b99c9c083417d46dd2b24e7933430f4d392a
child 443267 9c07484c05c8891b6d22f2e1dc73261a73ac829d
push id109332
push userbhackett@mozilla.com
push dateSun, 28 Oct 2018 22:24:50 +0000
treeherdermozilla-inbound@b31239e8fbe3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1502207
milestone65.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 1502207 Part 4 - Relax ordering requirements for atomic accesses to be specific to the atomic value itself, r=froydnj.
toolkit/recordreplay/Lock.cpp
toolkit/recordreplay/ProcessRecordReplay.cpp
toolkit/recordreplay/ProcessRecordReplay.h
toolkit/recordreplay/Thread.h
--- a/toolkit/recordreplay/Lock.cpp
+++ b/toolkit/recordreplay/Lock.cpp
@@ -11,23 +11,18 @@
 #include "SpinLock.h"
 #include "Thread.h"
 
 #include <unordered_map>
 
 namespace mozilla {
 namespace recordreplay {
 
-// The total number of locks that have been created. Reserved IDs:
-// 0: Locks that are not recorded.
-// 1: Used by gAtomicLock for atomic accesses.
-//
-// This is only used while recording, and increments gradually as locks are
-// created.
-static const size_t gAtomicLockId = 1;
+// The total number of locks that have been created. Each Lock is given a
+// non-zero id based on this counter.
 static Atomic<size_t, SequentiallyConsistent, Behavior::DontPreserve> gNumLocks;
 
 struct LockAcquires
 {
   // List of thread acquire orders for the lock. This is protected by the lock
   // itself.
   Stream* mAcquires;
 
@@ -58,16 +53,29 @@ static ChunkAllocator<LockAcquires> gLoc
 ///////////////////////////////////////////////////////////////////////////////
 
 // Table mapping native lock pointers to the associated Lock structure, for
 // every recorded lock in existence.
 typedef std::unordered_map<void*, Lock*> LockMap;
 static LockMap* gLocks;
 static ReadWriteSpinLock gLocksLock;
 
+static Lock*
+CreateNewLock(Thread* aThread, size_t aId)
+{
+  LockAcquires* info = gLockAcquires.Create(aId);
+  info->mAcquires = gRecordingFile->OpenStream(StreamName::Lock, aId);
+
+  if (IsReplaying()) {
+    info->ReadAndNotifyNextOwner(aThread);
+  }
+
+  return new Lock(aId);
+}
+
 /* static */ void
 Lock::New(void* aNativeLock)
 {
   Thread* thread = Thread::Current();
   RecordingEventSection res(thread);
   if (!res.CanAccessEvents()) {
     Destroy(aNativeLock); // Clean up any old lock, as below.
     return;
@@ -76,35 +84,30 @@ Lock::New(void* aNativeLock)
   thread->Events().RecordOrReplayThreadEvent(ThreadEvent::CreateLock);
 
   size_t id;
   if (IsRecording()) {
     id = gNumLocks++;
   }
   thread->Events().RecordOrReplayScalar(&id);
 
-  LockAcquires* info = gLockAcquires.Create(id);
-  info->mAcquires = gRecordingFile->OpenStream(StreamName::Lock, id);
-
-  if (IsReplaying()) {
-    info->ReadAndNotifyNextOwner(thread);
-  }
+  Lock* lock = CreateNewLock(thread, id);
 
   // Tolerate new locks being created with identical pointers, even if there
   // was no explicit Destroy() call for the old one.
   Destroy(aNativeLock);
 
   AutoWriteSpinLock ex(gLocksLock);
   thread->BeginDisallowEvents();
 
   if (!gLocks) {
     gLocks = new LockMap();
   }
 
-  gLocks->insert(LockMap::value_type(aNativeLock, new Lock(id)));
+  gLocks->insert(LockMap::value_type(aNativeLock, lock));
 
   thread->EndDisallowEvents();
 }
 
 /* static */ void
 Lock::Destroy(void* aNativeLock)
 {
   Lock* lock = nullptr;
@@ -196,65 +199,111 @@ Lock::Exit()
     mOwner = 0;
 
     // Notify the next owner before releasing the lock.
     LockAcquires* acquires = gLockAcquires.Get(mId);
     acquires->ReadAndNotifyNextOwner(thread);
   }
 }
 
-struct AtomicLock : public detail::MutexImpl
-{
-  using detail::MutexImpl::lock;
-  using detail::MutexImpl::unlock;
-};
-
-// Lock which is held during code sections that run atomically.
-static AtomicLock* gAtomicLock = nullptr;
-
-/* static */ void
-Lock::InitializeLocks()
-{
-  gNumLocks = gAtomicLockId;
-  gAtomicLock = new AtomicLock();
-
-  AssertEventsAreNotPassedThrough();
-
-  // There should be exactly one recorded lock right now, unless we had an
-  // initialization failure and didn't record the lock just created.
-  MOZ_RELEASE_ASSERT(!IsRecording() ||
-                     gNumLocks == gAtomicLockId + 1 ||
-                     gInitializationFailureMessage);
-}
-
 /* static */ void
 Lock::LockAquiresUpdated(size_t aLockId)
 {
   LockAcquires* acquires = gLockAcquires.MaybeGet(aLockId);
   if (acquires && acquires->mAcquires && acquires->mNextOwner == LockAcquires::NoNextOwner) {
     acquires->ReadAndNotifyNextOwner(Thread::Current());
   }
 }
 
+// We use a set of Locks to record and replay the order in which atomic
+// accesses occur. Each lock describes the acquire order for a disjoint set of
+// values; this is done to reduce contention between threads, and ensures that
+// when the same value pointer is used in two ordered atomic accesses, those
+// accesses will replay in the same order as they did while recording.
+// Instead of using platform mutexes, we manage the Locks directly to avoid
+// overhead in Lock::Find. Atomics accesses are a major source of recording
+// overhead, which we want to minimize.
+static const size_t NumAtomicLocks = 89;
+static Lock** gAtomicLocks;
+
+// While recording, these locks prevent multiple threads from simultaneously
+// owning the same atomic lock.
+static SpinLock* gAtomicLockOwners;
+
+/* static */ void
+Lock::InitializeLocks()
+{
+  Thread* thread = Thread::Current();
+
+  gNumLocks = 1;
+  gAtomicLocks = new Lock*[NumAtomicLocks];
+  for (size_t i = 0; i < NumAtomicLocks; i++) {
+    gAtomicLocks[i] = CreateNewLock(thread, gNumLocks++);
+  }
+  if (IsRecording()) {
+    gAtomicLockOwners = new SpinLock[NumAtomicLocks];
+    PodZero(gAtomicLockOwners, NumAtomicLocks);
+  }
+}
+
 extern "C" {
 
 MOZ_EXPORT void
-RecordReplayInterface_InternalBeginOrderedAtomicAccess()
+RecordReplayInterface_InternalBeginOrderedAtomicAccess(const void* aValue)
 {
   MOZ_RELEASE_ASSERT(IsRecordingOrReplaying());
-  if (!gInitializationFailureMessage) {
-    gAtomicLock->lock();
+
+  Thread* thread = Thread::Current();
+
+  // Determine which atomic lock to use for this access.
+  size_t atomicId;
+  {
+    RecordingEventSection res(thread);
+    if (!res.CanAccessEvents()) {
+      return;
+    }
+
+    thread->Events().RecordOrReplayThreadEvent(ThreadEvent::AtomicAccess);
+
+    atomicId = IsRecording() ? (HashGeneric(aValue) % NumAtomicLocks) : 0;
+    thread->Events().RecordOrReplayScalar(&atomicId);
+    MOZ_RELEASE_ASSERT(atomicId < NumAtomicLocks);
   }
+
+  // When recording, hold a spin lock so that no other thread can access this
+  // same atomic until this access ends. When replaying, we don't need to hold
+  // any actual lock, as the atomic access cannot race and the Lock structure
+  // ensures that accesses happen in the same order.
+  if (IsRecording()) {
+    gAtomicLockOwners[atomicId].Lock();
+  }
+
+  gAtomicLocks[atomicId]->Enter();
+
+  MOZ_RELEASE_ASSERT(thread->AtomicLockId().isNothing());
+  thread->AtomicLockId().emplace(atomicId);
 }
 
 MOZ_EXPORT void
 RecordReplayInterface_InternalEndOrderedAtomicAccess()
 {
   MOZ_RELEASE_ASSERT(IsRecordingOrReplaying());
-  if (!gInitializationFailureMessage) {
-    gAtomicLock->unlock();
+
+  Thread* thread = Thread::Current();
+  if (!thread || thread->PassThroughEvents() || thread->HasDivergedFromRecording()) {
+    return;
   }
+
+  MOZ_RELEASE_ASSERT(thread->AtomicLockId().isSome());
+  size_t atomicId = thread->AtomicLockId().ref();
+  thread->AtomicLockId().reset();
+
+  if (IsRecording()) {
+    gAtomicLockOwners[atomicId].Unlock();
+  }
+
+  gAtomicLocks[atomicId]->Exit();
 }
 
 } // extern "C"
 
 } // namespace recordreplay
 } // namespace mozilla
--- a/toolkit/recordreplay/ProcessRecordReplay.cpp
+++ b/toolkit/recordreplay/ProcessRecordReplay.cpp
@@ -144,23 +144,23 @@ RecordReplayInterface_Initialize(int aAr
 
   InitializeTriggers();
   InitializeWeakPointers();
   InitializeMemorySnapshots();
   Thread::SpawnAllThreads();
   InitializeCountdownThread();
   SetupDirtyMemoryHandler();
   InitializeMiddlemanCalls();
+  Lock::InitializeLocks();
 
   // Don't create a stylo thread pool when recording or replaying.
   putenv((char*) "STYLO_THREADS=1");
 
   thread->SetPassThrough(false);
 
-  Lock::InitializeLocks();
   InitializeRewindState();
   gRecordingPid = RecordReplayValue(gPid);
 
   gInitialized = true;
 }
 
 MOZ_EXPORT size_t
 RecordReplayInterface_InternalRecordReplayValue(size_t aValue)
--- a/toolkit/recordreplay/ProcessRecordReplay.h
+++ b/toolkit/recordreplay/ProcessRecordReplay.h
@@ -40,16 +40,19 @@ namespace recordreplay {
                                                                \
   /* Called RecordReplayBytes. */                              \
   _Macro(Bytes)                                                \
                                                                \
   /* Called RecordReplayAssert or RecordReplayAssertBytes. */  \
   _Macro(Assert)                                               \
   _Macro(AssertBytes)                                          \
                                                                \
+  /* Performed an atomic access. */                            \
+  _Macro(AtomicAccess)                                         \
+                                                               \
   /* Executed a nested callback (see Callback.h). */           \
   _Macro(ExecuteCallback)                                      \
                                                                \
   /* Finished executing nested callbacks in a library API (see Callback.h). */ \
   _Macro(CallbacksFinished)                                    \
                                                                \
   /* Restoring a data pointer used in a callback (see Callback.h). */ \
   _Macro(RestoreCallbackData)                                  \
--- a/toolkit/recordreplay/Thread.h
+++ b/toolkit/recordreplay/Thread.h
@@ -135,16 +135,19 @@ private:
 
   // Any callback which should be invoked so the thread can make progress,
   // and whether the callback has been invoked yet while the main thread is
   // waiting for threads to become idle. Protected by the thread monitor.
   std::function<void()> mUnrecordedWaitCallback;
   bool mUnrecordedWaitOnlyWhenDiverged;
   bool mUnrecordedWaitNotified;
 
+  // Identifier of any atomic which this thread currently holds.
+  Maybe<size_t> mAtomicLockId;
+
 public:
 ///////////////////////////////////////////////////////////////////////////////
 // Public Routines
 ///////////////////////////////////////////////////////////////////////////////
 
   // Accessors for some members that never change.
   size_t Id() { return mId; }
   NativeThreadId NativeId() { return mNativeId; }
@@ -249,16 +252,19 @@ public:
   // Start an existing thread, for use when the process has called a thread
   // creation system API when events were not passed through. The return value
   // is the native ID of the result.
   static NativeThreadId StartThread(Callback aStart, void* aArgument, bool aNeedsJoin);
 
   // Wait until this thread finishes executing its start routine.
   void Join();
 
+  // Give access to the atomic lock which the thread owns.
+  Maybe<size_t>& AtomicLockId() { return mAtomicLockId; }
+
 ///////////////////////////////////////////////////////////////////////////////
 // Thread Coordination
 ///////////////////////////////////////////////////////////////////////////////
 
   // Basic API for threads to coordinate activity with each other, for use
   // during replay. Each Notify() on a thread ID will cause that thread to
   // return from one call to Wait(). Thus, if a thread Wait()'s and then
   // another thread Notify()'s its ID, the first thread will wake up afterward.