Oilpan: Remove a visitor parameter from isHeapObjectAlive

Oilpan: Remove a visitor parameter from isHeapObjectAlive

The goal of this CL is to remove a visitor parameter from isHeapObjectAlive
(in preparation for removing a global variable, Heap::s_markingVisitor).
isHeapObjectAlive is just checking whether a given object is marked or not,
and thus it shouldn't need a visitor. So this change would make sense.

Most of the changes in this CL are just moving code around to make the CL compilable.

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

[haraken, 2015-06-01 06:05:19]

haraken@chromium.org changed reviewers:
+ keishi@chromium.org, oilpan-reviews@chromium.org

[haraken, 2015-06-01 06:05:20]

PTAL

The CL is big, but the essential part is just removing a visitor parameter from
isHeapObjectAlive. Other parts are just moving/removing code to make the CL
compilable.

[haraken, 2015-06-01 06:05:54]

haraken@chromium.org changed reviewers:
+ tkent@chromium.org

[haraken, 2015-06-01 06:05:55]

+tkent-san for wtf/ and public/.

[sof, 2015-06-01 06:40:06]

sigbjornf@opera.com changed reviewers:
+ sigbjornf@opera.com

[sof, 2015-06-01 06:40:06]

It seems reasonable to continue to offer visitor->isHeapObjectAlive() - Visitor
doesn't necessarily have to be tied to Heap / the Oilpan GC ?

[haraken, 2015-06-01 06:58:34]

On 2015/06/01 06:40:06, sof wrote:
> It seems reasonable to continue to offer visitor->isHeapObjectAlive() -
Visitor
> doesn't necessarily have to be tied to Heap / the Oilpan GC ?

Given that Visitor doesn't have to be tied to the heap (I agree with this),
what's a motivation of keeping isHeapObjectAlive an method of Visitor?

(a) If the result of isHeapObjectAlive doesn't depend on a visitor, it shouldn't
be a method of Visitor.

(b) If the result of isHeapObjectAlive depends on a visitor, it is wrong to call
isHeapObjectAlive(s_markingVisitor) in Heap::willObjectBeLazilySwept etc. It can
be a thread-local GC visitor or a snapshot GC visitor.

I think that (a) is the case, and that's why I'm trying to factor
isHeapObjectAlive out of Visitor.

[sof, 2015-06-01 07:13:01]

On 2015/06/01 06:58:34, haraken wrote:
> On 2015/06/01 06:40:06, sof wrote:
> > It seems reasonable to continue to offer visitor->isHeapObjectAlive() -
> Visitor
> > doesn't necessarily have to be tied to Heap / the Oilpan GC ?
> 
> Given that Visitor doesn't have to be tied to the heap (I agree with this),
> what's a motivation of keeping isHeapObjectAlive an method of Visitor?
> 
> (a) If the result of isHeapObjectAlive doesn't depend on a visitor, it
shouldn't
> be a method of Visitor.
> 
> (b) If the result of isHeapObjectAlive depends on a visitor, it is wrong to
call
> isHeapObjectAlive(s_markingVisitor) in Heap::willObjectBeLazilySwept etc. It
can
> be a thread-local GC visitor or a snapshot GC visitor.
> 
> I think that (a) is the case, and that's why I'm trying to factor
> isHeapObjectAlive out of Visitor.

(a) - it doesn't have to be named isHeapObjectAlive(), isObjectAlive() might be
a better name for it. The visitor should know what that means, not some third
party (Heap.) cf. the v8 effort to walk the Blink object graph some time ago --
would that now fit?

(b) It is not incorrect to call Heap::isHeapObjectAlive() from another Heap
method. If it is incorrect(?) to use a particular visitor from within its
implementation doesn't imply that Visitor::isHeapObjectAlive() should be removed
at the same time.

[haraken, 2015-06-01 07:53:11]

On 2015/06/01 07:13:01, sof wrote:
> On 2015/06/01 06:58:34, haraken wrote:
> > On 2015/06/01 06:40:06, sof wrote:
> > > It seems reasonable to continue to offer visitor->isHeapObjectAlive() -
> > Visitor
> > > doesn't necessarily have to be tied to Heap / the Oilpan GC ?
> > 
> > Given that Visitor doesn't have to be tied to the heap (I agree with this),
> > what's a motivation of keeping isHeapObjectAlive an method of Visitor?
> > 
> > (a) If the result of isHeapObjectAlive doesn't depend on a visitor, it
> shouldn't
> > be a method of Visitor.
> > 
> > (b) If the result of isHeapObjectAlive depends on a visitor, it is wrong to
> call
> > isHeapObjectAlive(s_markingVisitor) in Heap::willObjectBeLazilySwept etc. It
> can
> > be a thread-local GC visitor or a snapshot GC visitor.
> > 
> > I think that (a) is the case, and that's why I'm trying to factor
> > isHeapObjectAlive out of Visitor.
> 
> (a) - it doesn't have to be named isHeapObjectAlive(), isObjectAlive() might
be
> a better name for it. The visitor should know what that means, not some third
> party (Heap.) cf. the v8 effort to walk the Blink object graph some time ago
--
> would that now fit?
> 
> (b) It is not incorrect to call Heap::isHeapObjectAlive() from another Heap
> method. If it is incorrect(?) to use a particular visitor from within its
> implementation doesn't imply that Visitor::isHeapObjectAlive() should be
removed
> at the same time.

Maybe I should have explained more motivations for making this change :)

We will have three visitors:

- global marking visitor
- thread local visitor
- snapshot visitor

We can create these three Visitor instances when initializing the Heap (or we
can create a proper visitor per GCScope). While we're in a GCScope, it is clear
what visitor we should use. However, while we're not in a GCScope, it is not
clear what visitor we should use. Currently we're unconditionally using
s_markingVisitor while we're not in a GCScope, but it doesn't seem a right thing
to me because:

- Visitor is a notion associated with a marking phase. It seems strange that we
use a Visitor outside a GCScope.

- In practice, there are only two places where we use Visitors outside a
GCScope. The first one is isHeapObjectAliveTrait. The second one is
HashTable::process (where we have a FIXME). Given that isHeapObjectAliveTrait
doesn't need any Visitor to get the result, I'm trying to remove it in this CL.

If you prefer using s_markingVisitor unconditionally outside a GCScope, I can go
that way. (Note: That said, it is correct to unconditionally use
s_markingVisitor for isHeapObjectAliveTrait, but is wrong to use
s_markingVisitor for HashTable::process. The result changes depending on a
visitor.)

[sof, 2015-06-01 11:07:23]

On 2015/06/01 07:53:11, haraken wrote:
> On 2015/06/01 07:13:01, sof wrote:
> > On 2015/06/01 06:58:34, haraken wrote:
> > > On 2015/06/01 06:40:06, sof wrote:
> > > > It seems reasonable to continue to offer visitor->isHeapObjectAlive() -
> > > Visitor
> > > > doesn't necessarily have to be tied to Heap / the Oilpan GC ?
> > > 
> > > Given that Visitor doesn't have to be tied to the heap (I agree with
this),
> > > what's a motivation of keeping isHeapObjectAlive an method of Visitor?
> > > 
> > > (a) If the result of isHeapObjectAlive doesn't depend on a visitor, it
> > shouldn't
> > > be a method of Visitor.
> > > 
> > > (b) If the result of isHeapObjectAlive depends on a visitor, it is wrong
to
> > call
> > > isHeapObjectAlive(s_markingVisitor) in Heap::willObjectBeLazilySwept etc.
It
> > can
> > > be a thread-local GC visitor or a snapshot GC visitor.
> > > 
> > > I think that (a) is the case, and that's why I'm trying to factor
> > > isHeapObjectAlive out of Visitor.
> > 
> > (a) - it doesn't have to be named isHeapObjectAlive(), isObjectAlive() might
> be
> > a better name for it. The visitor should know what that means, not some
third
> > party (Heap.) cf. the v8 effort to walk the Blink object graph some time ago
> --
> > would that now fit?
> > 
> > (b) It is not incorrect to call Heap::isHeapObjectAlive() from another Heap
> > method. If it is incorrect(?) to use a particular visitor from within its
> > implementation doesn't imply that Visitor::isHeapObjectAlive() should be
> removed
> > at the same time.
> 
> Maybe I should have explained more motivations for making this change :)
> 
> We will have three visitors:
> 
> - global marking visitor
> - thread local visitor
> - snapshot visitor
> 
> We can create these three Visitor instances when initializing the Heap (or we
> can create a proper visitor per GCScope). While we're in a GCScope, it is
clear
> what visitor we should use. However, while we're not in a GCScope, it is not
> clear what visitor we should use. Currently we're unconditionally using
> s_markingVisitor while we're not in a GCScope, but it doesn't seem a right
thing
> to me because:
> 
> - Visitor is a notion associated with a marking phase. It seems strange that
we
> use a Visitor outside a GCScope.
> 

Why strange? Visiting the Blink object graph might have various uses, now that
we have the ability.

The removal of this method from Visitor gets us away from that slightly, so I
simply wanted to bring that up for consideration before jumping in.

[haraken, 2015-06-01 11:16:11]

On 2015/06/01 11:07:23, sof wrote:
> On 2015/06/01 07:53:11, haraken wrote:
> > On 2015/06/01 07:13:01, sof wrote:
> > > On 2015/06/01 06:58:34, haraken wrote:
> > > > On 2015/06/01 06:40:06, sof wrote:
> > > > > It seems reasonable to continue to offer visitor->isHeapObjectAlive()
-
> > > > Visitor
> > > > > doesn't necessarily have to be tied to Heap / the Oilpan GC ?
> > > > 
> > > > Given that Visitor doesn't have to be tied to the heap (I agree with
> this),
> > > > what's a motivation of keeping isHeapObjectAlive an method of Visitor?
> > > > 
> > > > (a) If the result of isHeapObjectAlive doesn't depend on a visitor, it
> > > shouldn't
> > > > be a method of Visitor.
> > > > 
> > > > (b) If the result of isHeapObjectAlive depends on a visitor, it is wrong
> to
> > > call
> > > > isHeapObjectAlive(s_markingVisitor) in Heap::willObjectBeLazilySwept
etc.
> It
> > > can
> > > > be a thread-local GC visitor or a snapshot GC visitor.
> > > > 
> > > > I think that (a) is the case, and that's why I'm trying to factor
> > > > isHeapObjectAlive out of Visitor.
> > > 
> > > (a) - it doesn't have to be named isHeapObjectAlive(), isObjectAlive()
might
> > be
> > > a better name for it. The visitor should know what that means, not some
> third
> > > party (Heap.) cf. the v8 effort to walk the Blink object graph some time
ago
> > --
> > > would that now fit?
> > > 
> > > (b) It is not incorrect to call Heap::isHeapObjectAlive() from another
Heap
> > > method. If it is incorrect(?) to use a particular visitor from within its
> > > implementation doesn't imply that Visitor::isHeapObjectAlive() should be
> > removed
> > > at the same time.
> > 
> > Maybe I should have explained more motivations for making this change :)
> > 
> > We will have three visitors:
> > 
> > - global marking visitor
> > - thread local visitor
> > - snapshot visitor
> > 
> > We can create these three Visitor instances when initializing the Heap (or
we
> > can create a proper visitor per GCScope). While we're in a GCScope, it is
> clear
> > what visitor we should use. However, while we're not in a GCScope, it is not
> > clear what visitor we should use. Currently we're unconditionally using
> > s_markingVisitor while we're not in a GCScope, but it doesn't seem a right
> thing
> > to me because:
> > 
> > - Visitor is a notion associated with a marking phase. It seems strange that
> we
> > use a Visitor outside a GCScope.
> > 
> 
> Why strange? Visiting the Blink object graph might have various uses, now that
> we have the ability.

Wouldn't Heap::isHeapObjectAlive(object) (which is a static method) suffice?
(Sorry, I don't fully understand why you want to keep it as an instance method
of Visitor.)

[sof, 2015-06-01 21:46:47]

On 2015/06/01 11:16:11, haraken wrote:
> On 2015/06/01 11:07:23, sof wrote:
....
> > 
> > Why strange? Visiting the Blink object graph might have various uses, now
that
> > we have the ability.
> 
> Wouldn't Heap::isHeapObjectAlive(object) (which is a static method) suffice?
> (Sorry, I don't fully understand why you want to keep it as an instance method
> of Visitor.)

Perhaps we cannot think of such an example now, and having
Heap::isHeapObjectAlive() return a correct result at all times is possible, but
I like the simplified model of trace() being explainable only in terms of what
Visitor operations it has access to.

But please don't let that block the reviewing & progress on this CL.

[haraken, 2015-06-02 01:03:28]

On 2015/06/01 21:46:47, sof wrote:
> On 2015/06/01 11:16:11, haraken wrote:
> > On 2015/06/01 11:07:23, sof wrote:
> ....
> > > 
> > > Why strange? Visiting the Blink object graph might have various uses, now
> that
> > > we have the ability.
> > 
> > Wouldn't Heap::isHeapObjectAlive(object) (which is a static method) suffice?
> > (Sorry, I don't fully understand why you want to keep it as an instance
method
> > of Visitor.)
> 
> Perhaps we cannot think of such an example now, and having
> Heap::isHeapObjectAlive() return a correct result at all times is possible,
but
> I like the simplified model of trace() being explainable only in terms of what
> Visitor operations it has access to.
> 
> But please don't let that block the reviewing & progress on this CL.

Heap::isHeapObjectAlive() is also going to be used to verify that a given
pointer is pointing to a traceable object (to verify that Persistent and
CrossThreadPersistent contain only traceable objects). We can introduce
Heap::markingVisitor() (which returns s_markingVisitor) and let the call site
write as Heap::markingVisitor()->isHeapObjectAlive(), but I guess
Heap::isHeapObjectAlive() is more straightforward.

[haraken, 2015-06-02 08:59:57]

So, this is ready for review. keishi-san?

[haraken, 2015-06-03 03:56:53]

keishi-san: friendly ping. A couple of CLs are blocked by this CL.

[Yuta Kitamura, 2015-06-03 04:58:28]

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

[Yuta Kitamura, 2015-06-03 04:58:29]

LGTM with comments

Your change description is chopped in the middle.

https://codereview.chromium.org/1166623002/diff/1/Source/platform/heap/Garbag...
File Source/platform/heap/GarbageCollected.h (right):

https://codereview.chromium.org/1166623002/diff/1/Source/platform/heap/Garbag...
Source/platform/heap/GarbageCollected.h:156: public:                            
                                \
This indent looks wrong; inconsistent with ll. 191-196.

[haraken, 2015-06-03 05:30:11]

> Your change description is chopped in the middle.
> 
>
https://codereview.chromium.org/1166623002/diff/1/Source/platform/heap/Garbag...
> File Source/platform/heap/GarbageCollected.h (right):
> 
>
https://codereview.chromium.org/1166623002/diff/1/Source/platform/heap/Garbag...
> Source/platform/heap/GarbageCollected.h:156: public:                          
 
>                                 \
> This indent looks wrong; inconsistent with ll. 191-196.

Thanks for review; done.

[haraken, 2015-06-03 05:48:59]

tkent-san: Mind taking a look at wtf/ and public/?

[tkent, 2015-06-03 06:08:44]

lgtm

[haraken, 2015-06-03 06:33:38]

The CQ bit was checked by haraken@chromium.org

[haraken, 2015-06-03 06:33:38]

The patchset sent to the CQ was uploaded after l-g-t-m from yutak@chromium.org
Link to the patchset: https://codereview.chromium.org/1166623002/#ps20001
(title: " ")

[commit-bot: I haz the power, 2015-06-03 06:34:00]

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

[commit-bot: I haz the power, 2015-06-03 07:07:46]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=196376