Refactored PathTracer in heap.cc....

src/heap.h

Index: src/heap.h
===================================================================
--- src/heap.h	(revision 6872)
+++ src/heap.h	(working copy)
@@ -30,6 +30,8 @@
 
 #include <math.h>
 
+#include "globals.h"
+#include "list.h"
 #include "spaces.h"
 #include "splay-tree-inl.h"
 #include "v8-counters.h"
@@ -2152,6 +2154,78 @@
 };
 
 
+#if defined(DEBUG) || defined(LIVE_OBJECT_LIST)
+// Helper class for tracing paths to a search target Object from all roots.
+// The TracePathFrom() method can be used to trace paths from a specific
+// object to the search target object.
+class PathTracer: public ObjectVisitor {

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

nit: space needed after PathTracer

[marklam, 2011/02/22 03:11:15]

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.

+ public:
+  enum WhatToFind {
+    FIND_ALL,   // Will find all matches.
+    FIND_FIRST  // Will stop the search after first match.
+  };
+
+  // For the WhatToFind arg, if FIND_FIRST is specified, tracing will stop
+  // after the first match.  If FIND_ALL is specified, then tracing will be
+  // done for all matches.
+  PathTracer(Object* search_target,

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

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.

[marklam, 2011/02/22 03:11:15]

Done.

+             bool search_for_any_global,
+             WhatToFind what_to_find,
+             bool skip_weak_refs)

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

Excuse me for being annoying but I would prefer using VisitMode enum for the
skip_weak_refs instead of bool.

[marklam, 2011/02/22 03:11:15]

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

+      : search_target_(search_target),
+        search_for_any_global_(search_for_any_global),
+        found_target_(false),
+        found_target_in_trace_(false),
+        what_to_find_(what_to_find),
+        skip_weak_refs_(skip_weak_refs),
+        object_stack_(20),
+        no_alloc() {}
+
+  void VisitPointers(Object** start, Object** end) {

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

Does this method need to be inline?

[marklam, 2011/02/22 03:11:15]

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.

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

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.

+    bool done = ((what_to_find_ == FIND_FIRST) && found_target_);
+    // Visit all HeapObject pointers in [start, end)
+    for (Object** p = start; !done && (p < end); p++) {
+      if ((*p)->IsHeapObject()) {
+        TracePathFrom(p);
+        done = ((what_to_find_ == FIND_FIRST) && found_target_);
+      }
+    }
+  }
+
+  void Reset() {

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

Does this method need to be inline?

[marklam, 2011/02/22 03:11:15]

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.

+    found_target_ = false;
+    object_stack_.Clear();
+  }
+  void TracePathFrom(Object** root);
+
+  bool found() const { return found_target_; }
+
+ protected:

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

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:15]

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.

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

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

+  class MarkVisitor;
+  class UnmarkVisitor;
+
+  void MarkRecursively(Object** p, MarkVisitor* mark_visitor);
+  void UnmarkRecursively(Object** p, UnmarkVisitor* unmark_visitor);
+  virtual void ProcessResults();
+
+  // Tags 0, 1, and 3 are used. Use 2 for marking visited HeapObject.
+  static const int kMarkTag = 2;
+
+  Object* search_target_;
+  bool search_for_any_global_;
+  bool found_target_;
+  bool found_target_in_trace_;
+  WhatToFind what_to_find_;
+  bool skip_weak_refs_;
+  List<Object*> object_stack_;
+
+  AssertNoAllocation no_alloc;  // i.e. no gc allowed.
+
+  DISALLOW_IMPLICIT_CONSTRUCTORS(PathTracer);
+};
+#endif  // DEBUG || LIVE_OBJECT_LIST
+
+
 } }  // namespace v8::internal
 
 #endif  // V8_HEAP_H_