Refactored PathTracer in heap.cc....

Refactored PathTracer in heap.cc.

This is so that it can be reused by LOL code later.

Committed: http://code.google.com/p/v8/source/detail?r=6886

[marklam, 2011-02-20 00:41:14]

Mikhail,

Here's a first step at refactoring the debug MarkRootObjectVisitor mechanism in
heap.cc.  I've now encapsulated it into a PathTracer class and provided a
virtual ProcessResults() method that can be overridden to do customized work
with the results found from the path tracing.  This will be used by the LOL code
later.

I thought I'd submit this change independently first so that you can see that it
is semantically equivalent to the pre-existing code with only minor changes. 
I've added comments at various places to let you know what and why I made those
changes where relevant.

Please take a look.  Thanks.

Mark

http://codereview.chromium.org/6541044/diff/1/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6541044/diff/1/src/heap.cc#newcode5186
src/heap.cc:5186: void PathTracer::TracePathFrom(Object** root) {
TracePathFrom() used to be MarkRootObjectRecursively().

http://codereview.chromium.org/6541044/diff/1/src/heap.cc#newcode5197
src/heap.cc:5197: ProcessResults();
ProcessResults() is introduced as a virtual method to allow a subclass to
customize it.  Technically, this tracer class can be used to do other things
instead of just printing a path.  LOL will use it to print more verbose
information with some filtering added.  There are other uses for it as well that
I'm still exploring.

http://codereview.chromium.org/6541044/diff/1/src/heap.cc#newcode5201
src/heap.cc:5201: void PathTracer::MarkRecursively(Object** p) {
MarkRecursively() used to be MarkObjectRecursively() in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.cc#newcode5238
src/heap.cc:5238: }
The pre-existing code would scan all entries in the object body.  However, the
Context object contains implicit 2 weak ref pointers that are treated specially
by GC.  For LOL's leak debugging, the weak refs should not be scanned either. 
Hence, I added the option to skip them here.

http://codereview.chromium.org/6541044/diff/1/src/heap.cc#newcode5242
src/heap.cc:5242: MarkRecursively(&map);
I moved the tracing of the map after the tracing of the body because in the
memory leak debugging work of LOL, the user is more likely to be interested in
the reference paths via the body properties than then map.  I don't expect this
to cause any problems for the existing heap debugging tracers.  If there's an
issue, please let me know and I can abstract this further.   Thanks.

http://codereview.chromium.org/6541044/diff/1/src/heap.cc#newcode5249
src/heap.cc:5249: void PathTracer::UnmarkRecursively(Object** p) {
UnmarkRecursively() used to be called UnmarkObjectRecursively() in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2164
src/heap.h:2164: class PathTracer: public ObjectVisitor {
PathTracer used to be called MarkRootVisitor in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2185
src/heap.h:2185: done = ((what_to_find_ == kFindFirst) && found_target_);
I added an optimization to bail here if we've already found what we want.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2190
src/heap.h:2190: void TracePathFrom(Object** root);
TracePathFrom() used to be called MarkRootObjectRecursively() in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2199
src/heap.h:2199: class MarkVisitor: public ObjectVisitor {
MarkVisitor used to be called MarkObjectVisitor in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2204
src/heap.h:2204: for (Object** p = start; !tracer_->found() && (p < end); p++) {
The check for !tracer->found() is an optimization that allows us to bail from
the loop if we've already found the target.  Otherwise, it would call
MarkRecursively() (previously MarkObjectRecursively()) which does returns and
does nothing if the target is found anyway.  So, there is no semantic change
here.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2215
src/heap.h:2215: class UnmarkVisitor: public ObjectVisitor {
UnmarkVisitor used to be called UnmarkObjectVisitor in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2232
src/heap.h:2232: void MarkRecursively(Object** p);
MarkRecursively() used to be called MarkObjectRecursively() in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2233
src/heap.h:2233: void UnmarkRecursively(Object** p);
UnmarkRecursively() used to be called UnmarkObjectRecursively() in heap.cc.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2244
src/heap.h:2244: List<Object*> object_stack_;
search_target_, search_for_any_global_, found_target_, and object_stack_ used to
be global statics.  I've encapsulated them in the tracer object now.

http://codereview.chromium.org/6541044/diff/1/src/heap.h#newcode2247
src/heap.h:2247: UnmarkVisitor unmark_visitor;
There used to be global static instances of these 2 visitors in heap.cc too. 
Now, their lifecycle is bound to the tracer's.

[marklam, 2011-02-20 08:23:06]

I've updated the CL with a bug fix for the kFindFirst mode of operation.  This
breaks the previous found_target_ flag into found_target_ and
found_target_in_trace_ flags.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2173
src/heap.h:2173: found_target_in_trace_(false),
There's a need to distinguish found_target_in_trace_ from the global
found_target_ because VisitPointers() is called for the range of one pointer at
a time.  This exposes a bug where TracePathFrom() needs to reset the found flag
for the current trace, but we also need a global flag to indicate whether the
target was found at all.  This wasn't a problem before because we did not
support the kFindFirst mode previously.

The solution is to separate these 2 found flags where found_target_in_trace_
tracks whether the target was found in the current trace path, and found_target_
tracks whether the target was found at all.

[mnaganov (inactive), 2011-02-21 10:59:46]

http://codereview.chromium.org/6541044/diff/1003/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6541044/diff/1003/src/heap.cc#newcode5187
src/heap.cc:5187: if (search_for_any_global_) {
Using two version of the constructor instead of run-time assert will look more
clear.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2166
src/heap.h:2166: PathTracer(Object* search_target,
When using raw pointers, you should instantiate AssertNoAllocation somewhere to
make sure that no GCs occur while you are operating on them. E.g. you can put it
into members list of this class. With GC helpers this wasn't an issue because
they were intended to be used during GC phases.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2167
src/heap.h:2167: bool search_for_any_global,
I have a concern about argument types. I'd suggest to use enums here (I'm not
sure why are you using 'int' for 'what_to_find', not enum.) For the last
argument, you can reuse VisitMode enum.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2199
src/heap.h:2199: // Values for the "what_to_find" argument of the constructor.
As a bonus of using enums, you will not need comments of such kind.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2204
src/heap.h:2204: class MarkVisitor: public ObjectVisitor {
Does this really need to be put in the header file? You can use scoped pointers
to visitors as class members and implement those helper classes inside the .cpp
file.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2235
src/heap.h:2235: PathTracer();
I think what you need here is DISALLOW_IMPLICIT_CONSTRUCTORS macro.

[marklam, 2011-02-21 17:45:15]

I've addressed all your comments except for the one in heap.cc about using 2
constructors.  Can you please provide some clarification on that (see my
response to the comment)?  Thanks.

BTW, the unsigned int counter changes came from another CL when I svn up on
bleeding edge.  They're not part of this CL.

http://codereview.chromium.org/6541044/diff/1003/src/heap.cc
File src/heap.cc (right):

http://codereview.chromium.org/6541044/diff/1003/src/heap.cc#newcode5187
src/heap.cc:5187: if (search_for_any_global_) {
On 2011/02/21 10:59:46, Mikhail Naganov (Chromium) wrote:
> Using two version of the constructor instead of run-time assert will look more
> clear.

This is code that was originally there from the MarkRootObjectVisitor
implementation.  But, can you please clarify what you're asking for here? 
Currently, the PathTracer constructor does take a search_for_any_global arg as
well as a search_target arg.  are you saying that you would prefer 2
constructors where one only has the search_target arg, and the other takes
neither but implicitly search for global objects?

I'm a bit uncomfortable with the implicit searching for global objects but any
other option will require a runtime assert as well.  Please advise.  Thanks.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2166
src/heap.h:2166: PathTracer(Object* search_target,
On 2011/02/21 10:59:46, Mikhail Naganov (Chromium) wrote:
> When using raw pointers, you should instantiate AssertNoAllocation somewhere
to
> make sure that no GCs occur while you are operating on them. E.g. you can put
it
> into members list of this class. With GC helpers this wasn't an issue because
> they were intended to be used during GC phases.

Done.  Thanks for pointing this out.  Since PathTracer holds on to a naked
pointer, I added an AssertNoAllocation as a member field.  This ensures that no
gc is done for the entire lifetime of the tracer.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2167
src/heap.h:2167: bool search_for_any_global,
On 2011/02/21 10:59:46, Mikhail Naganov (Chromium) wrote:
> I have a concern about argument types. I'd suggest to use enums here (I'm not
> sure why are you using 'int' for 'what_to_find', not enum.) For the last
> argument, you can reuse VisitMode enum.

Done.  I had mistakenly believed that enums are not scoped to the declaring
class in C++.  Thanks for making me revisit this.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2199
src/heap.h:2199: // Values for the "what_to_find" argument of the constructor.
On 2011/02/21 10:59:46, Mikhail Naganov (Chromium) wrote:
> As a bonus of using enums, you will not need comments of such kind.

Done.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2204
src/heap.h:2204: class MarkVisitor: public ObjectVisitor {
On 2011/02/21 10:59:46, Mikhail Naganov (Chromium) wrote:
> Does this really need to be put in the header file? You can use scoped
pointers
> to visitors as class members and implement those helper classes inside the
.cpp
> file.

Done.  I moved this into heap.cc, and instantiate them in TracePathFrom() and
passed them around instead of having them as member fields.  They serve as
utility classes anyway whose life cycle is tied with the trace path operation. 
So, this is cleaner.

http://codereview.chromium.org/6541044/diff/1003/src/heap.h#newcode2235
src/heap.h:2235: PathTracer();
On 2011/02/21 10:59:46, Mikhail Naganov (Chromium) wrote:
> I think what you need here is DISALLOW_IMPLICIT_CONSTRUCTORS macro.

Done.

[mnaganov (inactive), 2011-02-21 18:08:28]

Looks almost good, some more comments.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2161
src/heap.h:2161: class PathTracer: public ObjectVisitor {
nit: space needed after PathTracer

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2171
src/heap.h:2171: PathTracer(Object* search_target,
I have an idea about constructor args. Create a constant named, say,
ANY_GLOBAL_OBJECT, and assign it a NULL value. This way, you'll be able to use
only one argument -- 'search_target', and drop 'search_for_any_global'. My
concern is that searching for a specific target object and searching for any
global object are mutually exclusive, so you actually don't need two separate
args for specifying that. I agree that you'll still need to assert that non-NULL
search_target is a heap object.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2174
src/heap.h:2174: bool skip_weak_refs)
Excuse me for being annoying but I would prefer using VisitMode enum for the
skip_weak_refs instead of bool.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2184
src/heap.h:2184: void VisitPointers(Object** start, Object** end) {
Does this method need to be inline?

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2195
src/heap.h:2195: void Reset() {
Does this method need to be inline?

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2203
src/heap.h:2203: protected:
I'd prefer to use 'private:' in all cases, except when you really design your
class to be subclassed.

[marklam, 2011-02-22 03:11:14]

3rd time's the charm ... hopefully. =)

http://codereview.chromium.org/6541044/diff/4003/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2161
src/heap.h:2161: class PathTracer: public ObjectVisitor {
On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> nit: space needed after PathTracer

Done.  I've seen this done both ways throughout the code.  Didn't realized that
there's a preference.  I'll have the spaces from now on.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2171
src/heap.h:2171: PathTracer(Object* search_target,
On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> I have an idea about constructor args. Create a constant named, say,
> ANY_GLOBAL_OBJECT, and assign it a NULL value. This way, you'll be able to use
> only one argument -- 'search_target', and drop 'search_for_any_global'. My
> concern is that searching for a specific target object and searching for any
> global object are mutually exclusive, so you actually don't need two separate
> args for specifying that. I agree that you'll still need to assert that
non-NULL
> search_target is a heap object.

Done.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2174
src/heap.h:2174: bool skip_weak_refs)
On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> Excuse me for being annoying but I would prefer using VisitMode enum for the
> skip_weak_refs instead of bool.

Done.  Sorry for missing this one the first time around.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2184
src/heap.h:2184: void VisitPointers(Object** start, Object** end) {
On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> Does this method need to be inline?

Done.  This is a virtual method override (and I think the compiler will make
sure to only instantiate one instance of it).  The pattern of specifying this
inline is used in lots of places from what I can tell.  I didn't know if that
was the preferred expression.  Anyway, I've moved it to the cc file now.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2195
src/heap.h:2195: void Reset() {
On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> Does this method need to be inline?

Done.  It doesn't have to be, but I figured it was small and could be inlined. 
Anyway, I moved it to the cc file.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2203
src/heap.h:2203: protected:
On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> I'd prefer to use 'private:' in all cases, except when you really design your
> class to be subclassed.

I do intend for this class to be subclassed.  That will come in the LOL code
(which is what prompted this refactoring in the first place).  This is also why
ProcessResults() is virtual.

Or do you mean that you want some members to be limited to being private instead
of protected?  Currently, I don't think the LOL implementation needs access to
all of these members, but it seems extremely limiting to assume what the
subclass needs and not need in the future.  Unless you object to it being
somewhat general like this for forward looking purposes, I would prefer to keep
all these members protected instead of private.

[Søren Thygesen Gjesse, 2011-02-22 08:13:24]

LGTM

http://codereview.chromium.org/6541044/diff/1004/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/1004/src/heap.h#newcode2163
src/heap.h:2163: enum WhatToFind {
Going forward we should try to use names if the for kXxxYyy, however as we are
not at all consistent this is OK for now.

[Søren Thygesen Gjesse, 2011-02-22 10:06:30]

Committed: http://code.google.com/p/v8/source/detail?r=6886.

Thank you for the patch.

[mnaganov (inactive), 2011-02-22 10:12:01]

http://codereview.chromium.org/6541044/diff/4003/src/heap.h
File src/heap.h (right):

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2184
src/heap.h:2184: void VisitPointers(Object** start, Object** end) {
On 2011/02/22 03:11:15, marklam wrote:
> On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> > Does this method need to be inline?
> 
> Done.  This is a virtual method override (and I think the compiler will make
> sure to only instantiate one instance of it).  The pattern of specifying this
> inline is used in lots of places from what I can tell.  I didn't know if that
> was the preferred expression.  Anyway, I've moved it to the cc file now.

I'm just against having too much code in .h file. It is preferred to leave only
getters and setters there. If some non-trivial code really needs to be inline,
we usually have -inl.h file for it.

http://codereview.chromium.org/6541044/diff/4003/src/heap.h#newcode2203
src/heap.h:2203: protected:
On 2011/02/22 03:11:15, marklam wrote:
> On 2011/02/21 18:08:28, Mikhail Naganov (Chromium) wrote:
> > I'd prefer to use 'private:' in all cases, except when you really design
your
> > class to be subclassed.
> 
> I do intend for this class to be subclassed.  That will come in the LOL code
> (which is what prompted this refactoring in the first place).  This is also
why
> ProcessResults() is virtual.
> 

OK, I just wanted to be sure. Sorry, I forgot the whole picture.

> Or do you mean that you want some members to be limited to being private
instead
> of protected?  Currently, I don't think the LOL implementation needs access to
> all of these members, but it seems extremely limiting to assume what the
> subclass needs and not need in the future.  Unless you object to it being
> somewhat general like this for forward looking purposes, I would prefer to
keep
> all these members protected instead of private.

That's OK.