Implement shared locks in the style of pthread's rwlock.

Implement shared locks in the style of pthread's rwlock.

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/c782b2a046763dc427fa47d4e33afca59c994d60

[herb_g, 2015-06-29 14:54:39]

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

[herb_g, 2015-06-29 14:54:40]

Please point out my Skia style problems. I'm trying to learn the style guide.

[herb_g, 2015-06-29 14:54:43]

The CQ bit was checked by herb@google.com

[commit-bot: I haz the power, 2015-06-29 14:54:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215503005/1

[commit-bot: I haz the power, 2015-06-29 14:54:50]

Note for Reviewers:
The CQ is waiting for an approval.
If you believe that the CL is not ready yet, or if you would like to L-G-T-M
with comments then please uncheck the CQ checkbox.

Waiting for LGTM from valid reviewer(s) till 2015-06-29 20:54 UTC

[commit-bot: I haz the power, 2015-06-29 14:56:19]

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

[commit-bot: I haz the power, 2015-06-29 14:56:20]

Try jobs failed on following builders:
  Build-Ubuntu-GCC-x86_64-Release-Trybot on client.skia.compile (JOB_FAILED,
http://build.chromium.org/p/client.skia.compile/builders/Build-Ubuntu-GCC-x86...)

[mtklein, 2015-06-29 16:29:23]

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h
File src/core/SkSharedLock.h (right):

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:21: class SkSharedLock {
Let's implement this class in SkSharedLock.cpp and fall back on inlining if we
find ourselves caring?

As an experiment I implemented SkSemaphore out of line in a .cpp; so far I
haven't seen it on a profile.  As far as I can tell, even on Intel but
especially on ARM, cost(atomic instruction) >> cost(method call).

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:30: sk_memory_order_release);
preshing sez: sk_memory_order_acquire

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:50: // The number of shared lock holders waiting.
Number of threads waiting to hold a shared lock?  Seems weird to call them
holders before they're holding the lock, especially here where we definitely
know they're not.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:60: // Because we are waiting for writers then readers
bits are zero. An or will slam
Help me through this?  Is this what you're saying?

// We have an exclusive write lock, so there can't be any active readers.
// This means we can add the waiting readers to active readers by just
// or-ing the count into place.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:91: sk_memory_order_release, sk_memory_order_relaxed));
preshing sez: _acquire, _relaxed

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:114: static const int kLogThreadCount = 10;
Let's explain this?  Maybe

// We assume <1024 threads are interacting with each lock.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:119: kSharedOffset = (0 * kLogThreadCount),
I personally find it a bit more readable to line up the right-hand-sides of
parallel expressions like these, especially if it just takes adding some more
spaces before an '='.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:126: SkAtomic<int32_t> fQueueCounts;
// Using an int32_t makes it easier to call .fetch_add(-k).
// Doesn't matter much: we don't use the top two bits anyway.

(A fine alternative is to use uint32_t and ~0 if that reads easier to you.)

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp
File tests/SkSharedLockTest.cpp (right):

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:14: SkSharedLock sm;
Given that we've already got the precent of 'SkMutex' and we're even
abbreviating this as 'sm', we might consider a doing a
s/SkSharedLock/SkSharedMutex/g.

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:30: if (threadIndex % 4 != 0) {
Time to switch your editor to 4 space indents. :)

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:35: SkASSERTF(v == fShared[i], "v: %d i: %d, s[i]:
%d", v, i, fShared[i]);
Generally we'd use REPORTER_ASSERT(r, v == fShared[i]).  Otherwise, failures
here kill the whole test harness instead of just failing this test (and
SkASSERTF compiles away to nothing in Release mode).

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:66: sk_parallel_for(8, t);
Any reason to not write this all inline?

DEF_TEST(SkSharedLockMultiThreaded, r) {
  SkSharedLock sm;
  static const int kSharedSize = 10;
  int shared[kSharedSize];
  for (int i = 0; i < kSharedSize; ++i) {
    shared[i] = 0;
  }

  int value = 0;
  sk_parallel_for(8, [&](int threadIndex) {
    if (threadIndex % 4 != 0) {
      for (int c = 0; c < 100000; ++c) {
        sm.acquireShared();
        int v = shared[0];
        for (int i = 1; i < kSharedSize; ++i) {
          SkASSERTF(v == shared[i], "v: %d i: %d, s[i]: %d", v, i, shared[i]);
        }
        sm.releaseShared();
      }
    } else {
      for (int c = 0; c < 100000; ++c) {
        sm.acquire();
        value += 1;
        for (int i = 0; i < kSharedSize; ++i) {
          shared[i] = value;
        }
        sm.release();
      }
    }
 });
}

[herb_g, 2015-06-29 19:26:31]

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h
File src/core/SkSharedLock.h (right):

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:21: class SkSharedLock {
On 2015/06/29 16:29:22, mtklein wrote:
> Let's implement this class in SkSharedLock.cpp and fall back on inlining if we
> find ourselves caring?
> 
> As an experiment I implemented SkSemaphore out of line in a .cpp; so far I
> haven't seen it on a profile.  As far as I can tell, even on Intel but
> especially on ARM, cost(atomic instruction) >> cost(method call).

Done.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:30: sk_memory_order_release);
On 2015/06/29 16:29:22, mtklein wrote:
> preshing sez: sk_memory_order_acquire

Right. release makes no sense because the code is single threaded until that
point.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:50: // The number of shared lock holders waiting.
On 2015/06/29 16:29:22, mtklein wrote:
> Number of threads waiting to hold a shared lock?  Seems weird to call them
> holders before they're holding the lock, especially here where we definitely
> know they're not.

Done.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:60: // Because we are waiting for writers then readers
bits are zero. An or will slam
On 2015/06/29 16:29:22, mtklein wrote:
> Help me through this?  Is this what you're saying?
> 
> // We have an exclusive write lock, so there can't be any active readers.
> // This means we can add the waiting readers to active readers by just
> // or-ing the count into place.

Done.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:91: sk_memory_order_release, sk_memory_order_relaxed));
On 2015/06/29 16:29:22, mtklein wrote:
> preshing sez: _acquire, _relaxed

Done.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:114: static const int kLogThreadCount = 10;
On 2015/06/29 16:29:22, mtklein wrote:
> Let's explain this?  Maybe
> 
> // We assume <1024 threads are interacting with each lock.

Done.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:119: kSharedOffset = (0 * kLogThreadCount),
On 2015/06/29 16:29:22, mtklein wrote:
> I personally find it a bit more readable to line up the right-hand-sides of
> parallel expressions like these, especially if it just takes adding some more
> spaces before an '='.

Done.

https://codereview.chromium.org/1215503005/diff/1/src/core/SkSharedLock.h#new...
src/core/SkSharedLock.h:126: SkAtomic<int32_t> fQueueCounts;
On 2015/06/29 16:29:22, mtklein wrote:
> // Using an int32_t makes it easier to call .fetch_add(-k).
> // Doesn't matter much: we don't use the top two bits anyway.
> 
> (A fine alternative is to use uint32_t and ~0 if that reads easier to you.)

Acknowledged.

[herb_g, 2015-06-29 20:37:06]

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp
File tests/SkSharedLockTest.cpp (right):

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:30: if (threadIndex % 4 != 0) {
On 2015/06/29 16:29:22, mtklein wrote:
> Time to switch your editor to 4 space indents. :)

Done.

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:35: SkASSERTF(v == fShared[i], "v: %d i: %d, s[i]:
%d", v, i, fShared[i]);
On 2015/06/29 16:29:22, mtklein wrote:
> Generally we'd use REPORTER_ASSERT(r, v == fShared[i]).  Otherwise, failures
> here kill the whole test harness instead of just failing this test (and
> SkASSERTF compiles away to nothing in Release mode).

Done.

https://codereview.chromium.org/1215503005/diff/1/tests/SkSharedLockTest.cpp#...
tests/SkSharedLockTest.cpp:66: sk_parallel_for(8, t);
On 2015/06/29 16:29:22, mtklein wrote:
> Any reason to not write this all inline?
> 
> DEF_TEST(SkSharedLockMultiThreaded, r) {
>   SkSharedLock sm;
>   static const int kSharedSize = 10;
>   int shared[kSharedSize];
>   for (int i = 0; i < kSharedSize; ++i) {
>     shared[i] = 0;
>   }
> 
>   int value = 0;
>   sk_parallel_for(8, [&](int threadIndex) {
>     if (threadIndex % 4 != 0) {
>       for (int c = 0; c < 100000; ++c) {
>         sm.acquireShared();
>         int v = shared[0];
>         for (int i = 1; i < kSharedSize; ++i) {
>           SkASSERTF(v == shared[i], "v: %d i: %d, s[i]: %d", v, i, shared[i]);
>         }
>         sm.releaseShared();
>       }
>     } else {
>       for (int c = 0; c < 100000; ++c) {
>         sm.acquire();
>         value += 1;
>         for (int i = 0; i < kSharedSize; ++i) {
>           shared[i] = value;
>         }
>         sm.release();
>       }
>     }
>  });
> }

Done.

[mtklein, 2015-06-29 20:39:40]

The CQ bit was checked by mtklein@google.com

[mtklein, 2015-06-29 20:39:41]

lgtm

[commit-bot: I haz the power, 2015-06-29 20:39:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1215503005/40001

[commit-bot: I haz the power, 2015-06-29 20:46:57]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/c782b2a046763dc427fa47d4e33afca59c994d60