Calculates retaining paths through user fields and ignores VM objects

Calculates retaining paths through user fields and ignores VM objects
before reporting a path that goes through VM objects.

When a user attempts to check the retaining path of an instance of a
class, previously Observatory would report the first path found, even
if the path contains multiple VM objects.

Now, Observatory will attempt to find a path through the user fields and
ignores all VM objects aside from said fields. If that fails, it
defaults to the original algorithm.

BUG=
R=asiva@google.com, cbernaschina@google.com, rmacnak@google.com

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

[danunez, 2017-07-25 20:18:06]

This is the retaining path work I was talking about last week.

I will admit, I'm not entirely sure how to thoroughly test this one, but I
thought it better to let others look at it then ponder how to.

[cbernaschina, 2017-07-25 21:09:22]

Just a comment regarding the specific nature of the change

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

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:37: if (!include_vm_obj) {
Can we make this more generic?

Maybe storing a function pointer or a functor object that is optionally passed
to ObjectGraph::IterateObjects and will filter out elements?

[rmacnak, 2017-07-25 21:55:21]

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

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:38: Object& o = Object::Handle(*current);
Allocating a handle here is too expensive.

*current->GetClassId()

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:81: bool include_vm_obj;
include_vm_objects_

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:650: void
ObjectGraph::IterateObjectsNoVM(ObjectGraph::Visitor* visitor) {
Place next to ObjectGraph::IterateObjects.

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

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.h#n...
runtime/vm/object_graph.h:66: void IterateObjectsNoVM(Visitor* visitor);
Consider IterateUserObjects

[danunez, 2017-07-25 22:24:54]

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

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:37: if (!include_vm_obj) {
On 2017/07/25 21:09:22, cbernaschina wrote:
> Can we make this more generic?
> 
> Maybe storing a function pointer or a functor object that is optionally passed
> to ObjectGraph::IterateObjects and will filter out elements?

I like this idea and we discussed this a bit offline. We found that
ObjectGraph::IterateObjects will have to take in said function pointer/functor.

I was curious if others had a preferred way of making this change.

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:38: Object& o = Object::Handle(*current);
On 2017/07/25 21:55:21, rmacnak wrote:
> Allocating a handle here is too expensive.
> 
> *current->GetClassId()

RawObject::GetClassId() is private. I can make it public to avoid the handle.

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:81: bool include_vm_obj;
On 2017/07/25 21:55:21, rmacnak wrote:
> include_vm_objects_

Done.

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:650: void
ObjectGraph::IterateObjectsNoVM(ObjectGraph::Visitor* visitor) {
On 2017/07/25 21:55:21, rmacnak wrote:
> Place next to ObjectGraph::IterateObjects.

I can do this, but I will also have to move IterateUserFields() as well, since
it is a static function declared and defined in object_graph.cc.

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

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.h#n...
runtime/vm/object_graph.h:66: void IterateObjectsNoVM(Visitor* visitor);
On 2017/07/25 21:55:21, rmacnak wrote:
> Consider IterateUserObjects

Done.

[rmacnak, 2017-07-26 00:25:40]

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

https://codereview.chromium.org/2990643002/diff/1/runtime/vm/object_graph.cc#...
runtime/vm/object_graph.cc:38: Object& o = Object::Handle(*current);
On 2017/07/25 22:24:54, danunez wrote:
> On 2017/07/25 21:55:21, rmacnak wrote:
> > Allocating a handle here is too expensive.
> > 
> > *current->GetClassId()
> 
> RawObject::GetClassId() is private. I can make it public to avoid the handle.

Make ObjectGraph::Stack a friend instead. I'm sort of surprised it isn't already
given all the other RawObject going on in this function.

[danunez, 2017-07-26 20:54:59]

Addresses most comments. Still thinking about how to make this Stack building
approach more generic.

[rmacnak, 2017-07-26 21:36:09]

Please a test in runtime/observatory/tests/service that checks for a simple
retaining path, e.g. that a const object is retained directly by a field.

[rmacnak, 2017-07-27 20:20:56]

LGTM

[danunez, 2017-07-27 20:24:52]

Description was changed from

==========
Calculates retaining paths through user fields and ignores VM objects
before reporting a path that goes through VM objects.

When a user attempts to check the retaining path of an instance of a
class, previously Observatory would report the first path found, even
if the path contains multiple VM objects.

Now, Observatory will attempt to find a path through the user fields and
ignores all VM objects aside from said fields. If that fails, it
defaults to the original algorithm.

BUG=
R=rmacnak@google.com,asiva@google.com,cbernaschina@google.com
==========

to

==========
Calculates retaining paths through user fields and ignores VM objects
before reporting a path that goes through VM objects.

When a user attempts to check the retaining path of an instance of a
class, previously Observatory would report the first path found, even
if the path contains multiple VM objects.

Now, Observatory will attempt to find a path through the user fields and
ignores all VM objects aside from said fields. If that fails, it
defaults to the original algorithm.

BUG=
R=asiva@google.com, cbernaschina@google.com, rmacnak@google.com

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

[danunez, 2017-07-27 20:24:53]

Committed patchset #3 (id:40001) manually as
ced696af69c6746bac4c3a53b393efcea5206265 (presubmit successful).