Avoid to list internal Arrays in _GetRetainingPath api

Compacting _GetRetainingPath result avoiding some internal structures.

R=rmacnak@google.com

Committed: https://github.com/dart-lang/sdk/commit/a58c34254dcc8b04cc2edf69e5746bca8370e8df

[cbernaschina, 2016-08-11 03:59:40]

Description was changed from

==========
Avoid to list internal Arrays in _GetRetainingPath api

The api was listing internal Arrays with non @Instance items.
This doesn't follow the specification of the service protocol
and is redundant because generally these Arrays contain @Field
which are already listed by the api.
==========

to

==========
Avoid to list internal Arrays in _GetRetainingPath api

The api was listing internal Arrays with non @Instance items.
This doesn't follow the specification of the service protocol
and is redundant because generally these Arrays contain @Field
which are already listed by the api.
==========

[cbernaschina, 2016-08-11 03:59:41]

cbernaschina@google.com changed reviewers:
+ johnmccutchan@google.com, rmacnak@google.com, turnidge@google.com

[Cutch, 2016-08-11 13:23:21]

https://codereview.chromium.org/2231313002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/object_common.html (right):

https://codereview.chromium.org/2231313002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/object_common.html:68: <div
class="memberName">[{{ path['elements'].indexOf(element) }}]</div>
why compute the index of each time instead of remembering it?

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

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:331: RawObject* object;
It is generally unsafe to use a RawObject* you should use:

Object& object = Object::Handle();

But it looks like a new handle isn't necessary and you could just use current.

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:337: if (!isLastDartInstance && object->IsArray()) {
please document what this is testing for and why it is necessary

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (left):

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/service.cc#oldco...
runtime/vm/service.cc:2014: jselement.AddProperty("index", i);
Why stop sending this?

[Cutch, 2016-08-11 16:11:33]

Please write a service test which demonstrates the bug.

[rmacnak, 2016-08-11 17:20:25]

This approach makes me nervous. There will be objects where all retaining paths
include one of these arrays. I'm not sure what the visitor will do if this
happens, but I suspect it will claim some object references another object that
it does not reference.

From a Dart developer's perspective, it would be nice to get a retaining path
that doesn't involve VM internal objects, but to find such a path we'd need to
enumerate roots differently and selectively follow pointers (e.g., follow a
closure's context but not its function). And then there might not be such a
simple path and we need to fall back to giving the real path involving internal
objects.

Perhaps the service protocol should not distinguish between Object and Instance.

[cbernaschina, 2016-08-11 22:03:31]

https://codereview.chromium.org/2231313002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/object_common.html (right):

https://codereview.chromium.org/2231313002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/object_common.html:68: <div
class="memberName">[{{ path['elements'].indexOf(element) }}]</div>
On 2016/08/11 13:23:21, Cutch wrote:
> why compute the index of each time instead of remembering it?

It is just a temporary fix before the conversion of the element

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

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:331: RawObject* object;
On 2016/08/11 13:23:21, Cutch wrote:
> It is generally unsafe to use a RawObject* you should use:
> 
> Object& object = Object::Handle();
> 
> But it looks like a new handle isn't necessary and you could just use current.

Done.

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:337: if (!isLastDartInstance && object->IsArray()) {
On 2016/08/11 13:23:21, Cutch wrote:
> please document what this is testing for and why it is necessary

Done.

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/service.cc
File runtime/vm/service.cc (left):

https://codereview.chromium.org/2231313002/diff/1/runtime/vm/service.cc#oldco...
runtime/vm/service.cc:2014: jselement.AddProperty("index", i);
On 2016/08/11 13:23:21, Cutch wrote:
> Why stop sending this?

No other api in the service protocol adds this field

[Cutch, 2016-08-12 14:23:40]

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

https://codereview.chromium.org/2231313002/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:332: wasLastArray_ = false;
C++ style guide says variables are lower_case_like_this:

was_last_array_;

https://codereview.chromium.org/2231313002/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:335: bool ShouldOverwrite(RawObject* obj) {
ShouldReplace?

[cbernaschina, 2016-08-12 17:28:24]

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

https://codereview.chromium.org/2231313002/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:332: wasLastArray_ = false;
On 2016/08/12 14:23:39, Cutch wrote:
> C++ style guide says variables are lower_case_like_this:
> 
> was_last_array_;

Done.

https://codereview.chromium.org/2231313002/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:335: bool ShouldOverwrite(RawObject* obj) {
On 2016/08/12 14:23:40, Cutch wrote:
> ShouldReplace?

Changed approach, and name

[Cutch, 2016-08-12 17:41:11]

Please expand the tests to show that the retaining paths for a List and a Map
work as expected.

[cbernaschina, 2016-08-12 18:00:40]

Description was changed from

==========
Avoid to list internal Arrays in _GetRetainingPath api

The api was listing internal Arrays with non @Instance items.
This doesn't follow the specification of the service protocol
and is redundant because generally these Arrays contain @Field
which are already listed by the api.
==========

to

==========
Compacting _GetRetainingPath result avoiding some internal structures.
==========

[rmacnak, 2016-08-12 20:22:24]

lgtm

https://codereview.chromium.org/2231313002/diff/60001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2231313002/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2018: // TODO(koda): User-friendly interpretation for map
entries.
Remove TODO.

[cbernaschina, 2016-08-12 20:34:17]

https://codereview.chromium.org/2231313002/diff/60001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2231313002/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:2018: // TODO(koda): User-friendly interpretation for map
entries.
On 2016/08/12 20:22:24, rmacnak wrote:
> Remove TODO.

Done.

[cbernaschina, 2016-08-12 20:42:12]

Description was changed from

==========
Compacting _GetRetainingPath result avoiding some internal structures.
==========

to

==========
Compacting _GetRetainingPath result avoiding some internal structures.

R=rmacnak@google.com

Committed:
https://github.com/dart-lang/sdk/commit/a58c34254dcc8b04cc2edf69e5746bca8370e8df
==========

[cbernaschina, 2016-08-12 20:42:13]

Committed patchset #6 (id:100001) manually as
a58c34254dcc8b04cc2edf69e5746bca8370e8df (presubmit successful).