New lock implementation for mac

webrtc/base/criticalsection.cc

Index: webrtc/base/criticalsection.cc
diff --git a/webrtc/base/criticalsection.cc b/webrtc/base/criticalsection.cc
index 1f50c2355dbcef57b016f31dcc8c68501b9a84ca..65c79dc3b1a256be88648060a401330d6f0fe2e8 100644
--- a/webrtc/base/criticalsection.cc
+++ b/webrtc/base/criticalsection.cc
@@ -18,11 +18,19 @@ CriticalSection::CriticalSection() {
 #if defined(WEBRTC_WIN)
   InitializeCriticalSection(&crit_);
 #else
+#if defined(WEBRTC_MAC)
+  lock_queue_ = 0;
+  owning_thread_ = 0;
+  recursion_ = 0;
+  semaphore_ = dispatch_semaphore_create(0);
+  RTC_DCHECK(semaphore_);

[torbjorng (webrtc), 2016/01/20 15:46:43]

DCHECK? Shouldn't we barf in all builds if this call fails?

[tommi, 2016/01/21 15:04:21]

I'll just remove this check.  It's almost akin to checking new(), which we don't
do.  We also don't check calls that instantiate OS based primitives such as
locks in general.

+#else
   pthread_mutexattr_t mutex_attribute;
   pthread_mutexattr_init(&mutex_attribute);
   pthread_mutexattr_settype(&mutex_attribute, PTHREAD_MUTEX_RECURSIVE);
   pthread_mutex_init(&mutex_, &mutex_attribute);
   pthread_mutexattr_destroy(&mutex_attribute);
+#endif
   CS_DEBUG_CODE(thread_ = 0);
   CS_DEBUG_CODE(recursion_count_ = 0);
 #endif
@@ -32,15 +40,44 @@ CriticalSection::~CriticalSection() {
 #if defined(WEBRTC_WIN)
   DeleteCriticalSection(&crit_);
 #else
+#if defined(WEBRTC_MAC)
+  dispatch_release(semaphore_);
+#else
   pthread_mutex_destroy(&mutex_);
 #endif
+#endif
 }
 
 void CriticalSection::Enter() EXCLUSIVE_LOCK_FUNCTION() {
 #if defined(WEBRTC_WIN)
   EnterCriticalSection(&crit_);
 #else
+#if defined(WEBRTC_MAC)
+  int spin = 2000;
+  do {
+    if (TryEnter())

[torbjorng (webrtc), 2016/01/20 15:46:43]

TryEnter seems to perform a CompareAndSwap unconditionally (except for same
thread). That will move the lock cache-line to this thread's current physical
core, putting the cache-line in "dirty" state. This is bad when we fail to get
the lock, since subsequently the lock owner will need to cache the lock
cache-line before unlocking.

I leaner poll just reads the lock, and then conditionally immediately attempts
to lock it.  The racing condition between the poll and attempted locking should
not matter (at least not if all contenders use the same non-aggressive
approach).

[tommi, 2016/01/21 15:04:21]

I rewrote the loop to not call TryEnter() but instead do this special case since
I don't think it's relevant to the TryEnter case (i.e. there's no spinning in
that case).

In my timing measurements I didn't see a significant enough difference to be
able to say with confidence that there's a clear win, but it's not slower at
least and I understand why it makes sense to do this.  Included cache line
barriers in my test code btw.

+      return;
+    sched_yield();
+  } while (--spin);
+
+  if (AtomicOps::Increment(&lock_queue_) > 0) {
+    // Owning thread cannot be the current thread since TryEnter() would
+    // have succeeded.
+    RTC_DCHECK(owning_thread_ != pthread_self());
+    // Wait for the lock to become available.
+    dispatch_semaphore_wait(semaphore_, DISPATCH_TIME_FOREVER);
+    RTC_DCHECK(owning_thread_ == 0);
+    RTC_DCHECK(!recursion_);
+    owning_thread_ = pthread_self();
+  }
+
+  RTC_DCHECK(owning_thread_ == pthread_self());
+  ++recursion_;
+
+#else
   pthread_mutex_lock(&mutex_);
+#endif
+
 #if CS_DEBUG_CHECKS
   if (!recursion_count_) {
     RTC_DCHECK(!thread_);
@@ -57,8 +94,20 @@ bool CriticalSection::TryEnter() EXCLUSIVE_TRYLOCK_FUNCTION(true) {
 #if defined(WEBRTC_WIN)
   return TryEnterCriticalSection(&crit_) != FALSE;
 #else
+#if defined(WEBRTC_MAC)
+  if (owning_thread_ != pthread_self()) {
+    if (AtomicOps::CompareAndSwap(&lock_queue_, 0, 1) != 0)
+      return false;
+    owning_thread_ = pthread_self();
+    RTC_DCHECK(!recursion_);
+  } else {
+    AtomicOps::Increment(&lock_queue_);
+  }
+  ++recursion_;
+#else
   if (pthread_mutex_trylock(&mutex_) != 0)
     return false;
+#endif
 #if CS_DEBUG_CHECKS
   if (!recursion_count_) {
     RTC_DCHECK(!thread_);
@@ -82,8 +131,19 @@ void CriticalSection::Leave() UNLOCK_FUNCTION() {
   if (!recursion_count_)
     thread_ = 0;
 #endif
+#if defined(WEBRTC_MAC)
+  RTC_DCHECK_EQ(owning_thread_, pthread_self());
+  RTC_DCHECK_GE(recursion_, 0);
+  --recursion_;
+  if (!recursion_)
+    owning_thread_ = 0;
+
+  if (AtomicOps::Decrement(&lock_queue_) > 0 && !recursion_)
+    dispatch_semaphore_signal(semaphore_);
+#else
   pthread_mutex_unlock(&mutex_);
 #endif
+#endif
 }
 
 bool CriticalSection::CurrentThreadIsOwner() const {
@@ -135,13 +195,15 @@ bool TryCritScope::locked() const {
 }
 
 void GlobalLockPod::Lock() {
-#if !defined(WEBRTC_WIN)
+#if !defined(WEBRTC_WIN) && !defined(WEBRTC_MAC)
   const struct timespec ts_null = {0};
 #endif
 
   while (AtomicOps::CompareAndSwap(&lock_acquired, 0, 1)) {
 #if defined(WEBRTC_WIN)
     ::Sleep(0);
+#elif defined(WEBRTC_MAC)
+    sched_yield();
 #else
     nanosleep(&ts_null, nullptr);
 #endif