Remove ability for Release code to call getRefCnt() or getWeakRefCnt().

Remove ability for Release code to call getRefCnt() or getWeakRefCnt().

These getRefCnt() methods are not thread safe, so Skia code should not
be calling them.  unique() is fine.

SkDEBUG code (SkASSERTs) can still call getRefCnt() / getWeakRefCnt().

This adds tools/RefCntIs.{h,cpp}, which lets tests make their assertions in
both debug and release modes.

BUG=skia:2726
Committed: https://skia.googlesource.com/skia/+/4ae94ffce5ecf1b71cb5e295b68bf4ec9e697443

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

[Stephen White, 2014-07-07 21:10:27]

https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp#n...
src/core/SkImageFilter.cpp:427: //if (key->getRefCnt() >= fMinChildren) {
I would ask that you not land this as-is, since it will increase the RAM and
VRAM footprint of filters rather significantly.

The use of the refcount here is meant to approximate the "valence" of an image
filter, ie., how many children it has. In the default case, we don't cache a
filter unless it has more than 1 child (ie., unless we know we're going to
revisit the node during the same traversal). The correct way to fix this would
be to have an actual child count in each image filter, which is incremented when
a child is added, and decremented when it is removed.

I am scoping out some work for a replacement cache, which shouldn't depend on
valence, but it may be some time before it's done.

[reed1, 2014-07-07 21:12:54]

https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp#n...
src/core/SkImageFilter.cpp:427: //if (key->getRefCnt() >= fMinChildren) {
On 2014/07/07 21:10:27, Stephen White wrote:
> I would ask that you not land this as-is, since it will increase the RAM and
> VRAM footprint of filters rather significantly.
> 
> The use of the refcount here is meant to approximate the "valence" of an image
> filter, ie., how many children it has. In the default case, we don't cache a
> filter unless it has more than 1 child (ie., unless we know we're going to
> revisit the node during the same traversal). The correct way to fix this would
> be to have an actual child count in each image filter, which is incremented
when
> a child is added, and decremented when it is removed.
> 
> I am scoping out some work for a replacement cache, which shouldn't depend on
> valence, but it may be some time before it's done.

Perhaps we can add a block-comment describing what you just said, and maybe file
a bug so we track this concern.

[mtklein, 2014-07-07 22:25:23]

https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp
File src/core/SkImageFilter.cpp (right):

https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp#n...
src/core/SkImageFilter.cpp:427: //if (key->getRefCnt() >= fMinChildren) {
So, would something like PS 2 work until the replacement cache?  Using !unique()
is the same as getRefCnt() > 1, which is the same as the existing getRefCnt() >=
2, so the logic's still the same, but it's going through methods that can stay
visible, and is commented up to remind us that !unique() means "at some point >
1, but that might have changed already".

Aren't we adding things to the cache if they have 1 or more children? 
Presumably something else holds one ref on the SkImageFilter already, right? 
Then the second is taken by the first child?

[Stephen White, 2014-07-08 00:47:26]

On 2014/07/07 22:25:23, mtklein wrote:
> https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp
> File src/core/SkImageFilter.cpp (right):
> 
>
https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp#n...
> src/core/SkImageFilter.cpp:427: //if (key->getRefCnt() >= fMinChildren) {
> So, would something like PS 2 work until the replacement cache?

Not quite -- see below.

> Using !unique()
> is the same as getRefCnt() > 1, which is the same as the existing getRefCnt()
>=
> 2, so the logic's still the same, but it's going through methods that can stay
> visible, and is commented up to remind us that !unique() means "at some point
>
> 1, but that might have changed already".

> Aren't we adding things to the cache if they have 1 or more children? 
> Presumably something else holds one ref on the SkImageFilter already, right? 
> Then the second is taken by the first child?

The SkPaint holds a ref onto the root node of the DAG, and from there, each node
refs its inputs. So the root node has exactly one ref, and each other node has
one or more refs (depending on its valence). (During construction, the nodes may
actually have more refs, but by them time the SkPaint holds it, it's as above).

Having said that, I realize I didn't mention that the cache is actually used in
two modes: in the "within Skia" mode, described above, the cache is instantiated
just before drawing, and minChildren is set to 2. This mode is used in impl-side
painting, and is only used for intra-frame caching (ie., only while drawing a
single frame/tile). The cache is destroyed at end of frame/tile, so it's just
used to avoid re-processing the same node during a single traversal.

There's also an "external cache" mode, which is used by non-impl-side painting.
This mode is set by Blink, and creates a cache with fMinChildren set to 1. This
means we actually cache all results, until they are manually invalidated by
Blink. This cache is persistent, and maintained from frame-to-frame, and
emulates the legacy FilterEffect code in Blink which has this behaviour: nodes
which don't change from frame to frame return their cached results. (The new
cache should bring frame-to-frame cacheing to the impl-side painting case, which
is the more important case going forward anyway, since every platform will
switch to impl-side painting.)

In the interim, I think we could do something like "if (fMinChildren == 1 ||
!unique()) { ... }

Then the non-impl-side case will take the first clause, and the impl-side case
will take the second.

[mtklein, 2014-07-08 01:12:53]

On 2014/07/08 00:47:26, Stephen White wrote:
> On 2014/07/07 22:25:23, mtklein wrote:
> > https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp
> > File src/core/SkImageFilter.cpp (right):
> > 
> >
>
https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp#n...
> > src/core/SkImageFilter.cpp:427: //if (key->getRefCnt() >= fMinChildren) {
> > So, would something like PS 2 work until the replacement cache?
> 
> Not quite -- see below.
> 
> > Using !unique()
> > is the same as getRefCnt() > 1, which is the same as the existing
getRefCnt()
> >=
> > 2, so the logic's still the same, but it's going through methods that can
stay
> > visible, and is commented up to remind us that !unique() means "at some
point
> >
> > 1, but that might have changed already".
> 
> > Aren't we adding things to the cache if they have 1 or more children? 
> > Presumably something else holds one ref on the SkImageFilter already, right?

> > Then the second is taken by the first child?
> 
> The SkPaint holds a ref onto the root node of the DAG, and from there, each
node
> refs its inputs. So the root node has exactly one ref, and each other node has
> one or more refs (depending on its valence). (During construction, the nodes
may
> actually have more refs, but by them time the SkPaint holds it, it's as
above).
> 
> Having said that, I realize I didn't mention that the cache is actually used
in
> two modes: in the "within Skia" mode, described above, the cache is
instantiated
> just before drawing, and minChildren is set to 2. This mode is used in
impl-side
> painting, and is only used for intra-frame caching (ie., only while drawing a
> single frame/tile). The cache is destroyed at end of frame/tile, so it's just
> used to avoid re-processing the same node during a single traversal.
> 
> There's also an "external cache" mode, which is used by non-impl-side
painting.
> This mode is set by Blink, and creates a cache with fMinChildren set to 1.
This
> means we actually cache all results, until they are manually invalidated by
> Blink. This cache is persistent, and maintained from frame-to-frame, and
> emulates the legacy FilterEffect code in Blink which has this behaviour: nodes
> which don't change from frame to frame return their cached results. (The new
> cache should bring frame-to-frame cacheing to the impl-side painting case,
which
> is the more important case going forward anyway, since every platform will
> switch to impl-side painting.)
> 
> In the interim, I think we could do something like "if (fMinChildren == 1 ||
> !unique()) { ... }
> 
> Then the non-impl-side case will take the first clause, and the impl-side case
> will take the second.

How's this look?

[Stephen White, 2014-07-08 02:31:50]

On 2014/07/08 01:12:53, mtklein wrote:
> On 2014/07/08 00:47:26, Stephen White wrote:
> > On 2014/07/07 22:25:23, mtklein wrote:
> > >
https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp
> > > File src/core/SkImageFilter.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/378643003/diff/1/src/core/SkImageFilter.cpp#n...
> > > src/core/SkImageFilter.cpp:427: //if (key->getRefCnt() >= fMinChildren) {
> > > So, would something like PS 2 work until the replacement cache?
> > 
> > Not quite -- see below.
> > 
> > > Using !unique()
> > > is the same as getRefCnt() > 1, which is the same as the existing
> getRefCnt()
> > >=
> > > 2, so the logic's still the same, but it's going through methods that can
> stay
> > > visible, and is commented up to remind us that !unique() means "at some
> point
> > >
> > > 1, but that might have changed already".
> > 
> > > Aren't we adding things to the cache if they have 1 or more children? 
> > > Presumably something else holds one ref on the SkImageFilter already,
right?
> 
> > > Then the second is taken by the first child?
> > 
> > The SkPaint holds a ref onto the root node of the DAG, and from there, each
> node
> > refs its inputs. So the root node has exactly one ref, and each other node
has
> > one or more refs (depending on its valence). (During construction, the nodes
> may
> > actually have more refs, but by them time the SkPaint holds it, it's as
> above).
> > 
> > Having said that, I realize I didn't mention that the cache is actually used
> in
> > two modes: in the "within Skia" mode, described above, the cache is
> instantiated
> > just before drawing, and minChildren is set to 2. This mode is used in
> impl-side
> > painting, and is only used for intra-frame caching (ie., only while drawing
a
> > single frame/tile). The cache is destroyed at end of frame/tile, so it's
just
> > used to avoid re-processing the same node during a single traversal.
> > 
> > There's also an "external cache" mode, which is used by non-impl-side
> painting.
> > This mode is set by Blink, and creates a cache with fMinChildren set to 1.
> This
> > means we actually cache all results, until they are manually invalidated by
> > Blink. This cache is persistent, and maintained from frame-to-frame, and
> > emulates the legacy FilterEffect code in Blink which has this behaviour:
nodes
> > which don't change from frame to frame return their cached results. (The new
> > cache should bring frame-to-frame cacheing to the impl-side painting case,
> which
> > is the more important case going forward anyway, since every platform will
> > switch to impl-side painting.)
> > 
> > In the interim, I think we could do something like "if (fMinChildren == 1 ||
> > !unique()) { ... }
> > 
> > Then the non-impl-side case will take the first clause, and the impl-side
case
> > will take the second.
> 
> How's this look?

Filters LGTM.

[mtklein, 2014-07-08 11:48:26]

Have a look Mike?

[reed1, 2014-07-08 13:10:56]

lgtm

https://codereview.chromium.org/378643003/diff/60001/tools/RefCntIs.h
File tools/RefCntIs.h (right):

https://codereview.chromium.org/378643003/diff/60001/tools/RefCntIs.h#newcode10
tools/RefCntIs.h:10: bool RefCntIs(const SkRefCntBase&, int32_t);
Seems out of place to have these w/ no namespace or class or prefix, but ok.

[mtklein, 2014-07-08 13:17:54]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-07-08 13:18:11]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/378643003/60001

[commit-bot: I haz the power, 2014-07-08 13:48:24]

Change committed as 4ae94ffce5ecf1b71cb5e295b68bf4ec9e697443

[mtklein, 2014-07-09 12:58:01]

A revert of this CL has been created in
https://codereview.chromium.org/380703002/ by mtklein@google.com.

The reason for reverting is: External unit tests call getRefCnt()..

[mtklein, 2014-07-09 14:30:41]

Going to reland just the SkImageFilter changes for now, then follow up hiding
getRefCnt().

[mtklein, 2014-07-09 14:30:55]

The CQ bit was checked by mtklein@google.com

[commit-bot: I haz the power, 2014-07-09 14:32:00]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/378643003/80001

[commit-bot: I haz the power, 2014-07-09 14:52:37]

Change committed as bd7746da97e389c1068333117407b213b378f9db