Include parent field/list index in retaining path.

Include parent field/list index in retaining path.

R=johnmccutchan@google.com

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

[Cutch, 2014-06-30 15:56:34]

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

https://codereview.chromium.org/350403005/diff/40001/runtime/vm/object_graph_...
runtime/vm/object_graph_test.cc:113: HANDLESCOPE(isolate);
Can we add a test with a custom class:

class Foo {
  var field1;
  var field2;
}

and verify the offset from parent is correct for both fields?

[koda, 2014-06-30 16:20:04]

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

https://codereview.chromium.org/350403005/diff/40001/runtime/vm/object_graph_...
runtime/vm/object_graph_test.cc:113: HANDLESCOPE(isolate);
On 2014/06/30 15:56:34, Cutch wrote:
> Can we add a test with a custom class:
> 
> class Foo {
>   var field1;
>   var field2;
> }
> 
> and verify the offset from parent is correct for both fields?

I intend to add such a test in service_test when interpreting the offset as a
field in the JSON response.

I could extend this CL to also translate the offset to a field id. What do you
think?

[koda, 2014-06-30 20:42:26]

On 2014/06/30 16:20:04, koda wrote:
>
https://codereview.chromium.org/350403005/diff/40001/runtime/vm/object_graph_...
> File runtime/vm/object_graph_test.cc (right):
> 
>
https://codereview.chromium.org/350403005/diff/40001/runtime/vm/object_graph_...
> runtime/vm/object_graph_test.cc:113: HANDLESCOPE(isolate);
> On 2014/06/30 15:56:34, Cutch wrote:
> > Can we add a test with a custom class:
> > 
> > class Foo {
> >   var field1;
> >   var field2;
> > }
> > 
> > and verify the offset from parent is correct for both fields?
> 
> I intend to add such a test in service_test when interpreting the offset as a
> field in the JSON response.
> 
> I could extend this CL to also translate the offset to a field id. What do you
> think?

Extended CL to add parentField and parentListIndex properties, including unit
test. PTAL

[koda, 2014-07-01 18:06:42]

On 2014/06/30 20:42:26, koda wrote:
> On 2014/06/30 16:20:04, koda wrote:
> >
>
https://codereview.chromium.org/350403005/diff/40001/runtime/vm/object_graph_...
> > File runtime/vm/object_graph_test.cc (right):
> > 
> >
>
https://codereview.chromium.org/350403005/diff/40001/runtime/vm/object_graph_...
> > runtime/vm/object_graph_test.cc:113: HANDLESCOPE(isolate);
> > On 2014/06/30 15:56:34, Cutch wrote:
> > > Can we add a test with a custom class:
> > > 
> > > class Foo {
> > >   var field1;
> > >   var field2;
> > > }
> > > 
> > > and verify the offset from parent is correct for both fields?
> > 
> > I intend to add such a test in service_test when interpreting the offset as
a
> > field in the JSON response.
> > 
> > I could extend this CL to also translate the offset to a field id. What do
you
> > think?
> 
> Extended CL to add parentField and parentListIndex properties, including unit
> test. PTAL

Per our offline discussion, I will further extend the CL to surface these in the
Observatory UI. Hold off review for now.

[koda, 2014-07-01 23:40:02]

FYI, during manual testing I uncovered a serious flaw in the current version of
this CL.

The straight-forward change in Stack to store RawObject** instead of RawObject*
is not safe for the roots, because there is code like this (from
StackFrame::VisitObjectPointers):
  ...
  // Visit the code object.
  RawObject* raw_code = code.raw();
  visitor->VisitPointer(&raw_code);
  ...

Thus, we must either have special handling of the roots, or always store both
the RawObject** and the RawObject*.

[koda, 2014-07-02 16:45:51]

Updated with Observatory UI and bug fix.

Here is a script for manual testing:

path.dart:
class Foo {
  var a;
  var b;
  Foo(this.a, this.b);
}

main() {
  var list = new List<Foo>(10);
  list[5] = new Foo(42.toString(), new Foo(87.toString(), 17.toString()));
  while (true) {}
}

Run: dart --observe ~/path.dart

In Observatory:
'root'
'path.dart'
'class Foo'
'instances'
find 'strongly reachable' list; it should contain 2 elements
one of which should have a field containing "87"
go to the instance "87"
find its retaining path; it should have length 4, described as follows:

 "87" in var a
 Foo in var b
 Foo at list index 5 of
 _List(10)

[Cutch, 2014-07-02 18:23:51]

lgtm

https://codereview.chromium.org/350403005/diff/100001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/350403005/diff/100001/runtime/vm/service_test...
runtime/vm/service_test.cc:616: "var idElem;\n"  // Ditto.
maybe align these comments

[koda, 2014-07-02 18:38:17]

Per offline discussion, added manual testing instructions in client/tests/ui.

https://codereview.chromium.org/350403005/diff/100001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/350403005/diff/100001/runtime/vm/service_test...
runtime/vm/service_test.cc:616: "var idElem;\n"  // Ditto.
On 2014/07/02 18:23:50, Cutch wrote:
> maybe align these comments

Done.

[koda, 2014-07-02 20:38:14]

Committed patchset #9 manually as r37956 (presubmit successful).