Add a version of heap snapshots that use only fields and stack frames as roots and only include ins…

Add a version of heap snapshots that use only fields and stack frames as roots and only include instances.

R=johnmccutchan@google.com

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

[rmacnak, 2016-11-17 20:20:21]

rmacnak@google.com changed reviewers:
+ johnmccutchan@google.com

[Cutch, 2016-11-18 20:40:32]

a bunch of comments.

How can we unit test the algorithm code? Can we build a test helper that will
generate canned graphs that we can validate the results of?

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/object_graph.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:223: const kStackCid = 1;
can we get this from the VM over the protocol somehow?

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:670: print("Warning: graph has
unconnected nodes");
remove or use the logger?

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:673: for (var i = 1; i <= dfsNumber;
i++) {
assert(() {
....
});

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:685: for (var i = ROOT + 1; i <= N;
i++) {
how about we add a:

const FIRST_OBJECT = ROOT + 1;

and use that everywhere?

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:922: for (var i = 1; i <= Nconnected;
i++) {
FIRST_OBJECT

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/repositories/heap_snapshot.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/repositories/heap_snapshot.dart:34: {String
roots: 'VM',
HeapSnapshotRoots roots: HeapSnapshotRoots.VM

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/repositories/heap_snapshot.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/heap_snapshot.dart:19: final String
roots;
HeapSnapshotRoots

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/heap_snapshot.dart:104: {String roots:
"VM",
HeapSnapshotRoots

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/repositories/top_retaining_instances.dart
(right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/repositories/top_retaining_instances.dart:14: final
raw = await isolate.fetchHeapSnapshot("VM", true).last;
HeapSnapshotRoots.VM

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:1524: Stream
fetchHeapSnapshot(String roots, bool collectGarbage) {
HeapSnapshotRoots roots

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/dominator_tree_test.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/dominator_tree_test.dart:61: final raw = await
isolate.fetchHeapSnapshot("VM", false).last;
HeapSnapshotRoots.VM

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/graph_test.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/graph_test.dart:46: RawHeapSnapshot raw =
await isolate.fetchHeapSnapshot("VM", false).last;
ditto

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

https://codereview.chromium.org/2502283003/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:616: const intptr_t kRootCid = 0;
maybe move this and kStackCid into the real class list to avoid accidental
conflicts

[rmacnak, 2016-11-18 22:34:02]

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/object_graph.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:223: const kStackCid = 1;
On 2016/11/18 20:40:31, Cutch wrote:
> can we get this from the VM over the protocol somehow?

pushing in the header alongside the object alignment

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:670: print("Warning: graph has
unconnected nodes");
On 2016/11/18 20:40:31, Cutch wrote:
> remove or use the logger?

Changed to use logger.

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:673: for (var i = 1; i <= dfsNumber;
i++) {
On 2016/11/18 20:40:31, Cutch wrote:
> assert(() {
> ....
> });

Done.

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:685: for (var i = ROOT + 1; i <= N;
i++) {
On 2016/11/18 20:40:31, Cutch wrote:
> how about we add a:
> 
> const FIRST_OBJECT = ROOT + 1;
> 
> and use that everywhere?

ROOT is the first object.

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/object_graph.dart:922: for (var i = 1; i <= Nconnected;
i++) {
On 2016/11/18 20:40:31, Cutch wrote:
> FIRST_OBJECT

1 here is a DFS number, not a node index.

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
File runtime/observatory/lib/src/models/repositories/heap_snapshot.dart (right):

https://codereview.chromium.org/2502283003/diff/20001/runtime/observatory/lib...
runtime/observatory/lib/src/models/repositories/heap_snapshot.dart:34: {String
roots: 'VM',
On 2016/11/18 20:40:31, Cutch wrote:
> HeapSnapshotRoots roots: HeapSnapshotRoots.VM

Done. Here and elsewhere.

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

https://codereview.chromium.org/2502283003/diff/20001/runtime/vm/object_graph...
runtime/vm/object_graph.cc:616: const intptr_t kRootCid = 0;
On 2016/11/18 20:40:31, Cutch wrote:
> maybe move this and kStackCid into the real class list to avoid accidental
> conflicts

Done.

[Cutch, 2016-11-28 18:11:12]

LGTM after fixing VM rooted tests to be different than the User rooted tests.

https://codereview.chromium.org/2502283003/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2502283003/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:1524: static String
_rootsToString(M.HeapSnapshotRoots roots) {
I think this works too:

roots.toString().split('.')[1];

https://codereview.chromium.org/2502283003/diff/60001/runtime/observatory/tes...
File runtime/observatory/tests/service/object_graph_vm_test.dart (right):

https://codereview.chromium.org/2502283003/diff/60001/runtime/observatory/tes...
runtime/observatory/tests/service/object_graph_vm_test.dart:48: await
isolate.fetchHeapSnapshot(M.HeapSnapshotRoots.vm, false).last;
This and the other VM rooted test should be altered to demonstrate a difference
with the User rooted test.

https://codereview.chromium.org/2502283003/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2502283003/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:153: // A sentinel used by the vm service's heap
snapshots.
used by ... to represent a stack frame.

[rmacnak, 2016-11-28 22:57:56]

Added test that checks for an object dominated by the stack.

https://codereview.chromium.org/2502283003/diff/60001/runtime/observatory/lib...
File runtime/observatory/lib/src/service/object.dart (right):

https://codereview.chromium.org/2502283003/diff/60001/runtime/observatory/lib...
runtime/observatory/lib/src/service/object.dart:1524: static String
_rootsToString(M.HeapSnapshotRoots roots) {
On 2016/11/28 18:11:12, Cutch wrote:
> I think this works too:
> 
> roots.toString().split('.')[1];

The names don't match. The service enum naming convention is different from the
Dart one.

https://codereview.chromium.org/2502283003/diff/60001/runtime/vm/raw_object.h
File runtime/vm/raw_object.h (right):

https://codereview.chromium.org/2502283003/diff/60001/runtime/vm/raw_object.h...
runtime/vm/raw_object.h:153: // A sentinel used by the vm service's heap
snapshots.
On 2016/11/28 18:11:12, Cutch wrote:
> used by ... to represent a stack frame.

Done.

[Cutch, 2016-11-28 23:11:17]

lgtm

[rmacnak, 2016-11-29 00:00:11]

Description was changed from

==========
Add a version of heap snapshots that use only fields and stack frames as roots
and only include instances.
==========

to

==========
Add a version of heap snapshots that use only fields and stack frames as roots
and only include instances.

R=johnmccutchan@google.com

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

[rmacnak, 2016-11-29 00:00:12]

Committed patchset #5 (id:80001) manually as
0cf02040f870a256cc0fc4eb90a3c46a82008fec (presubmit successful).