vmservice: basic object graph serialization.

vmservice: basic object graph serialization.

Sends a service event with binary data encoding the basic shape of the object graph; for each object, its:
* size
* class id
* neighbors (excluding smis and nulls)

This is a minimal implementation to make the first unit test pass (TDD).

R=johnmccutchan@google.com

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

[Cutch, 2014-12-02 16:46:26]

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/object_graph.dart (right):

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:13: ByteData _data;
could this be final?

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:38: int _id;
could this be final?

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:40: int classId;
could these be made final? or at least have non-public setters.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:41: List<ObjectVertex>
succ = new List<ObjectVertex>();
could this be final?

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:47: Map<int,
ObjectVertex> _idToVertex = new Map<int, ObjectVertex>();
could this be final?

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:56: obj.classId =
stream.readUnsigned();
if you read the id, size, and classId and then called _asVertex(id, size,
classId) you could make all the fields final in ObjectVertex

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:59:
obj.succ.add(_asVertex(last));
Oh, I see why you can't make _asVertex take all three properties.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/service_io.dart (right):

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/service_io.dart:52: print('nonString:
${data.lengthInBytes}');
Remove this or make it into a log call.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/test/graph_test.dart (right):

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/graph_test.dart:37: List<ObjectVertex>
foos = graph.vertices.where((ObjectVertex obj) => obj.classId == fooId);
assert that fooId != null before?

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/graph_test.dart:50: fooId =
int.parse(fooClass.id.substring(prefix.length));
Not necessarily for your change- it might make sense to have a classId getter on
Class.

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

https://codereview.chromium.org/773573005/diff/20001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:394: // we'll need to encode which fields were
omitted here.
Just curious: Why not send tagged pointers over the wire and include a sentinel
null object?

https://codereview.chromium.org/773573005/diff/20001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:460: OS::PrintErr("%" Pd "\n", visitor.count());
Remove before submitting

[Cutch, 2014-12-02 16:47:48]

lgtm after comments are addressed

[koda, 2014-12-02 18:47:12]

Thanks for the quick response! Submitting soon.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/object_graph.dart (right):

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:13: ByteData _data;
On 2014/12/02 16:46:26, Cutch wrote:
> could this be final?

Done.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:38: int _id;
On 2014/12/02 16:46:26, Cutch wrote:
> could this be final?

Done.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:40: int classId;
On 2014/12/02 16:46:26, Cutch wrote:
> could these be made final? or at least have non-public setters.

Hid the setters.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:41: List<ObjectVertex>
succ = new List<ObjectVertex>();
On 2014/12/02 16:46:26, Cutch wrote:
> could this be final?

Done.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:47: Map<int,
ObjectVertex> _idToVertex = new Map<int, ObjectVertex>();
On 2014/12/02 16:46:26, Cutch wrote:
> could this be final?

Done.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:56: obj.classId =
stream.readUnsigned();
On 2014/12/02 16:46:26, Cutch wrote:
> if you read the id, size, and classId and then called _asVertex(id, size,
> classId) you could make all the fields final in ObjectVertex

No.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/object_graph.dart:59:
obj.succ.add(_asVertex(last));
On 2014/12/02 16:46:26, Cutch wrote:
> Oh, I see why you can't make _asVertex take all three properties.

Right. I added a comment explaining the two-stage initialization.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/lib/service_io.dart (right):

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/lib/service_io.dart:52: print('nonString:
${data.lengthInBytes}');
On 2014/12/02 16:46:26, Cutch wrote:
> Remove this or make it into a log call.

Done.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/test/graph_test.dart (right):

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/graph_test.dart:37: List<ObjectVertex>
foos = graph.vertices.where((ObjectVertex obj) => obj.classId == fooId);
On 2014/12/02 16:46:26, Cutch wrote:
> assert that fooId != null before?

Done.

https://codereview.chromium.org/773573005/diff/20001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/graph_test.dart:50: fooId =
int.parse(fooClass.id.substring(prefix.length));
On 2014/12/02 16:46:26, Cutch wrote:
> Not necessarily for your change- it might make sense to have a classId getter
on
> Class.

Agreed, I've added a TODO.

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

https://codereview.chromium.org/773573005/diff/20001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:394: // we'll need to encode which fields were
omitted here.
On 2014/12/02 16:46:26, Cutch wrote:
> Just curious: Why not send tagged pointers over the wire and include a
sentinel
> null object?

1. For now, we mainly care about the graph shape and retained sizes. In the
future, we will probably have sentinels for all VM-heap objects (possibly
reusing some code from snapshot system).

2. Dividing by kObjectAlignment allows the ids to fit into Smis on the
Observatory side. Added a comment on this.

https://codereview.chromium.org/773573005/diff/20001/runtime/vm/object_graph....
runtime/vm/object_graph.cc:460: OS::PrintErr("%" Pd "\n", visitor.count());
On 2014/12/02 16:46:26, Cutch wrote:
> Remove before submitting

Done.

[koda, 2014-12-02 19:33:15]

Committed patchset #2 (id:20001) manually as r42070 (presubmit successful).