ObjectGraph::RetainingPath

ObjectGraph::RetainingPath

Adds a utility method to find a retaining path and unit test.
Next step is to expose it via VM service and Observatory.

R=johnmccutchan@google.com

Committed: https://code.google.com/p/dart/source/detail?r=36381

[koda, 2014-05-20 17:20:24]

Ping.

This has become a highly requested feature, and a primitive version has already
helped track down a memory leak.

[Cutch, 2014-05-20 17:39:12]

sorry for the delay. Just getting situated in Aarhus. 

lgtm with some suggestions to the API and some extra tests.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph.cc
File runtime/vm/object_graph.cc (right):

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:224: if (length_ < path_.Length()) {
Could we allow path to be Array::null() and call like this:

// Query the length.
intptr_t l = RetainingPath(obj, Array::null());

// Allocate array length.
const Array& arr = Array::Handle(Array::New(l));

// Call again to get actual path.
RetainingPath(obj, arr);

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph_...
File runtime/vm/object_graph_test.cc (right):

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph_...
runtime/vm/object_graph_test.cc:111: }
What about a test that doesn't have a path?

Also, a test where the Array passed in is too short to hold the path but the
return value from RetainingPath is the true length. That is, pass in an array of
length 0 and then determine the length necessary, and call it a second time to
get the actual path.

[turnidge, 2014-05-20 18:00:38]

DBC

I saw you were waiting on a review, so I started one.  Looks like john has
picked it up in the meantime.  Here are my comments.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph.cc
File runtime/vm/object_graph.cc (right):

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:215: ASSERT(Isolate::Current()->no_gc_scope_depth()
!= 0);
We could consider clearing path_ here.

Are we unable to use a GrowableArray because it might trigger gc during growth?

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:217: intptr_t length() const { return length_; }
blank line here would help me.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:239: NoGCScope no_gc_scope_;
Maybe comment on what you are doing here with obj and raw.  I know you comment
in the header file, but perhaps commenting here would be more clear. 
"Temporarily clear the object handle so that blah" or whatever.

[koda, 2014-05-20 20:37:49]

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph.cc
File runtime/vm/object_graph.cc (right):

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:215: ASSERT(Isolate::Current()->no_gc_scope_depth()
!= 0);
On 2014/05/20 18:00:38, turnidge wrote:
> We could consider clearing path_ here.
> 
> Are we unable to use a GrowableArray because it might trigger gc during
growth?

Yes, we need to avoid GC during the search.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:217: intptr_t length() const { return length_; }
On 2014/05/20 18:00:38, turnidge wrote:
> blank line here would help me.

Done.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:224: if (length_ < path_.Length()) {
On 2014/05/20 17:39:12, Cutch wrote:
> Could we allow path to be Array::null() and call like this:
> 
> // Query the length.
> intptr_t l = RetainingPath(obj, Array::null());
> 
> // Allocate array length.
> const Array& arr = Array::Handle(Array::New(l));
> 
> // Call again to get actual path.
> RetainingPath(obj, arr);

Done. Although I expect the common use will be to pass in an array with a
reasonable upper bound, rather than making two calls.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:239: NoGCScope no_gc_scope_;
On 2014/05/20 18:00:38, turnidge wrote:
> Maybe comment on what you are doing here with obj and raw.  I know you comment
> in the header file, but perhaps commenting here would be more clear. 
> "Temporarily clear the object handle so that blah" or whatever.

Done.

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph_...
File runtime/vm/object_graph_test.cc (right):

https://codereview.chromium.org/286203015/diff/30001/runtime/vm/object_graph_...
runtime/vm/object_graph_test.cc:111: }
On 2014/05/20 17:39:12, Cutch wrote:
> What about a test that doesn't have a path?
> 
> Also, a test where the Array passed in is too short to hold the path but the
> return value from RetainingPath is the true length. That is, pass in an array
of
> length 0 and then determine the length necessary, and call it a second time to
> get the actual path.

Done.

[koda, 2014-05-20 20:38:07]

Committed patchset #5 manually as r36381 (presubmit successful).