WeakRefCnt

[bungeman, 2012-02-09 22:08:18]

This change introduces SkWeakRefCnt and SkWeakRefCntable and makes SkTypeface
SkWeakRefCntable. The changes to SkFontHost_win and ports.gyp are not part of
this CL proper, but are here to show how this would be used. SkTypefaceCache
still passes SkTypeface in and out, but now only holds weak references to them
in SkWeakRefCnt<SkTypeface>.

Note also the changes in SkTypefaceCache presuppose returning refed SkTypefaces
to avoid threading issues. The SkFontHost_win here contains the changes which
will be required.

[TomH, 2012-02-09 22:10:18]

Why do we need this?

http://codereview.appspot.com/5649046/diff/1/src/core/SkWeakRefCnt.h
File src/core/SkWeakRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/1/src/core/SkWeakRefCnt.h#newcode13
src/core/SkWeakRefCnt.h:13: A strong SkWeakRefCnt::expired() will always return
false.
Eww, can we have better naming?

[bungeman, 2012-02-09 22:31:10]

The reason we want this is for typefaces created from streams. We need to be
able to put these typefaces into the typeface cache because we need to be able
to look them up by id, etc. Currently, the typeface cache keeps a reference to
the typefaces passed into it, and this reference prevents the typeface's
destructor from running. We really want the destructor to run once the user no
longer has a reference because otherwise we won't free the typeface data until
the typeface cache is purged. Furthermore, we know it is safe to remove a
typeface created from a stream as soon as the user loses their last reference to
it, as even before that typeface could be purged from the cache at any time.

There are two normal ways of going about solving this. The most common is with
external ref counts, like when using scoped_ptr and weak_ptr. Then the cache
would just hold weak_ptr. With internal ref counts, like in COM, a split
identity pattern is used, but this requires unref to be virtual. In our case we
have an internal ref count without a virtual unref, and this was the cleanest
and most general way I could come up with to do something like weak references
under these constraints.

http://codereview.appspot.com/5649046/diff/1/src/core/SkWeakRefCnt.h
File src/core/SkWeakRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/1/src/core/SkWeakRefCnt.h#newcode13
src/core/SkWeakRefCnt.h:13: A strong SkWeakRefCnt::expired() will always return
false.
On 2012/02/09 22:10:18, TomH wrote:
> Eww, can we have better naming?

I've thought about it, but I'm not sure what else to call it. I'm open to
suggestions and will think about it some more.

[bungeman, 2012-02-10 18:57:07]

A different approach will need to be taken.

http://codereview.appspot.com/5649046/diff/1/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/5649046/diff/1/src/ports/SkFontHost_win.cpp#new...
src/ports/SkFontHost_win.cpp:171: SkSafeRef(fTarget);
This isn't safe. It violates the constraint that only those with a ref already
may call ref.

[bungeman, 2012-02-16 20:36:16]

This makes the ref and unref on SkTypeface virtual. Once again the gyp and font
host changes are just to show how this would be used. If this approach is taken,
we will need to update flattenable, picture, and pipe since SkTypeface would no
longer be a SkRefCnt.

[bungeman, 2012-02-17 18:26:21]

Patch set 3 is a draft of how to allow SkRefCnt objects to be weakly usable.
This is rather loosely based on shared_ptr and weak_ptr implementations, but in
this case the shared_ptr, weak_ptr, counter, and referenced object are all the
same entity.

This is a weak version of weak pointerness, in that when the last strong
reference goes away the referenced object is not destroyed but instead can
release resources it owns and place itself in an invalid state (though it need
not do so). The referenced object (because it is also the count) is not deleted
until the last weak_unref. In other words, a weakly held SkRefCnt will still
strongly hold the SkRefCnt object's memory, but does not prevent any derived
class' fields and methods from becoming invalid.

This change will require (as noted in SkRefCnt.h) a new function in
SkThread_platform, sk_atomic_conditional_inc, which only incs when the value is
not 0.

[bungeman, 2012-04-11 22:27:38]

After much todo, this is now ready for review. As mentioned before, this CL
contains more than just the required changes in order to show how this would be
used.

The only real objection I still have is that a simple ref(), unref() sequence is
now 3 atomic operations instead of the previous two, since we need at least one
additional atomic operation to manage the weakness. I'm not sure of a good way
to avoid this yet.

[Steve VanDeBogart, 2012-04-11 22:48:54]

A couple drive-by comments.

http://codereview.appspot.com/5649046/diff/13001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/13001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:99: bool weak_lock() const {
I can see how it might be pretty easy to ignore the return value of this method.
 Does the false case only happen when weak_lock() is called from within
dispose()?  If so, could you just validate that fRefCnt is still zero after
dispose before calling weak_unref() ?

http://codereview.appspot.com/5649046/diff/13001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:157: mutable int32_t fWeakCnt;
RefCounted objects are now a pointer bigger?  This is one reason you might
consider a separate type for a weak pointer class.

[bungeman, 2012-04-11 23:18:45]

http://codereview.appspot.com/5649046/diff/13001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/13001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:99: bool weak_lock() const {
On 2012/04/11 22:48:54, Steve VanDeBogart wrote:
> I can see how it might be pretty easy to ignore the return value of this
method.
>  Does the false case only happen when weak_lock() is called from within
> dispose()?  If so, could you just validate that fRefCnt is still zero after
> dispose before calling weak_unref() ?

This should be considered to be able to return false at any time the caller is
unsure if a strong reference already exists. Since it is always an error to
ignore this value, I will add the new SK_WARN_UNUSED_RESULT to it.

http://codereview.appspot.com/5649046/diff/13001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:157: mutable int32_t fWeakCnt;
On 2012/04/11 22:48:54, Steve VanDeBogart wrote:
> RefCounted objects are now a pointer bigger?  This is one reason you might
> consider a separate type for a weak pointer class.

The objects will be 32 bits bigger. The biggest difficulty with having two
RefCnt classes is that we inherit from them rather than just use them. Having
two affects the class hierarchy as well.

[bungeman, 2012-05-07 22:33:46]

This splits SkWeakRefCnt out into its own class which inherits from SkRefCnt.
This reduces the footprint of a SkRefCnt instance to what it was before, and
eliminates the need for SkRefCnt only objects to decrement the weak count (since
they don't have one). The only added cost to SkRefCnt is that there are now two
virtual calls when the reference count drops to zero; it now calls
'internal_dispose' which then calls 'delete'. On the SkWeakRefCnt side there is
a similar additional cost from the previous patch set (7).

[Steve VanDeBogart, 2012-05-08 00:21:46]

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:111: /** Default construct, initializing the reference
counts to 1.
Does this mean that RefCnt is 1 and weak ref count is 1?  Thus, if I construct a
weak ref object, I have to unref and weak_unref it to destroy it?
It seems to me that the object should start in the same state as a RefCnt object
so that a weakly ref counted object could be used in the same situation/same
sequence of code that a strongly ref counted object is used in.

I think the invariant argument below explains this, but a more direct comment
here might help.

http://codereview.appspot.com/5649046/diff/23001/include/core/SkThread_platfo...
File include/core/SkThread_platform.h (right):

http://codereview.appspot.com/5649046/diff/23001/include/core/SkThread_platfo...
include/core/SkThread_platform.h:29: void sk_membar_aquire__after_atomic_dec() {
}
Why the double underscore?

http://codereview.appspot.com/5649046/diff/23001/src/core/SkTypefaceCache.cpp
File src/core/SkTypefaceCache.cpp (right):

http://codereview.appspot.com/5649046/diff/23001/src/core/SkTypefaceCache.cpp...
src/core/SkTypefaceCache.cpp:16: void SkTypefaceCache::add(SkTypeface* face,
SkTypeface::Style requestedStyle, bool strong) {
nit: 80 column line wrap and other instances.

[bungeman, 2012-05-08 13:51:52]

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:111: /** Default construct, initializing the reference
counts to 1.
On 2012/05/08 00:21:47, Steve VanDeBogart wrote:
> Does this mean that RefCnt is 1 and weak ref count is 1?  Thus, if I construct
a
> weak ref object, I have to unref and weak_unref it to destroy it?
> It seems to me that the object should start in the same state as a RefCnt
object
> so that a weakly ref counted object could be used in the same situation/same
> sequence of code that a strongly ref counted object is used in.
> 
> I think the invariant argument below explains this, but a more direct comment
> here might help.

Just for clarity, the answer to the question is no, you should only weak_unref
if you have weak_refed. A SkWeakRefCnt is substitutable for a SkRefCnt and can
be used as such.

The way to look at the algorithm is that the strong refs collectively hold one
weak ref. When the strong ref goes to zero, this weak ref is released. This
would be helpful to document, if only to aid in debugging. Otherwise it could be
frustrating trying to determine why the weak ref count appears to often be one
greater than one expects.

http://codereview.appspot.com/5649046/diff/23001/include/core/SkThread_platfo...
File include/core/SkThread_platform.h (right):

http://codereview.appspot.com/5649046/diff/23001/include/core/SkThread_platfo...
include/core/SkThread_platform.h:29: void sk_membar_aquire__after_atomic_dec() {
}
On 2012/05/08 00:21:47, Steve VanDeBogart wrote:
> Why the double underscore?

Somewhat because that's the way the Linux kernel does it, and that's where most
experience with these sorts of things comes from. The rationale is that there
can exist a sk_membar_aquire, but then there are specific versions to be used
around specific kinds of operations (in case that platform doesn't actually need
a membar in those situations). So the name has two parts, what it does and where
it is used. I'm not sure of a better naming convention.

http://codereview.appspot.com/5649046/diff/23001/src/core/SkTypefaceCache.cpp
File src/core/SkTypefaceCache.cpp (right):

http://codereview.appspot.com/5649046/diff/23001/src/core/SkTypefaceCache.cpp...
src/core/SkTypefaceCache.cpp:16: void SkTypefaceCache::add(SkTypeface* face,
SkTypeface::Style requestedStyle, bool strong) {
On 2012/05/08 00:21:47, Steve VanDeBogart wrote:
> nit: 80 column line wrap and other instances.

Done.

[Steve VanDeBogart, 2012-05-08 22:52:45]

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:111: /** Default construct, initializing the reference
counts to 1.
On 2012/05/08 13:51:53, bungeman wrote:
> On 2012/05/08 00:21:47, Steve VanDeBogart wrote:
> > Does this mean that RefCnt is 1 and weak ref count is 1?  Thus, if I
construct
> a
> > weak ref object, I have to unref and weak_unref it to destroy it?
> > It seems to me that the object should start in the same state as a RefCnt
> object
> > so that a weakly ref counted object could be used in the same situation/same
> > sequence of code that a strongly ref counted object is used in.
> > 
> > I think the invariant argument below explains this, but a more direct
comment
> > here might help.
> 
> Just for clarity, the answer to the question is no, you should only weak_unref
> if you have weak_refed. A SkWeakRefCnt is substitutable for a SkRefCnt and can
> be used as such.
> 
> The way to look at the algorithm is that the strong refs collectively hold one
> weak ref. When the strong ref goes to zero, this weak ref is released. This
> would be helpful to document, if only to aid in debugging. Otherwise it could
be
> frustrating trying to determine why the weak ref count appears to often be one
> greater than one expects.

Hmm, is that necessary. Couldn't the internal_dispose of the weak ref count
class delete(this) if the weak ref count was already zero instead of doing a
weak_unref()?

http://codereview.appspot.com/5649046/diff/30001/include/core/SkThread_platfo...
File include/core/SkThread_platform.h (right):

http://codereview.appspot.com/5649046/diff/30001/include/core/SkThread_platfo...
include/core/SkThread_platform.h:31: static __attribute__((always_inline))
int32_t sk_atomic_conditional_inc(int32_t* addr) {
nit: 80 col

http://codereview.appspot.com/5649046/diff/30001/src/core/SkTypefaceCache.h
File src/core/SkTypefaceCache.h (right):

http://codereview.appspot.com/5649046/diff/30001/src/core/SkTypefaceCache.h#n...
src/core/SkTypefaceCache.h:45: static void Add(SkTypeface*, SkTypeface::Style
requested, bool strong = true);
nit: 80 col

http://codereview.appspot.com/5649046/diff/30001/src/ports/SkFontHost_win.cpp
File src/ports/SkFontHost_win.cpp (right):

http://codereview.appspot.com/5649046/diff/30001/src/ports/SkFontHost_win.cpp...
src/ports/SkFontHost_win.cpp:168: CustomLogFontTypeface(SkTypeface::Style style,
SkFontID fontID, const LOGFONT& lf, HANDLE handle) :
nit: 80 col + more in this file.

http://codereview.appspot.com/5649046/diff/30001/src/ports/SkThread_win.cpp
File src/ports/SkThread_win.cpp (right):

http://codereview.appspot.com/5649046/diff/30001/src/ports/SkThread_win.cpp#n...
src/ports/SkThread_win.cpp:37: if
(_InterlockedCompareExchange(reinterpret_cast<LONG*>(addr), value + 1, value) ==
value) {
nit: 80 col

http://codereview.appspot.com/5649046/diff/30001/tests/FontHostStreamTest.cpp
File tests/FontHostStreamTest.cpp (right):

http://codereview.appspot.com/5649046/diff/30001/tests/FontHostStreamTest.cpp...
tests/FontHostStreamTest.cpp:75:
paint.setTypeface(SkTypeface::CreateFromName("Georgia",
SkTypeface::kNormal))->unref();
nit: 80 col + more in this file.

[bungeman, 2012-05-09 18:56:19]

I'll fix up the line lengths before checking anything in.

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:111: /** Default construct, initializing the reference
counts to 1.
On 2012/05/08 22:52:45, Steve VanDeBogart wrote:
> On 2012/05/08 13:51:53, bungeman wrote:
> > On 2012/05/08 00:21:47, Steve VanDeBogart wrote:
> > > Does this mean that RefCnt is 1 and weak ref count is 1?  Thus, if I
> construct
> > a
> > > weak ref object, I have to unref and weak_unref it to destroy it?
> > > It seems to me that the object should start in the same state as a RefCnt
> > object
> > > so that a weakly ref counted object could be used in the same
situation/same
> > > sequence of code that a strongly ref counted object is used in.
> > > 
> > > I think the invariant argument below explains this, but a more direct
> comment
> > > here might help.
> > 
> > Just for clarity, the answer to the question is no, you should only
weak_unref
> > if you have weak_refed. A SkWeakRefCnt is substitutable for a SkRefCnt and
can
> > be used as such.
> > 
> > The way to look at the algorithm is that the strong refs collectively hold
one
> > weak ref. When the strong ref goes to zero, this weak ref is released. This
> > would be helpful to document, if only to aid in debugging. Otherwise it
could
> be
> > frustrating trying to determine why the weak ref count appears to often be
one
> > greater than one expects.
> 
> Hmm, is that necessary. Couldn't the internal_dispose of the weak ref count
> class delete(this) if the weak ref count was already zero instead of doing a
> weak_unref()?

Because the test in weak_unref is not 'has fWeakCnt become 0?' (as it may
appear) but is 'have both fRefCnt *and* fWeakCnt become 0?'.

I am assuming that what you are trying to propose is to initialize fWeakCnt to 0
and then when the fRefCnt goes to 0 delete the object when fWeakCnt goes to 0.
The issue with this is when someone does have a weak reference. The last strong
ref goes away and fRefCnt goes to 0. The object is now in the disposed state but
still exists. How do we then implement weak_unref to do the right thing? Note
that the fWeakCnt could go to 0 while fRefCnt is still > 0. As stated above,
weak_unref needs to know that both fRefCnt and fWeakCnt have gone to 0.

Note that as is, SkWeakRefCnt::internal_dispose() could be 'optimized' by
testing fWeakCnt for being 1 (just the collectively held weak ref) and then call
SkRefCnt::internal_dispose() directly (see patch set 10). This would skip one
atomic decrement. I'm not sure this is worth the added code size and complexity.
Such an optimization, should it be discovered useful, could be added later if
desired.

[Steve VanDeBogart, 2012-05-10 00:05:16]

On 2012/05/09 18:56:19, bungeman wrote:
> I'll fix up the line lengths before checking anything in.
> 
> http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h
> File include/core/SkRefCnt.h (right):
> 
>
http://codereview.appspot.com/5649046/diff/23001/include/core/SkRefCnt.h#newc...
> include/core/SkRefCnt.h:111: /** Default construct, initializing the reference
> counts to 1.
> On 2012/05/08 22:52:45, Steve VanDeBogart wrote:
> > On 2012/05/08 13:51:53, bungeman wrote:
> > > On 2012/05/08 00:21:47, Steve VanDeBogart wrote:
> > > > Does this mean that RefCnt is 1 and weak ref count is 1?  Thus, if I
> > construct
> > > a
> > > > weak ref object, I have to unref and weak_unref it to destroy it?
> > > > It seems to me that the object should start in the same state as a
RefCnt
> > > object
> > > > so that a weakly ref counted object could be used in the same
> situation/same
> > > > sequence of code that a strongly ref counted object is used in.
> > > > 
> > > > I think the invariant argument below explains this, but a more direct
> > comment
> > > > here might help.
> > > 
> > > Just for clarity, the answer to the question is no, you should only
> weak_unref
> > > if you have weak_refed. A SkWeakRefCnt is substitutable for a SkRefCnt and
> can
> > > be used as such.
> > > 
> > > The way to look at the algorithm is that the strong refs collectively hold
> one
> > > weak ref. When the strong ref goes to zero, this weak ref is released.
This
> > > would be helpful to document, if only to aid in debugging. Otherwise it
> could
> > be
> > > frustrating trying to determine why the weak ref count appears to often be
> one
> > > greater than one expects.
> > 
> > Hmm, is that necessary. Couldn't the internal_dispose of the weak ref count
> > class delete(this) if the weak ref count was already zero instead of doing a
> > weak_unref()?
> 
> Because the test in weak_unref is not 'has fWeakCnt become 0?' (as it may
> appear) but is 'have both fRefCnt *and* fWeakCnt become 0?'.
> 
> I am assuming that what you are trying to propose is to initialize fWeakCnt to
0
> and then when the fRefCnt goes to 0 delete the object when fWeakCnt goes to 0.
> The issue with this is when someone does have a weak reference. The last
strong
> ref goes away and fRefCnt goes to 0. The object is now in the disposed state
but
> still exists. How do we then implement weak_unref to do the right thing? Note
> that the fWeakCnt could go to 0 while fRefCnt is still > 0. As stated above,
> weak_unref needs to know that both fRefCnt and fWeakCnt have gone to 0.

Is it not possible to check that?  It might require a two phase check, i.e.
atomic_dec fWeakCnt to 0.  If fRefCnt is 0, check that fWeakCnt is still 0 ->
delete?  Since you prevent a weak reference from turning into a strong reference
if there are no other strong references won't that work?

Context: I think it's worth some time to make both fRefCnt and fWeakCnt reflect
the the naturally expected values because if they don't, at some point someone
is going to go down a hole while debugging only to discover N hours later that
they didn't see that the strong refs hold a weak ref.

That being said, I don't want to hold this up more - LGTM

> Note that as is, SkWeakRefCnt::internal_dispose() could be 'optimized' by
> testing fWeakCnt for being 1 (just the collectively held weak ref) and then
call
> SkRefCnt::internal_dispose() directly (see patch set 10). This would skip one
> atomic decrement. I'm not sure this is worth the added code size and
complexity.
> Such an optimization, should it be discovered useful, could be added later if
> desired.

[bungeman, 2012-05-14 19:40:14]

This patch set (11) contains something which could be checked in.

Running the bench included here shows several timings of interest on my machine.
Compared to the previous implementation, the slowdown in SkRefCnt of the one
extra virtual call is unnoticeable. When used on the stack, SkWeakRefCnt takes
the same time as SkRefCnt, as would be expected. When used in a low cost heap
(in place new, empty delete), SkRefCnt is about 20% slower. When used with a
more realistic heap (regular new and delete) SkRefCnt is 7% slower.

[bungeman, 2012-05-14 19:50:08]

On 2012/05/14 19:40:14, bungeman wrote:
> This patch set (11) contains something which could be checked in.
> 
> Running the bench included here shows several timings of interest on my
machine.
> Compared to the previous implementation, the slowdown in SkRefCnt of the one
> extra virtual call is unnoticeable. When used on the stack, SkWeakRefCnt takes
> the same time as SkRefCnt, as would be expected. When used in a low cost heap
> (in place new, empty delete), SkRefCnt is about 20% slower. When used with a
> more realistic heap (regular new and delete) SkRefCnt is 7% slower.

Oops, typo there. Those last two lines should be

... SkWeakRefCnt is about 20% slower than SkRefCnt. When used with a more
realistic heap (regular new and delete) SkWeakRefCnt is 7% slower than SkRefCnt.

[reed1, 2012-05-14 19:52:48]

lgtm

[reed1, 2012-05-14 20:03:13]

http://codereview.appspot.com/5649046/diff/27018/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/27018/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:141: */
other names?

- attempt_ref()
- strong_ref()
- try_foo_ref()

Also, doc pseudo usage pattern?

if (myweak....lock()) {
   ...
   myweak.unref();
} else {
   // we have been "disposed" (or weak_disposed)
}

[reed1, 2012-05-14 20:03:40]

can we move this to its own header?

[bungeman, 2012-05-14 21:06:54]

Updated documentation and changed weak_lock to try_ref.

http://codereview.appspot.com/5649046/diff/27018/include/core/SkRefCnt.h
File include/core/SkRefCnt.h (right):

http://codereview.appspot.com/5649046/diff/27018/include/core/SkRefCnt.h#newc...
include/core/SkRefCnt.h:141: */
On 2012/05/14 20:03:13, reed1 wrote:
> other names?
> 
> - attempt_ref()
> - strong_ref()
> - try_foo_ref()
> 
> Also, doc pseudo usage pattern?
> 
> if (myweak....lock()) {
>    ...
>    myweak.unref();
> } else {
>    // we have been "disposed" (or weak_disposed)
> }

try_ref seems to look nice when used.

Added some example-ish things to the class doc.

[bungeman, 2012-05-17 18:56:10]

Committed revision r3978.
Android issues committed revision r3979 and r3982.