add some debugging to SkNVRefCnt

add some debugging to SkNVRefCnt

BUG=skia:

Committed: https://skia.googlesource.com/skia/+/90d0ff013bbd8e5295d1517d41cb408e9d9f4d93

[reed1, 2014-11-21 22:44:52]

reed@google.com changed reviewers:
+ bsalomon@google.com, mtklein@google.com

[reed1, 2014-11-21 22:44:53]

these helped me today with the picture work...

[reed1, 2014-11-21 22:44:56]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-21 22:45:31]

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

[commit-bot: I haz the power, 2014-11-21 22:45:31]

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 2014-11-22 04:45 UTC

[bsalomon, 2014-11-21 22:48:50]

https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:273: SkDEBUGCODE(fRefCnt = 1;)   // for the assert in
our destructor
Why 1? For stack objects? Do we want to allow that?

[reed1, 2014-11-21 23:08:03]

On 2014/11/21 22:48:50, bsalomon wrote:
> https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h
> File include/core/SkRefCnt.h (right):
> 
>
https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h#newc...
> include/core/SkRefCnt.h:273: SkDEBUGCODE(fRefCnt = 1;)   // for the assert in
> our destructor
> Why 1? For stack objects? Do we want to allow that?

Hmm, good point/question. I was just trying to emulate our other class, but
perhaps wrongly. I'll post a 0 version

[reed1, 2014-11-21 23:21:46]

On 2014/11/21 23:08:03, reed1 wrote:
> On 2014/11/21 22:48:50, bsalomon wrote:
> > https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h
> > File include/core/SkRefCnt.h (right):
> > 
> >
>
https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h#newc...
> > include/core/SkRefCnt.h:273: SkDEBUGCODE(fRefCnt = 1;)   // for the assert
in
> > our destructor
> > Why 1? For stack objects? Do we want to allow that?
> 
> Hmm, good point/question. I was just trying to emulate our other class, but
> perhaps wrongly. I'll post a 0 version

Ah, perhaps(?) some wisdom from the past, perhaps not : If I switch to 0, then
NO ONE can call delete... they MUST call unref(). We have other code that wanted
to have a strong contract and deliberate was calling delete (which would assert
if we're base 0). More to ponder...

[mtklein, 2014-11-22 00:08:14]

https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:256: #ifdef SK_DEBUG
Double debug guards seem like overkill?  Should be identical without the
#ifdef...#endif.

https://codereview.chromium.org/745383003/diff/1/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:273: SkDEBUGCODE(fRefCnt = 1;)   // for the assert in
our destructor
On 2014/11/21 22:48:50, bsalomon wrote:
> Why 1? For stack objects? Do we want to allow that?

I'm game to try disallowing the stack and delete in the spirit of trying new
things.

Would that be best as,
  protected:
    ~SkNVRefCnt() { SkASSERT(0 == fRefCnt); } 
?

For reference, I had been toying with writing unref() like this:
  if (this->unique() || 1 == sk_atomic_dec(&fRefCnt)) {
    SkDELETE((const Derived*)this);
  }
to avoid the last write when we see we're unique (ought to be pretty common). 
This has the nice effect that if we never actually share the object, we never
issue a single atomic write.

If we were to do something like that, the asserts do complicate it, as it's
legal to end up with 0 or 1 in the dtor.  We'd want something like this:
  ~SkNVRefCnt() { SkASSERT(this->unique()); }
  void unref() const {
    if (this->unique()) {
       SkDELETE((const Derived*)this);
    } else if (1 == sk_atomic_dec(&fRefCnt)) {
       SkDEBUGCODE(fRefCnt = 1);
       SkDELETE((const Derived*)this);
    }
  }
That'd also allow direct deleters.

[commit-bot: I haz the power, 2014-11-22 04:45:36]

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

[commit-bot: I haz the power, 2014-11-22 04:45:37]

No LGTM from a valid reviewer yet. Please ask for an LGTM from a
full Skia committer from
https://skia.googlesource.com/skia/+/master/CQ_COMMITTERS

[reed1, 2014-11-24 16:02:30]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 16:02:43]

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

[commit-bot: I haz the power, 2014-11-24 16:02:43]

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 2014-11-24 22:02 UTC

[reed1, 2014-11-24 16:02:47]

now with must-call-unref checks. PTAL

[mtklein, 2014-11-24 16:07:27]

lgtm

https://codereview.chromium.org/745383003/diff/20001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/745383003/diff/20001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:256: #ifdef SK_DEBUG
#ifdef SK_DEBUG is still redundant here.  The dtor's a no-op in Release.

[commit-bot: I haz the power, 2014-11-24 16:21:12]

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

[commit-bot: I haz the power, 2014-11-24 16:21:14]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[reed1, 2014-11-24 16:43:13]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 16:44:02]

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

[reed1, 2014-11-24 16:44:20]

The CQ bit was unchecked by reed@google.com

[reed1, 2014-11-24 17:13:14]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 17:14:13]

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

[commit-bot: I haz the power, 2014-11-24 17:30:24]

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

[commit-bot: I haz the power, 2014-11-24 17:30:25]

Try jobs failed on following builders:
  Test-Ubuntu13.10-GCE-NoGPU-x86_64-Debug-Trybot on client.skia
(http://build.chromium.org/p/client.skia/builders/Test-Ubuntu13.10-GCE-NoGPU-x...)

[mtklein, 2014-11-24 17:34:05]

https://codereview.chromium.org/745383003/diff/60001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

https://codereview.chromium.org/745383003/diff/60001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:257: ~SkNVRefCnt() {
Oh wait, I forgot, not lgtm.  Make this protected?

[reed1, 2014-11-24 18:03:32]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 18:04:44]

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

[commit-bot: I haz the power, 2014-11-24 18:04:45]

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

[commit-bot: I haz the power, 2014-11-24 18:04:46]

A disapproval has been posted by following reviewers: mtklein@google.com.
Please make sure to get positive review before another CQ attempt.

[reed1, 2014-11-24 18:25:46]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 18:26:08]

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

[commit-bot: I haz the power, 2014-11-24 18:26:09]

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

[commit-bot: I haz the power, 2014-11-24 18:26:10]

A disapproval has been posted by following reviewers: mtklein@google.com.
Please make sure to get positive review before another CQ attempt.

[reed1, 2014-11-24 19:12:48]

#6 should fix bench

[mtklein, 2014-11-24 19:13:58]

lgtm

[reed1, 2014-11-24 19:57:28]

The CQ bit was checked by reed@google.com

[commit-bot: I haz the power, 2014-11-24 19:57:54]

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

[commit-bot: I haz the power, 2014-11-24 20:02:35]

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