Add SkNVRefCnt, prune down SkPicture's size

Add SkNVRefCnt, prune down SkPicture's size

SkNVRefCnt is a variant of SkRefCnt that's Not Virtual, so weighs 4 bytes
instead of 8 or 16.  There's only benefit to doing this if the deriving class
does not otherwise need a vtable, e.g. SkPicture.

I've stripped out some cruft from SkPicture, rearranged fields to pack tightly, 
and added compile asserts for the sizes of SkPicture, SkRecord, and
SkVarAlloc.

BUG=skia:3144
Committed: https://skia.googlesource.com/skia/+/08d1fccf6eeec0a9fd5421e59e4d05daccf6e339

[mtklein, 2014-11-19 18:54:28]

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

[tomhudson, 2014-11-19 18:59:06]

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

https://codereview.chromium.org/741793002/diff/60001/include/core/SkPicture.h...
include/core/SkPicture.h:298: mutable uint32_t fUniqueID;
Follow-on CL to move this up top for better packing?

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

https://codereview.chromium.org/741793002/diff/60001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:249: 
Can we put something like a short version of the CL description here as a class
header comment? Since there's an abbreviation in the class name, the purpose of
the class isn't clear, and even spelling out the abbreviation leaves it
inobvious.

[mtklein, 2014-11-19 19:19:15]

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

https://codereview.chromium.org/741793002/diff/60001/include/core/SkPicture.h...
include/core/SkPicture.h:298: mutable uint32_t fUniqueID;
On 2014/11/19 18:59:06, tomhudson wrote:
> Follow-on CL to move this up top for better packing?

Done.

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

https://codereview.chromium.org/741793002/diff/60001/include/core/SkRefCnt.h#...
include/core/SkRefCnt.h:249: 
On 2014/11/19 18:59:06, tomhudson wrote:
> Can we put something like a short version of the CL description here as a
class
> header comment? Since there's an abbreviation in the class name, the purpose
of
> the class isn't clear, and even spelling out the abbreviation leaves it
> inobvious.

Done.

[tomhudson, 2014-11-19 20:05:25]

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

[tomhudson, 2014-11-19 20:05:26]

Mostly LGTM; beside the one comment inline I wonder if 184 -> 144 isn't too big
a drop, so fired off the trybots that have been problematic.

https://codereview.chromium.org/741793002/diff/140001/include/core/SkPicture.h
File include/core/SkPicture.h (left):

https://codereview.chromium.org/741793002/diff/140001/include/core/SkPicture....
include/core/SkPicture.h:64: typedef SkRefCnt INHERITED;
Did I miss a reason why you removed the INHERITED from the inner AccelData
class?

[mtklein, 2014-11-19 20:08:46]

https://codereview.chromium.org/741793002/diff/140001/include/core/SkPicture.h
File include/core/SkPicture.h (left):

https://codereview.chromium.org/741793002/diff/140001/include/core/SkPicture....
include/core/SkPicture.h:64: typedef SkRefCnt INHERITED;
On 2014/11/19 20:05:26, tomhudson wrote:
> Did I miss a reason why you removed the INHERITED from the inner AccelData
> class?

It's unused.  Can't do anything but confuse us.

[tomhudson, 2014-11-19 20:47:44]

On 2014/11/19 20:05:26, tomhudson wrote:
> Mostly LGTM; beside the one comment inline I wonder if 184 -> 144 isn't too
big
> a drop, so fired off the trybots that have been problematic.


Trybots seem happy to confirm that you've cut peak size by >20%; I have no more
concerns.

[reed2, 2014-11-20 02:03:30]

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

[reed2, 2014-11-20 02:03:30]

I think SkRecorder::newDrawableSnapshot will be broken by this, but not sure.
It's custom SkData handler assumed the contents were an array SkRefCnt*, which
it knew how to unref. Can be fixed obviously, but it is a small example of
losing a little bit of generality.

[mtklein, 2014-11-20 14:15:24]

On 2014/11/20 02:03:30, reed2 wrote:
> I think SkRecorder::newDrawableSnapshot will be broken by this, but not sure.
> It's custom SkData handler assumed the contents were an array SkRefCnt*, which
> it knew how to unref. Can be fixed obviously, but it is a small example of
> losing a little bit of generality.

Oh, very true.  Fixed.

This is why I'm spooked to use SkData for newDrawableSnapshot.  We
reinterpret_cast away all the type safety.

[reed1, 2014-11-20 17:06:20]

On 2014/11/20 14:15:24, mtklein wrote:
> On 2014/11/20 02:03:30, reed2 wrote:
> > I think SkRecorder::newDrawableSnapshot will be broken by this, but not
sure.
> > It's custom SkData handler assumed the contents were an array SkRefCnt*,
which
> > it knew how to unref. Can be fixed obviously, but it is a small example of
> > losing a little bit of generality.
> 
> Oh, very true.  Fixed.
> 
> This is why I'm spooked to use SkData for newDrawableSnapshot.  We
> reinterpret_cast away all the type safety.

And why I'm spooked by losing the predictability of a uniform refcnt base :)

[reed1, 2014-11-20 17:07:40]

with futuristic misgivings :) lgtm

[mtklein, 2014-11-20 17:09:01]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-11-20 17:09:16]

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

[commit-bot: I haz the power, 2014-11-20 17:18:42]

Committed patchset #10 (id:170001) as
https://skia.googlesource.com/skia/+/08d1fccf6eeec0a9fd5421e59e4d05daccf6e339

[f(malita), 2014-11-20 19:31:58]

On 2014/11/20 17:18:42, I haz the power (commit-bot) wrote:
> Committed patchset #10 (id:170001) as
> https://skia.googlesource.com/skia/+/08d1fccf6eeec0a9fd5421e59e4d05daccf6e339

What's the plan for Blink interoperability? We wrap SkRefCnt-derived objects in
WTF::RefPtr<> all over the place and them canaries aren't happy about this.

The RefPtr template uses ::deref() instead of ::unref(), and that's currently
duct-taped in skia/config/{sk_ref_cnt_ext_release.h,sk_ref_cnt_ext_debug.h}.

Would adding an ::deref() alias suffice?

[humper, 2014-11-20 21:39:29]

A revert of this CL (patchset #10 id:170001) has been created in
https://codereview.chromium.org/749683002/ by humper@google.com.

The reason for reverting is: Breaking Canary and DEPS roll.