Outline SkSpinlock::acquire().

Outline SkSpinlock::acquire().

The proportion of time spent doing useful work is well over 99% in acquire(),
so outlining it doesn't hurt speed at all, and makes it much easier to pick out
on a profile.

It'd be about 50/50 work/overhead if we outlined the extremely-cheap release().
I also tried outlining some SkRefCnt methods with similar mixed results.

BUG=skia:

No public API changes.
TBR=reed@google.com

Committed: https://skia.googlesource.com/skia/+/828877d7267bcb7c272a1df4e36a6a79894bf00d

[mtklein_C, 2015-07-08 13:37:16]

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

[mtklein_C, 2015-07-08 13:37:36]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-07-08 13:37:52]

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

[commit-bot: I haz the power, 2015-07-08 13:45:23]

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

[commit-bot: I haz the power, 2015-07-08 13:45:23]

Dry run: This issue passed the CQ dry run.

[herb_g, 2015-07-08 13:53:21]

Have you thought about using function to get insight into wait time. You could
have the first acquire function do 1000 iterations, and after that have it call
a separate function that does 9000 iterations then have that function call a
function that does 90,000 iterations. This way you can see in instruments if a
particular path has a contention problem.

[mtklein, 2015-07-08 14:04:45]

On 2015/07/08 13:53:21, herb1 wrote:
> Have you thought about using function to get insight into wait time. You could
> have the first acquire function do 1000 iterations, and after that have it
call
> a separate function that does 9000 iterations then have that function call a
> function that does 90,000 iterations. This way you can see in instruments if a
> particular path has a contention problem.

Ooh, I like that.

One variant I've been thinking of too is to spin something like this:

acquire() {
  while (fLocked.load(sk_memory_order_relaxed)) {}
  while (fLocked.exchange(true, sk_memory_order_acquire)) {}
}

basically, while obviously contended, don't bother with xchg, instead just loop
on loading.

[herb_g, 2015-07-08 16:08:52]

So, this would be a kind of double check locking. I don't think this is what you
want. I think you should optimize for the un-contended case. So almost all the
time you just execute a single exchange and fall through the loop instead of
almost all of the time you execute a load fall through the loop and then execute
the exchange and fall through the loop. I could see...
if (!fLocked.exchange(true, sk_memory_order_acquire)) {
   // call spin out of line
   StartSpinning().
}

[mtklein, 2015-07-08 16:41:30]

On 2015/07/08 16:08:52, herb1 wrote:
> So, this would be a kind of double check locking. I don't think this is what
you
> want. I think you should optimize for the un-contended case. So almost all the
> time you just execute a single exchange and fall through the loop instead of
> almost all of the time you execute a load fall through the loop and then
execute
> the exchange and fall through the loop. I could see...
> if (!fLocked.exchange(true, sk_memory_order_acquire)) {
>    // call spin out of line
>    StartSpinning().
> }

Good point.  I agree that optimizing for contention is counterproductive.

What do you think of this?  I actually tried a hybrid approach you can see in
patch set 4, but Clang actually optimized away the read-only part of the loop,
effectively rewriting into patch set 5 for me!

[mtklein, 2015-07-08 16:44:03]

On 2015/07/08 16:41:30, mtklein wrote:
> On 2015/07/08 16:08:52, herb1 wrote:
> > So, this would be a kind of double check locking. I don't think this is what
> you
> > want. I think you should optimize for the un-contended case. So almost all
the
> > time you just execute a single exchange and fall through the loop instead of
> > almost all of the time you execute a load fall through the loop and then
> execute
> > the exchange and fall through the loop. I could see...
> > if (!fLocked.exchange(true, sk_memory_order_acquire)) {
> >    // call spin out of line
> >    StartSpinning().
> > }
> 
> Good point.  I agree that optimizing for contention is counterproductive.
> 
> What do you think of this?  I actually tried a hybrid approach you can see in
> patch set 4, but Clang actually optimized away the read-only part of the loop,
> effectively rewriting into patch set 5 for me!

Wow, this was a great idea.  Thanks.  You can see now that under a typical MPD
load (something like 10 tiles, nytimes.skp), the existing thread pool spinlock
is not contended... we're almost always getting the lock on the first try.

[mtklein, 2015-07-08 16:44:26]

The CQ bit was checked by mtklein@google.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-08 16:44:39]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1212253013/80001

[commit-bot: I haz the power, 2015-07-08 16:58:46]

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

[commit-bot: I haz the power, 2015-07-08 16:58:48]

Dry run: This issue passed the CQ dry run.

[mtklein_C, 2015-07-09 17:25:52]

ping?  I'm finding this really insightful when profiling your glyph cache change
with different numbers of threads running.

[mtklein_C, 2015-07-09 17:26:16]

The CQ bit was checked by mtklein@chromium.org to run a CQ dry run

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

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1212253013/100001

[herb_g, 2015-07-09 17:33:35]

lgtm

Nice. I will have to hurry and integrate this into my current CL.

[mtklein, 2015-07-09 17:37:09]

The CQ bit was unchecked by mtklein@google.com

[mtklein, 2015-07-09 17:37:10]

The CQ bit was checked by mtklein@google.com

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

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

[commit-bot: I haz the power, 2015-07-09 17:38:55]

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

[commit-bot: I haz the power, 2015-07-09 17:38:56]

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...)

[mtklein, 2015-07-09 17:50:09]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2015-07-09 17:50:35]

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

[commit-bot: I haz the power, 2015-07-09 17:51:40]

Committed patchset #6 (id:100001) as
https://skia.googlesource.com/skia/+/828877d7267bcb7c272a1df4e36a6a79894bf00d