Create debug only SkSingleOwner

Create debug only SkSingleOwner

This is so Gpu code can guard against improper multithreaded usage in
debug builds

TBR=bsalomon@google.com
BUG=skia:
GOLD_TRYBOT_URL= https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&issue=1555953004

Committed: https://skia.googlesource.com/skia/+/1de610a5287cf61d4f3a1fdc7413bd74827a8b6a

[joshua.litt, 2016-01-05 21:56:00]

Description was changed from

==========
Create debug only SkRecursiveMutex

This is so Gpu code can guard against improper multithreaded usage in
debug builds

BUG=skia:
==========

to

==========
Create debug only SkRecursiveMutex

This is so Gpu code can guard against improper multithreaded usage in
debug builds

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-01-05 21:59:26]

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

[joshualitt, 2016-01-05 21:59:26]

I realize this is not really ideal, but I think for debug only purposes it
should be okay(assuming correct implementation)

[mtklein, 2016-01-05 22:00:59]

why don't you make sure it works by including some code that uses it?

[joshualitt, 2016-01-05 22:10:33]

On 2016/01/05 22:00:59, mtklein wrote:
> why don't you make sure it works by including some code that uses it?

k, context guards added

[joshualitt, 2016-01-05 22:11:07]

joshualitt@google.com changed reviewers:
+ bsalomon@google.com, robertphillips@chromium.org

[robertphillips, 2016-01-05 22:33:49]

robertphillips@google.com changed reviewers:
+ robertphillips@google.com

[robertphillips, 2016-01-05 22:33:49]

Ganesh-side lgtm

[mtklein, 2016-01-05 22:47:50]

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:87: TAKE_DEBUG_MUTEX
What is the constraint that you are trying to enforce?

It looks like both Create() methods call initCommon().  Does every GrContext hit
this line before it exits Create()?

So is the constraint, a GrContext must only be used on the thread that creates
it?

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:208: void GrContext::flush(int flagsBitfield) {
Why do some methods TAKE_DEBUG_MUTEX and not others?

[mtklein, 2016-01-05 22:55:45]

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:208: void GrContext::flush(int flagsBitfield) {
On 2016/01/05 22:47:50, mtklein wrote:
> Why do some methods TAKE_DEBUG_MUTEX and not others?

Oh, I see.  That's very confusing.

[joshualitt, 2016-01-06 14:03:27]

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:87: TAKE_DEBUG_MUTEX
On 2016/01/05 22:47:50, mtklein wrote:
> What is the constraint that you are trying to enforce?
> 
> It looks like both Create() methods call initCommon().  Does every GrContext
hit
> this line before it exits Create()?
> 
> So is the constraint, a GrContext must only be used on the thread that creates
> it?

No, but a GrContext may only ever be used from one thread at a time.  That
single thread may arbitrarily re-enter the GrContext.

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:208: void GrContext::flush(int flagsBitfield) {
On 2016/01/05 22:55:45, mtklein wrote:
> On 2016/01/05 22:47:50, mtklein wrote:
> > Why do some methods TAKE_DEBUG_MUTEX and not others?
> 
> Oh, I see.  That's very confusing.

I can explicitly break it out of the macro if that makes it easier to follow

[mtklein, 2016-01-06 14:33:46]

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp
File src/gpu/GrContext.cpp (right):

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:87: TAKE_DEBUG_MUTEX
On 2016/01/06 14:03:27, joshualitt wrote:
> On 2016/01/05 22:47:50, mtklein wrote:
> > What is the constraint that you are trying to enforce?
> > 
> > It looks like both Create() methods call initCommon().  Does every GrContext
> hit
> > this line before it exits Create()?
> > 
> > So is the constraint, a GrContext must only be used on the thread that
creates
> > it?
> 
> No, but a GrContext may only ever be used from one thread at a time.  That
> single thread may arbitrarily re-enter the GrContext.

So what do we need the reentrant mutex for?  We're not also trying to queue
other threads up for access to the GrContext, are we?

Can't you just do something ordinary?

class SingleOwner {
public:
    SingleOwner() : fOwner(kIllegalThreadID), fReentranceCount(0) {}

    struct AutoEnforce {
        AutoEnforce(SingleOwner* so) : fSO(so) { fSO->enter(); }
        ~AutoEnforce() { fSO->exit(); }

        SingleOwner* fSO;
    };

private:
    void enter() {
        SkAutoMutexAcquire lock(fMutex);
        SkThreadID self = SkGetThreadID();
        SkASSERT(fOwner == self || fOwner == kIllegalThreadID);
        fReentranceCount++;
        fOwner = self;
    }

    void exit() {
        SkAutoMutexAcquire lock(fMutex);
        fReentranceCount--;
        if (fReentranceCount == 0) {
            fOwner = kIllegalThreadID;
        }
    }

    SkMutex fMutex;
    SkThreadID fOwner;    // guarded by fMutex
    int fReentranceCount; // guarded by fMutex
};

https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
src/gpu/GrContext.cpp:208: void GrContext::flush(int flagsBitfield) {
On 2016/01/06 14:03:27, joshualitt wrote:
> On 2016/01/05 22:55:45, mtklein wrote:
> > On 2016/01/05 22:47:50, mtklein wrote:
> > > Why do some methods TAKE_DEBUG_MUTEX and not others?
> > 
> > Oh, I see.  That's very confusing.
> 
> I can explicitly break it out of the macro if that makes it easier to follow

I think it would.

[joshualitt, 2016-01-06 15:05:21]

Description was changed from

==========
Create debug only SkRecursiveMutex

This is so Gpu code can guard against improper multithreaded usage in
debug builds

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Create debug only SkSingleOwner

This is so Gpu code can guard against improper multithreaded usage in
debug builds

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-01-06 15:06:20]

On 2016/01/06 14:33:46, mtklein wrote:
> https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp
> File src/gpu/GrContext.cpp (right):
> 
>
https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
> src/gpu/GrContext.cpp:87: TAKE_DEBUG_MUTEX
> On 2016/01/06 14:03:27, joshualitt wrote:
> > On 2016/01/05 22:47:50, mtklein wrote:
> > > What is the constraint that you are trying to enforce?
> > > 
> > > It looks like both Create() methods call initCommon().  Does every
GrContext
> > hit
> > > this line before it exits Create()?
> > > 
> > > So is the constraint, a GrContext must only be used on the thread that
> creates
> > > it?
> > 
> > No, but a GrContext may only ever be used from one thread at a time.  That
> > single thread may arbitrarily re-enter the GrContext.
> 
> So what do we need the reentrant mutex for?  We're not also trying to queue
> other threads up for access to the GrContext, are we?
> 
> Can't you just do something ordinary?
> 
> class SingleOwner {
> public:
>     SingleOwner() : fOwner(kIllegalThreadID), fReentranceCount(0) {}
> 
>     struct AutoEnforce {
>         AutoEnforce(SingleOwner* so) : fSO(so) { fSO->enter(); }
>         ~AutoEnforce() { fSO->exit(); }
> 
>         SingleOwner* fSO;
>     };
> 
> private:
>     void enter() {
>         SkAutoMutexAcquire lock(fMutex);
>         SkThreadID self = SkGetThreadID();
>         SkASSERT(fOwner == self || fOwner == kIllegalThreadID);
>         fReentranceCount++;
>         fOwner = self;
>     }
> 
>     void exit() {
>         SkAutoMutexAcquire lock(fMutex);
>         fReentranceCount--;
>         if (fReentranceCount == 0) {
>             fOwner = kIllegalThreadID;
>         }
>     }
> 
>     SkMutex fMutex;
>     SkThreadID fOwner;    // guarded by fMutex
>     int fReentranceCount; // guarded by fMutex
> };
done, I can move it to its own file if you prefer
> 
>
https://codereview.chromium.org/1555953004/diff/40001/src/gpu/GrContext.cpp#n...
> src/gpu/GrContext.cpp:208: void GrContext::flush(int flagsBitfield) {
> On 2016/01/06 14:03:27, joshualitt wrote:
> > On 2016/01/05 22:55:45, mtklein wrote:
> > > On 2016/01/05 22:47:50, mtklein wrote:
> > > > Why do some methods TAKE_DEBUG_MUTEX and not others?
> > > 
> > > Oh, I see.  That's very confusing.
> > 
> > I can explicitly break it out of the macro if that makes it easier to follow
> 
> I think it would.
done

[mtklein, 2016-01-06 15:08:27]

https://codereview.chromium.org/1555953004/diff/60001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1555953004/diff/60001/include/gpu/GrContext.h...
include/gpu/GrContext.h:393: // In debug builds we take a debug mutex to guard
against improper thread handling
I'd put it right here.

[joshualitt, 2016-01-06 15:09:47]

https://codereview.chromium.org/1555953004/diff/60001/include/gpu/GrContext.h
File include/gpu/GrContext.h (right):

https://codereview.chromium.org/1555953004/diff/60001/include/gpu/GrContext.h...
include/gpu/GrContext.h:393: // In debug builds we take a debug mutex to guard
against improper thread handling
On 2016/01/06 15:08:27, mtklein wrote:
> I'd put it right here.

This will end up being used by a number of Gr* classes and SkGpuDevice.  I can
make it GrSingleOwner and move it to its own header.

[mtklein, 2016-01-06 15:16:03]

> This will end up being used by a number of Gr* classes and SkGpuDevice.  I can
> make it GrSingleOwner and move it to its own header.

Ok, I don't not believe you, but why not do it then?

[joshualitt, 2016-01-06 15:22:17]

On 2016/01/06 15:16:03, mtklein wrote:
> > This will end up being used by a number of Gr* classes and SkGpuDevice.  I
can
> > make it GrSingleOwner and move it to its own header.
> 
> Ok, I don't not believe you, but why not do it then?

Because literally the next CL I'm putting together is to do this to
GrDrawContext.  It takes 2 seconds, may as well just do it now.

[mtklein, 2016-01-06 15:23:58]

On 2016/01/06 15:22:17, joshualitt wrote:
> On 2016/01/06 15:16:03, mtklein wrote:
> > > This will end up being used by a number of Gr* classes and SkGpuDevice.  I
> can
> > > make it GrSingleOwner and move it to its own header.
> > 
> > Ok, I don't not believe you, but why not do it then?
> 
> Because literally the next CL I'm putting together is to do this to
> GrDrawContext.  It takes 2 seconds, may as well just do it now.

That makes sense if you add the changes to this CL.

[joshualitt, 2016-01-06 15:40:31]

On 2016/01/06 15:23:58, mtklein wrote:
> On 2016/01/06 15:22:17, joshualitt wrote:
> > On 2016/01/06 15:16:03, mtklein wrote:
> > > > This will end up being used by a number of Gr* classes and SkGpuDevice. 
I
> > can
> > > > make it GrSingleOwner and move it to its own header.
> > > 
> > > Ok, I don't not believe you, but why not do it then?
> > 
> > Because literally the next CL I'm putting together is to do this to
> > GrDrawContext.  It takes 2 seconds, may as well just do it now.
> 
> That makes sense if you add the changes to this CL.

done

[mtklein, 2016-01-06 15:42:54]

lgtm

[joshualitt, 2016-01-06 15:43:15]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-01-06 15:43:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com
Link to the patchset: https://codereview.chromium.org/1555953004/#ps120001
(title: "tweaks")

[commit-bot: I haz the power, 2016-01-06 15:43:26]

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

[commit-bot: I haz the power, 2016-01-06 15:44:26]

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

[commit-bot: I haz the power, 2016-01-06 15:44:27]

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

[joshualitt, 2016-01-06 16:08:22]

The CQ bit was checked by joshualitt@google.com

[joshualitt, 2016-01-06 16:08:22]

The patchset sent to the CQ was uploaded after l-g-t-m from
robertphillips@google.com, mtklein@google.com
Link to the patchset: https://codereview.chromium.org/1555953004/#ps140001
(title: "tweaks")

[commit-bot: I haz the power, 2016-01-06 16:08:30]

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

[commit-bot: I haz the power, 2016-01-06 16:09:51]

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

[commit-bot: I haz the power, 2016-01-06 16:09:52]

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

[joshualitt, 2016-01-06 16:12:08]

Description was changed from

==========
Create debug only SkSingleOwner

This is so Gpu code can guard against improper multithreaded usage in
debug builds

BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Create debug only SkSingleOwner

This is so Gpu code can guard against improper multithreaded usage in
debug builds

TBR=bsalomon@google.com
BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

[joshualitt, 2016-01-06 16:16:12]

The CQ bit was checked by joshualitt@google.com

[commit-bot: I haz the power, 2016-01-06 16:16:18]

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

[commit-bot: I haz the power, 2016-01-06 16:26:11]

Description was changed from

==========
Create debug only SkSingleOwner

This is so Gpu code can guard against improper multithreaded usage in
debug builds

TBR=bsalomon@google.com
BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...
==========

to

==========
Create debug only SkSingleOwner

This is so Gpu code can guard against improper multithreaded usage in
debug builds

TBR=bsalomon@google.com
BUG=skia:
GOLD_TRYBOT_URL=
https://gold.skia.org/search2?unt=true&query=source_type%3Dgm&master=false&is...

Committed:
https://skia.googlesource.com/skia/+/1de610a5287cf61d4f3a1fdc7413bd74827a8b6a
==========

[commit-bot: I haz the power, 2016-01-06 16:26:12]

Committed patchset #8 (id:140001) as
https://skia.googlesource.com/skia/+/1de610a5287cf61d4f3a1fdc7413bd74827a8b6a