Add debug mode to shared mutex.

src/core/SkSharedMutex.h

 /*
  * Copyright 2015 Google Inc.
  *
  * Use of this source code is governed by a BSD-style license that can be
  * found in the LICENSE file.
  */
 
 #ifndef SkSharedLock_DEFINED
 #define SkSharedLock_DEFINED
 
 #include "SkAtomics.h"
 #include "SkSemaphore.h"
 #include "SkTypes.h"
-    
-// This is a shared lock implementation similar to pthreads rwlocks. This implementation is
-// cribbed from Preshing's article:
+
+// There are two shared lock implementations one debug the other is high performance. They implement
+// an interface similar to pthread's rwlocks.
+// This is a shared lock implementation similar to pthreads rwlocks. The high performance
+// implementation is cribbed from Preshing's article:
 // http://preshing.com/20150316/semaphores-are-surprisingly-versatile/
 //
 // This lock does not obey strict queue ordering. It will always alternate between readers and
 // a single writer.
+
+#ifdef SK_DEBUG

[mtklein, 2015/09/09 18:48:12]

I generally think google3's style on not indenting preprocessor directives or
code inside them is wrong.  This reads more easily with indentation:

#ifdef SK_DEBUG
  #include ...
  #include ...
  #ifdef SK_BUILD_FOR_WIN
    #include <windows.h>
    class SkThreadID {
      ...
    };
  #else
    #include <pthread.h>
    class SkThreadID {
      ...
    };
  #endif

   class SkThreadIDSet {
      ...
   };
   class SkSharedMutex {
      ...
   };
#else
   class SkSharedMutex {

   };
#endif

+#include "SkMutex.h"
+#include "SkTDArray.h"
+#ifdef SK_BUILD_FOR_WIN
+#include <windows.h>
+class SkThreadID {
+public:
+    SkThreadID() : fThreadID(GetCurrentThreadId()) { }
+    bool operator==(const SkThreadID& that) const {
+        return fThreadID == that.fThreadID;
+    }
+
+    int64_t toInt() const {
+        return (int64_t)fThreadID;
+    }
+
+private:
+    DWORD fThreadID;
+};
+#else
+#include <pthread.h>
+class SkThreadID {

[mtklein, 2015/09/09 18:48:12]

Why don't we nest these inside SkSharedMutex as ThreadID and ThreadIDSet? 
They're not going to be used outside it, right?

+public:
+    SkThreadID() : fThreadID(pthread_self()) { }
+    bool operator==(const SkThreadID& that) const {
+        return fThreadID == that.fThreadID;
+    }
+    int64_t toInt() const {
+        return (int64_t)fThreadID;
+    }
+private:
+    pthread_t fThreadID;
+};
+#endif
+
+class SkThreadIDSet {
+public:
+    // Returns true if threadID is in the set.
+    bool Find(SkThreadID threadID) const {

[mtklein, 2015/09/09 18:48:12]

find, tryAdd, tryRemove, swap, count()  :P

+        for (auto& t : fThreadIDs) {
+            if (t == threadID) return true;
+        }
+        return false;
+    }
+
+    // Returns true if did not already exist.
+    bool TryAdd(SkThreadID threadID) {
+        for (auto& t : fThreadIDs) {
+            if (t == threadID) return false;
+        }
+        fThreadIDs.append(1, &threadID);
+        return true;
+    }
+    // Returns true if already exists in Set.
+    bool TryRemove(SkThreadID threadID) {
+        for (int i = 0; i < fThreadIDs.count(); ++i) {
+            if (fThreadIDs[i] == threadID) {
+                fThreadIDs.remove(i);
+                return true;
+            }
+        }
+        return false;
+    }
+
+    void Swap(SkThreadIDSet& other) {
+        fThreadIDs.swap(other.fThreadIDs);
+    }
+
+    int Count() const {
+        return fThreadIDs.count();
+    }
+
+private:
+    SkTDArray<SkThreadID> fThreadIDs;
+};
+
 class SkSharedMutex {
 public:
     SkSharedMutex();
     ~SkSharedMutex();
     // Acquire lock for exclusive use.
     void acquire();
 
     // Release lock for exclusive use.
     void release();
 
     // Fail if exclusive is not held.
-#ifdef SK_DEBUG
     void assertHeld() const;
-#else
-    void assertHeld() const {}
-#endif
 
     // Acquire lock for shared use.
     void acquireShared();
 
     // Release lock for shared use.
     void releaseShared();
 
     // Fail if shared lock not held.
-#ifdef SK_DEBUG
     void assertHeldShared() const;
+
+private:
+    SkThreadIDSet    fCurrentShared;
+    SkThreadIDSet    fWaitingExclusive;
+    SkThreadIDSet    fWaitingShared;
+    int              fSharedQueueSelect{0};
+    mutable SkMutex  fMu;
+    SkSemaphore      fSharedQueue[2];
+    SkSemaphore      fExclusiveQueue;
+};
 #else
+class SkSharedMutex {
+public:
+    SkSharedMutex();
+    ~SkSharedMutex();
+    // Acquire lock for exclusive use.
+    void acquire();
+
+    // Release lock for exclusive use.
+    void release();
+
+    // Fail if exclusive is not held.
+    void assertHeld() const {}
+
+    // Acquire lock for shared use.
+    void acquireShared();
+
+    // Release lock for shared use.
+    void releaseShared();
+
+    // Fail if shared lock not held.
     void assertHeldShared() const {}
-#endif
 
 private:
     SkAtomic<int32_t> fQueueCounts;

[mtklein, 2015/09/09 18:48:11]

I think I'd feel more cozy if there really were just one SkSharedMutex
implementation, with some SkDEBUGCODE(...) / #ifdef SK_DEBUG ... #endif extras. 
 It feels better when the code we run in Debug mode is a superset of the Release
mode code, not something totally different.

When arranged this way there's no way, e.g., that we can SkASSERT() anything
about fQueueCounts sensibly, as fQueueCounts only exists in Release mode.

     SkSemaphore fSharedQueue;
     SkSemaphore fExclusiveQueue;
 };
 
+#endif // SK_DEBUG
+
 
 #endif // SkSharedLock_DEFINED