[oilpan] Resurrect a loop-back persistent handle in RefCountedHeapAllocated

[oilpan] Resurrect a loop-back persistent handle in RefCountedHeapAllocated

We have to resurrect a loop-back persistent handle
(RefCountedHeapAllocated::m_keepAlive) when a reference count goes up
from 0 to 1. This can happen in the following scenario:

(1) The reference count becomes 0, but Handles keep references to the object.
(2) The Handle is assigned to a RefPtr. The reference count becomes 1.

R=ager@chromium.org, ager@google.com

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151398

[haraken, 2013-05-29 12:19:23]

r?

[Mads Ager (chromium), 2013-05-29 12:31:06]

The code looks good. Would be cool to have a test in
Source/heap/tests/HeapTest.cpp.

https://codereview.chromium.org/16042013/diff/1/Source/wtf/RefCounted.h
File Source/wtf/RefCounted.h (right):

https://codereview.chromium.org/16042013/diff/1/Source/wtf/RefCounted.h#newco...
Source/wtf/RefCounted.h:124: #if CHECK_REF_COUNTED_LIFECYCLE
When we get into the situation that we have an object with refcount 0 in a
handle that we assign to a refptr these assertions will fire. That seems like a
good thing. We will have to temporarily disable this verification for the
transition period because going to 0 refcount and back up is expected in the
transition period. When we hit this we need to decide whether to temporarily
just flip CHECK_REF_COUNTED_LIFECYCLE to false until we no longer use
RefCountedHeapAllocated.

[haraken, 2013-05-29 13:37:05]

Added a test. r?

https://codereview.chromium.org/16042013/diff/1/Source/wtf/RefCounted.h
File Source/wtf/RefCounted.h (right):

https://codereview.chromium.org/16042013/diff/1/Source/wtf/RefCounted.h#newco...
Source/wtf/RefCounted.h:124: #if CHECK_REF_COUNTED_LIFECYCLE

Disabled the check to make the test case workable. I think we need to anyway
disable the check at some point in order to use RefCountedHeapAllocated.

[Mads Ager (google), 2013-05-29 13:47:54]

LGTM

https://codereview.chromium.org/16042013/diff/5001/Source/heap/tests/HeapTest...
File Source/heap/tests/HeapTest.cpp (right):

https://codereview.chromium.org/16042013/diff/5001/Source/heap/tests/HeapTest...
Source/heap/tests/HeapTest.cpp:397: // Only handle1 and handle2 keep references
to object1 and object2.
Maybe check that PersistentBase::visitRoots does not visit any roots at this
point?

https://codereview.chromium.org/16042013/diff/5001/Source/heap/tests/HeapTest...
Source/heap/tests/HeapTest.cpp:402: handle1 =
Handle<RefCountedAndHeapAllocated>(object1.get());
These lines are just reassigning the same value to the handles. Remove?

Additionally, let's check again that there are two objects visited by
PersistentBase::visitRoots(&visitor)?

[haraken, 2013-05-29 14:52:16]

https://codereview.chromium.org/16042013/diff/5001/Source/heap/tests/HeapTest...
File Source/heap/tests/HeapTest.cpp (right):

https://codereview.chromium.org/16042013/diff/5001/Source/heap/tests/HeapTest...
Source/heap/tests/HeapTest.cpp:402: handle1 =
Handle<RefCountedAndHeapAllocated>(object1.get());
On 2013/05/29 13:47:54, Mads Ager (google) wrote:
> These lines are just reassigning the same value to the handles. Remove?
> 
> Additionally, let's check again that there are two objects visited by
> PersistentBase::visitRoots(&visitor)?

Good catch! I verified that PersistentBase::visitRoots(&visitor) visited 0
object in the patch set 2. I fixed the issue in the patch set 3.

Would you take a look again? (See the diff between the patch set 2 and 3.)

[haraken, 2013-05-29 14:53:45]

https://codereview.chromium.org/16042013/diff/8001/Source/wtf/RefCounted.h
File Source/wtf/RefCounted.h (right):

https://codereview.chromium.org/16042013/diff/8001/Source/wtf/RefCounted.h#ne...
Source/wtf/RefCounted.h:158: --m_refCount;

This is critical :)

[Mads Ager (chromium), 2013-05-29 16:46:38]

LGTM

https://codereview.chromium.org/16042013/diff/8001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/16042013/diff/8001/Source/heap/Heap.h#newcode37
Source/heap/Heap.h:37: #include <stdio.h>
Remove.

https://codereview.chromium.org/16042013/diff/8001/Source/wtf/RefCounted.h
File Source/wtf/RefCounted.h (right):

https://codereview.chromium.org/16042013/diff/8001/Source/wtf/RefCounted.h#ne...
Source/wtf/RefCounted.h:158: --m_refCount;
On 2013/05/29 14:53:45, haraken wrote:
> 
> This is critical :)

Yeah, I see, otherwise the ref will actually never reach zero. :-)

[haraken, 2013-05-29 16:49:02]

Thanks for reviewing!

https://codereview.chromium.org/16042013/diff/8001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/16042013/diff/8001/Source/heap/Heap.h#newcode37
Source/heap/Heap.h:37: #include <stdio.h>
On 2013/05/29 16:46:38, Mads Ager (chromium) wrote:
> Remove.

Done.

[haraken, 2013-05-29 16:49:50]

Committed patchset #4 manually as r151398 (presubmit successful).

[abarth-chromium, 2013-05-29 20:39:37]

https://codereview.chromium.org/16042013/diff/14001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/16042013/diff/14001/Source/heap/Heap.h#newcod...
Source/heap/Heap.h:447: if (UNLIKELY(!RefCounted<T>::refCount())) {
This is likely to be a noticeable perf hit on some classes.  (Obviously not an
issue if we use this class only for a short period of time.)

[haraken, 2013-05-30 04:50:52]

https://codereview.chromium.org/16042013/diff/14001/Source/heap/Heap.h
File Source/heap/Heap.h (right):

https://codereview.chromium.org/16042013/diff/14001/Source/heap/Heap.h#newcod...
Source/heap/Heap.h:447: if (UNLIKELY(!RefCounted<T>::refCount())) {
On 2013/05/29 20:39:38, abarth wrote:
> This is likely to be a noticeable perf hit on some classes.  (Obviously not an
> issue if we use this class only for a short period of time.)

Yes, but I guess we will face a bunch of other performance problems when
migrating RefCounted objects to the handle system. I'll study them when
migrating CSSValue hierarchies to the managed heap.

[Mads Ager (google), 2013-05-30 05:55:00]

On 2013/05/30 04:50:52, haraken wrote:
> https://codereview.chromium.org/16042013/diff/14001/Source/heap/Heap.h
> File Source/heap/Heap.h (right):
> 
>
https://codereview.chromium.org/16042013/diff/14001/Source/heap/Heap.h#newcod...
> Source/heap/Heap.h:447: if (UNLIKELY(!RefCounted<T>::refCount())) {
> On 2013/05/29 20:39:38, abarth wrote:
> > This is likely to be a noticeable perf hit on some classes.  (Obviously not
an
> > issue if we use this class only for a short period of time.)
> 
> Yes, but I guess we will face a bunch of other performance problems when
> migrating RefCounted objects to the handle system. I'll study them when
> migrating CSSValue hierarchies to the managed heap.

The plan is to get the node hierarchy and the css hierarchy moved to the managed
heap and get rid of ref counting for them completely. At that point, none of
those hierarchies will use RefCountedHeapAllocated anymore and we can start
measuring performance. We know that we are adding overhead right now and
RefCountedHeapAllocated is only there as a transition thing. Measuring
performance at this point makes little sense since we are still doing all the
reference counting and have added the handles in addition. We need reference
counting gone before measuring performance makes sense. :)