Add VMMetric and some sample metrics

- Add VMMetric and some sample metrics

- Per isolate:

* heap.old.used
* heap.old.capacity
* heap.old.external
* heap.new.used
* heap.new.capacity
* heap.new.external

- VM:

* vm.isolate.count

- New metrics service namespace: /metrics/vm/...

R=koda@google.com

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

[koda, 2014-08-12 22:03:03]

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

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/json_stream.c...
runtime/vm/json_stream.cc:411: if (s == NULL) {
Do we really want to silently accept this? I fear it could hide bugs.

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h
File runtime/vm/metrics.h (right):

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:45: int64_t value() const { return value_; }
It's confusing to have both value() and Value() in the same public interface. It
it guaranteed that the "consumer" will never call value() directly (if so, then
why does it have access to that method)? Should a subclass always call set_value
when Value is called?

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:63: int64_t value_;
int64_t? I thought you had decided on double for all metrics?

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:76: class VMMetricHeapOldUsed : public VMMetric {
Please try to minimize the amount of code one must add for a trivial counter.
Ideally, I would love to be able to add a single "increment" statement within a
loop and automagically create a counter (as with the existing counters.h)

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/vm_sources.gypi
File runtime/vm/vm_sources.gypi (right):

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/vm_sources.gy...
runtime/vm/vm_sources.gypi:272: 'metrics.cc',
Sort.

[koda, 2014-08-12 22:04:15]

It would be great to include an example of a metric that does not need to
subclass VMMetric, to show how much/little boilerplate is needed.

[Cutch, 2014-08-13 17:51:00]

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

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/json_stream.c...
runtime/vm/json_stream.cc:411: if (s == NULL) {
On 2014/08/12 22:03:03, koda wrote:
> Do we really want to silently accept this? I fear it could hide bugs.

If someone calls:

AddProperty("key", value);

where value is:

const char* value = NULL;

This will result in an empty JSON string which I think is the correct behaviour.

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h
File runtime/vm/metrics.h (right):

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:45: int64_t value() const { return value_; }
On 2014/08/12 22:03:03, koda wrote:
> It's confusing to have both value() and Value() in the same public interface.
It
> it guaranteed that the "consumer" will never call value() directly (if so,
then
> why does it have access to that method)? Should a subclass always call
set_value
> when Value is called?

Moved Value to behind 'protected:' and added comments.

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:63: int64_t value_;
On 2014/08/12 22:03:03, koda wrote:
> int64_t? I thought you had decided on double for all metrics?

On the service it is still a double but I think most(all?) VM metrics will be
integral.

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:76: class VMMetricHeapOldUsed : public VMMetric {
On 2014/08/12 22:03:03, koda wrote:
> Please try to minimize the amount of code one must add for a trivial counter.
> Ideally, I would love to be able to add a single "increment" statement within
a
> loop and automagically create a counter (as with the existing counters.h)

A trivial isolate owned counter would require the following:

1) Add a line to ISOLATE_METRIC_LIST:

V(VMMetric, CounterName, "counter.name", kCounter)

2a) To increment the counter:

Isolate::Current()->GetCounterNameMetric()->increment();

2b) To set the counter:

Isolate::Current()->GetCounterNameMetric()->set_value(44);

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/vm_sources.gypi
File runtime/vm/vm_sources.gypi (right):

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/vm_sources.gy...
runtime/vm/vm_sources.gypi:272: 'metrics.cc',
On 2014/08/12 22:03:03, koda wrote:
> Sort.

Done.

[koda, 2014-08-13 22:43:00]

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

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/json_stream.c...
runtime/vm/json_stream.cc:411: if (s == NULL) {
On 2014/08/13 17:50:59, Cutch wrote:
> On 2014/08/12 22:03:03, koda wrote:
> > Do we really want to silently accept this? I fear it could hide bugs.
> 
> If someone calls:
> 
> AddProperty("key", value);
> 
> where value is:
> 
> const char* value = NULL;
> 
> This will result in an empty JSON string which I think is the correct
behaviour.

I meant that NULL is often used to indicate an (unintended) error rather than a
benign "missing value", in which case this behavior either postpones detection
or hides the problem.

But I'll leave it up to your discretion; I understand that it can be useful. But
at least document it in the header and add a unit test.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc
File runtime/vm/metrics.cc (right):

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:30: Unit unit) {
Assert Init is called only once.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:58: case VMMetric::kCounter: return "counter";
I suppose this mapping will need to be sync'ed with a corresponding one in the
Observatory. If so, then the header might be a more suitable place for it, but
I'll leave it up to you.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:74: obj.AddPropertyF("id", "metrics/vm/%s", name_);
There is nothing that distinguishes per-isolate metrics from vm-global metrics.
Consider including an isolate reference on the per-isolate ones.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:81: void VMMetric::RegisterWithIsolate() {
Assert no duplicate registration?

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:121: void VMMetric::RegisterWithVM() {
Ditto.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:154: previous->set_next(head->next());
Just a general observation: we have lots of linked list implementations in the
VM; "someone" should make a utility like GrowableArray...

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:165: Isolate* isolate = Isolate::Current();
Shouldn't these all use "isolate_" rather than the current isolate?

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h
File runtime/vm/metrics.h (right):

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:43: // Initialize and register a counter for the VM.
The terminology is a bit awkward here: the entire class is called VMMetric, but
then there are two cases, one of which is "for the VM".

Consider another term, like "cross-isolate" or "global". Or rename the class.

Also, "counter" -> "metric".

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:64: intptr_t unit() const { return unit_; }
Use Unit type rather than intptr_t.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:66: Isolate* isolate() const { return isolate_; }
Does this need to be public? If yes, at least say it's NULL for a VM-global
metric.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics_test.cc
File runtime/vm/metrics_test.cc (right):

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics_test....
runtime/vm/metrics_test.cc:33: // 99 bytes.
It would be nicer if the unit (and also name and description) could, optionally,
be specified in the subclass declaration instead of the Init call site, since,
as your comment acknowledges, this is the place where you care. I suppose one
could just override Init?

https://codereview.chromium.org/464953002/diff/40001/tests/lib/profiler/metri...
File tests/lib/profiler/metrics_test.dart (right):

https://codereview.chromium.org/464953002/diff/40001/tests/lib/profiler/metri...
tests/lib/profiler/metrics_test.dart:103: var counter = new Counter('vm',
'description');
Could you also add a test to observatory/test that successfully requests a VM
metric?

[Cutch, 2014-08-14 20:56:19]

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

https://codereview.chromium.org/464953002/diff/20001/runtime/vm/json_stream.c...
runtime/vm/json_stream.cc:411: if (s == NULL) {
On 2014/08/13 22:42:59, koda wrote:
> On 2014/08/13 17:50:59, Cutch wrote:
> > On 2014/08/12 22:03:03, koda wrote:
> > > Do we really want to silently accept this? I fear it could hide bugs.
> > 
> > If someone calls:
> > 
> > AddProperty("key", value);
> > 
> > where value is:
> > 
> > const char* value = NULL;
> > 
> > This will result in an empty JSON string which I think is the correct
> behaviour.
> 
> I meant that NULL is often used to indicate an (unintended) error rather than
a
> benign "missing value", in which case this behavior either postpones detection
> or hides the problem.
> 
> But I'll leave it up to your discretion; I understand that it can be useful.
But
> at least document it in the header and add a unit test.

Acknowledged.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc
File runtime/vm/metrics.cc (right):

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:30: Unit unit) {
On 2014/08/13 22:42:59, koda wrote:
> Assert Init is called only once.

Done.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:58: case VMMetric::kCounter: return "counter";
On 2014/08/13 22:42:59, koda wrote:
> I suppose this mapping will need to be sync'ed with a corresponding one in the
> Observatory. If so, then the header might be a more suitable place for it, but
> I'll leave it up to you.

Acknowledged.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:74: obj.AddPropertyF("id", "metrics/vm/%s", name_);
On 2014/08/13 22:42:59, koda wrote:
> There is nothing that distinguishes per-isolate metrics from vm-global
metrics.
> Consider including an isolate reference on the per-isolate ones.

It is distinguished in libservice by the Metric's "owner" field. We usually
don't send references to owner of objects.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:81: void VMMetric::RegisterWithIsolate() {
On 2014/08/13 22:42:59, koda wrote:
> Assert no duplicate registration?

Done.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:121: void VMMetric::RegisterWithVM() {
On 2014/08/13 22:42:59, koda wrote:
> Ditto.

Done.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:154: previous->set_next(head->next());
On 2014/08/13 22:42:59, koda wrote:
> Just a general observation: we have lots of linked list implementations in the
> VM; "someone" should make a utility like GrowableArray...

Acknowledged.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.cc#ne...
runtime/vm/metrics.cc:165: Isolate* isolate = Isolate::Current();
On 2014/08/13 22:42:59, koda wrote:
> Shouldn't these all use "isolate_" rather than the current isolate?

Done and also ASSERT(isolate() == Isolate::Current());

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h
File runtime/vm/metrics.h (right):

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:43: // Initialize and register a counter for the VM.
On 2014/08/13 22:43:00, koda wrote:
> The terminology is a bit awkward here: the entire class is called VMMetric,
but
> then there are two cases, one of which is "for the VM".
> 
> Consider another term, like "cross-isolate" or "global". Or rename the class.
> 
> Also, "counter" -> "metric".

Done. Renamed VMMetric -> Metric.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:64: intptr_t unit() const { return unit_; }
On 2014/08/13 22:42:59, koda wrote:
> Use Unit type rather than intptr_t.

Done.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics.h#new...
runtime/vm/metrics.h:66: Isolate* isolate() const { return isolate_; }
On 2014/08/13 22:43:00, koda wrote:
> Does this need to be public? If yes, at least say it's NULL for a VM-global
> metric.

Done.

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics_test.cc
File runtime/vm/metrics_test.cc (right):

https://codereview.chromium.org/464953002/diff/40001/runtime/vm/metrics_test....
runtime/vm/metrics_test.cc:33: // 99 bytes.
On 2014/08/13 22:43:00, koda wrote:
> It would be nicer if the unit (and also name and description) could,
optionally,
> be specified in the subclass declaration instead of the Init call site, since,
> as your comment acknowledges, this is the place where you care. I suppose one
> could just override Init?

Yes. Although overriding is probably more verbose than putting the data at the
Init call. The vast majority of metrics will be in the macro list in the header
which already encodes this information in one place.

https://codereview.chromium.org/464953002/diff/40001/tests/lib/profiler/metri...
File tests/lib/profiler/metrics_test.dart (right):

https://codereview.chromium.org/464953002/diff/40001/tests/lib/profiler/metri...
tests/lib/profiler/metrics_test.dart:103: var counter = new Counter('vm',
'description');
On 2014/08/13 22:43:00, koda wrote:
> Could you also add a test to observatory/test that successfully requests a VM
> metric?

Done.

[koda, 2014-08-15 17:16:14]

LGTM with a suggestion

https://codereview.chromium.org/464953002/diff/60001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/test/vm_metrics_test.dart (right):

https://codereview.chromium.org/464953002/diff/60001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/vm_metrics_test.dart:22: for (var member
in members) {
Iterable.any would make this a one-liner.

[Cutch, 2014-08-15 17:32:41]

https://codereview.chromium.org/464953002/diff/60001/runtime/bin/vmservice/ob...
File runtime/bin/vmservice/observatory/test/vm_metrics_test.dart (right):

https://codereview.chromium.org/464953002/diff/60001/runtime/bin/vmservice/ob...
runtime/bin/vmservice/observatory/test/vm_metrics_test.dart:22: for (var member
in members) {
On 2014/08/15 17:16:14, koda wrote:
> Iterable.any would make this a one-liner.

Done.

[Cutch, 2014-08-15 17:35:09]

Committed patchset #5 manually as 39305 (presubmit successful).