Add debug mode to shared mutex.

Add debug mode to shared mutex.

Committed: https://skia.googlesource.com/skia/+/966e3d30baf676dc399dc9c97ef15cc2c4f8d461

[herb_g, 2015-09-09 18:10:48]

herb@google.com changed reviewers:
+ mtklein@google.com

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

https://codereview.chromium.org/1307863009/diff/20001/src/core/SkSharedMutex.h
File src/core/SkSharedMutex.h (right):

https://codereview.chromium.org/1307863009/diff/20001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:24: #ifdef SK_DEBUG
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

https://codereview.chromium.org/1307863009/diff/20001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:45: class SkThreadID {
Why don't we nest these inside SkSharedMutex as ThreadID and ThreadIDSet? 
They're not going to be used outside it, right?

https://codereview.chromium.org/1307863009/diff/20001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:62: bool Find(SkThreadID threadID) const {
find, tryAdd, tryRemove, swap, count()  :P

https://codereview.chromium.org/1307863009/diff/20001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:155: SkAtomic<int32_t> fQueueCounts;
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.

[herb_g, 2015-09-17 19:34:34]

PTAL

[mtklein_C, 2015-09-17 19:51:41]

mtklein@chromium.org changed reviewers:
+ mtklein@chromium.org

[mtklein_C, 2015-09-17 19:51:42]

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex.cpp
File src/core/SkSharedMutex.cpp (right):

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.cpp:75: int currentShardCount;
shard -> shared?

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.cpp:82: SkASSERT(false);
SkFAIL or SkDEBUGFAIL

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.cpp:109: SkDebugf("Thread %lx did not have the lock
held.\n", threadID.toInt());
ditto, etc.

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex.h
File src/core/SkSharedMutex.h (right):

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:21: static threadID_t getThreadID() { return
GetCurrentThreadId(); }
These standalone statics are typically get_thread_id()

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:25: static threadID_t getThreadID() { return
pthread_self(); }
static int64_t get_thread_id() { return (int64_t)pthread_self(); }
static int64_t get_thread_id() { return (int64_t)GetCurrentThreadId(); }d

?

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:48: void assertHeld() const;
undo these bits?

[herb_g, 2015-09-17 21:39:41]

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex.cpp
File src/core/SkSharedMutex.cpp (right):

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.cpp:75: int currentShardCount;
On 2015/09/17 19:51:42, mtklein_C wrote:
> shard -> shared?

Done.

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.cpp:82: SkASSERT(false);
On 2015/09/17 19:51:42, mtklein_C wrote:
> SkFAIL or SkDEBUGFAIL

Done.

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.cpp:109: SkDebugf("Thread %lx did not have the lock
held.\n", threadID.toInt());
On 2015/09/17 19:51:42, mtklein_C wrote:
> ditto, etc.

Done.

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex.h
File src/core/SkSharedMutex.h (right):

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:21: static threadID_t getThreadID() { return
GetCurrentThreadId(); }
On 2015/09/17 19:51:42, mtklein_C wrote:
> These standalone statics are typically get_thread_id()

Done.

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:25: static threadID_t getThreadID() { return
pthread_self(); }
On 2015/09/17 19:51:42, mtklein_C wrote:
> static int64_t get_thread_id() { return (int64_t)pthread_self(); }
> static int64_t get_thread_id() { return (int64_t)GetCurrentThreadId(); }d
> 
> ?

Done.

https://codereview.chromium.org/1307863009/diff/40001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:48: void assertHeld() const;
On 2015/09/17 19:51:42, mtklein_C wrote:
> undo these bits?

Done.

[mtklein, 2015-09-17 22:07:24]

lgtm

https://codereview.chromium.org/1307863009/diff/60001/src/core/SkSharedMutex.h
File src/core/SkSharedMutex.h (right):

https://codereview.chromium.org/1307863009/diff/60001/src/core/SkSharedMutex....
src/core/SkSharedMutex.h:53: skstd::unique_ptr<ThreadIDSet>fCurrentShared;
sssppace

[herb_g, 2015-09-17 22:12:37]

The CQ bit was checked by herb@google.com

[herb_g, 2015-09-17 22:12:37]

The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1307863009/#ps80001
(title: "remove space")

[commit-bot: I haz the power, 2015-09-17 22:13:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307863009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307863009/80001

[commit-bot: I haz the power, 2015-09-17 22:14:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-17 22:14:07]

Try jobs failed on following builders:
  skia_presubmit-Trybot on client.skia.fyi (JOB_FAILED,
http://build.chromium.org/p/client.skia.fyi/builders/skia_presubmit-Trybot/bu...)

[herb_g, 2015-09-18 13:50:33]

herb@google.com changed reviewers:
+ djsollen@google.com

[herb_g, 2015-09-18 13:50:33]

I added SkDEBUGFAILF so you can have formatted fail statements. Since this is a
change to an API include (SkTypes.h) it needs your approval.

[djsollen, 2015-09-18 13:56:00]

api lgtm

[herb_g, 2015-09-18 13:59:54]

The CQ bit was checked by herb@google.com

[commit-bot: I haz the power, 2015-09-18 14:00:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1307863009/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1307863009/80001

[commit-bot: I haz the power, 2015-09-18 14:00:50]

Committed patchset #5 (id:80001) as
https://skia.googlesource.com/skia/+/966e3d30baf676dc399dc9c97ef15cc2c4f8d461