Add Metrics for heap high water marks

Add Metrics for heap high water marks

Will be printed at isolate exit when VM launched with: --print-metrics

R=koda@google.com, rmacnak@google.com

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

[Cutch, 2015-10-05 20:10:52]

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

[Cutch, 2015-10-05 20:14:02]

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

[koda, 2015-10-05 20:35:23]

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

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/metrics.h#ne...
runtime/vm/metrics.h:113: void SetValue(int64_t new_value);
SetValue is somewhat misleading, but not sure what to replace it with:
IncludeValue? ObserveValue? AddValue? UpdateValue?

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/metrics.h#ne...
runtime/vm/metrics.h:123: void SetValue(int64_t new_value);
Ditto.

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:719: to_->size_in_words() * kWordSize);
Non-standard indentation?

[Cutch, 2015-10-05 20:54:35]

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

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/metrics.h#ne...
runtime/vm/metrics.h:113: void SetValue(int64_t new_value);
On 2015/10/05 20:35:23, koda wrote:
> SetValue is somewhat misleading, but not sure what to replace it with:
> IncludeValue? ObserveValue? AddValue? UpdateValue?

Agreed. I had 'SampleValue' at one point but decided it was not worth trying to
pick the ideal name because the semantics are pretty obvious.

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/metrics.h#ne...
runtime/vm/metrics.h:123: void SetValue(int64_t new_value);
On 2015/10/05 20:35:23, koda wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/scavenger.cc
File runtime/vm/scavenger.cc (right):

https://codereview.chromium.org/1391473002/diff/30001/runtime/vm/scavenger.cc...
runtime/vm/scavenger.cc:719: to_->size_in_words() * kWordSize);
On 2015/10/05 20:35:23, koda wrote:
> Non-standard indentation?

Done.

[sra1, 2015-10-05 22:00:27]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2015-10-05 22:00:28]

I'm not sure this "Fixes #24483", but I may be misreading the code.

For https://github.com/dart-lang/sdk/issues/24483 we want the max memory used in
data structures of the program.

It would be nice if there was a metric that hid details like old or new heaps.
What we want is a max-used over all heaps (old, new, whatever you invent
tomorrow) that reflects the #bytes of data that the program can reach.  i.e. a
metric that is a property of the program (at some arbitrary GC 'samples') that
is mostly independent of any policy or mechanism of the GC.

[koda, 2015-10-05 23:20:05]

On 2015/10/05 22:00:28, sra1 wrote:
> I'm not sure this "Fixes #24483", but I may be misreading the code.
> 
> For https://github.com/dart-lang/sdk/issues/24483 we want the max memory used
in
> data structures of the program.
> 
> It would be nice if there was a metric that hid details like old or new heaps.
> What we want is a max-used over all heaps (old, new, whatever you invent
> tomorrow) that reflects the #bytes of data that the program can reach.  i.e. a
> metric that is a property of the program (at some arbitrary GC 'samples') that
> is mostly independent of any policy or mechanism of the GC.

That sounds like a max over new_space.used_in_bytes + old_space.used_in_byte,
which should be possible to track even without any changes to the VM itself,
since we already track these.

[Cutch, 2015-10-05 23:42:23]

On 2015/10/05 23:20:05, koda wrote:
> On 2015/10/05 22:00:28, sra1 wrote:
> > I'm not sure this "Fixes #24483", but I may be misreading the code.
> > 
> > For https://github.com/dart-lang/sdk/issues/24483 we want the max memory
used
> in
> > data structures of the program.
> > 
> > It would be nice if there was a metric that hid details like old or new
heaps.
> > What we want is a max-used over all heaps (old, new, whatever you invent
> > tomorrow) that reflects the #bytes of data that the program can reach.  i.e.
a
> > metric that is a property of the program (at some arbitrary GC 'samples')
that
> > is mostly independent of any policy or mechanism of the GC.
> 
> That sounds like a max over new_space.used_in_bytes + old_space.used_in_byte,
> which should be possible to track even without any changes to the VM itself,
> since we already track these.

We can debate which values should have the maximum be tracked in the bug and not
this CL.

[Cutch, 2015-10-05 23:44:57]

Daniel, Ryan: PTAL.

[koda, 2015-10-06 00:11:11]

Other than the overflow, LGTM. (Also make sure the CL description reflects
whether this CL actually fixes the particular issue referenced.)

https://codereview.chromium.org/1391473002/diff/50001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/1391473002/diff/50001/runtime/vm/pages.cc#new...
runtime/vm/pages.cc:542: usage_.capacity_in_words * kWordSize);
This could overflow on 32-bit with a >2 GB heap (capacity_in_words is an
intptr_t). Consider casting before the multiplication or changing the type of
capacity_in_words.

[rmacnak, 2015-10-06 00:19:43]

lgtm

https://codereview.chromium.org/1391473002/diff/50001/runtime/vm/heap.h
File runtime/vm/heap.h (right):

https://codereview.chromium.org/1391473002/diff/50001/runtime/vm/heap.h#newco...
runtime/vm/heap.h:314: Isolate* isolate_;
Undoes 5dc51af924b9d3cb1005a530fe95dc1663b1eb53 :(.

But okay, I should use alignment directives here instead.

[sra1, 2015-10-06 03:34:39]

On 2015/10/05 23:42:23, Cutch wrote:
> On 2015/10/05 23:20:05, koda wrote:
> > On 2015/10/05 22:00:28, sra1 wrote:
> > > I'm not sure this "Fixes #24483", but I may be misreading the code.
> > > 
> > > For https://github.com/dart-lang/sdk/issues/24483 we want the max memory
> used
> > in
> > > data structures of the program.
> > > 
> > > It would be nice if there was a metric that hid details like old or new
> heaps.
> > > What we want is a max-used over all heaps (old, new, whatever you invent
> > > tomorrow) that reflects the #bytes of data that the program can reach. 
i.e.
> a
> > > metric that is a property of the program (at some arbitrary GC 'samples')
> that
> > > is mostly independent of any policy or mechanism of the GC.
> > 
> > That sounds like a max over new_space.used_in_bytes +
old_space.used_in_byte,
> > which should be possible to track even without any changes to the VM itself,
> > since we already track these.
> 
> We can debate which values should have the maximum be tracked in the bug and
not
> this CL.

Sure, but the CL does not provide what we asked for.

[Cutch, 2015-10-06 14:03:28]

Committed patchset #5 (id:70001) manually as
d9297fa72a26a4b6d5575f685ae86cf254f596fb (presubmit successful).

[Cutch, 2015-10-06 14:03:53]

Landed and bug left open.

https://codereview.chromium.org/1391473002/diff/50001/runtime/vm/pages.cc
File runtime/vm/pages.cc (right):

https://codereview.chromium.org/1391473002/diff/50001/runtime/vm/pages.cc#new...
runtime/vm/pages.cc:542: usage_.capacity_in_words * kWordSize);
On 2015/10/06 00:11:10, koda wrote:
> This could overflow on 32-bit with a >2 GB heap (capacity_in_words is an
> intptr_t). Consider casting before the multiplication or changing the type of
> capacity_in_words.

Done.

[Siggi Cherem (dart-lang), 2015-10-06 15:32:25]

On 2015/10/06 03:34:39, sra1 wrote:
> On 2015/10/05 23:42:23, Cutch wrote:
> > On 2015/10/05 23:20:05, koda wrote:
> > > On 2015/10/05 22:00:28, sra1 wrote:
> > > > I'm not sure this "Fixes #24483", but I may be misreading the code.
> > > > 
> > > > For https://github.com/dart-lang/sdk/issues/24483 we want the max memory
> > used
> > > in
> > > > data structures of the program.
> > > > 
> > > > It would be nice if there was a metric that hid details like old or new
> > heaps.
> > > > What we want is a max-used over all heaps (old, new, whatever you invent
> > > > tomorrow) that reflects the #bytes of data that the program can reach. 
> i.e.
> > a
> > > > metric that is a property of the program (at some arbitrary GC
'samples')
> > that
> > > > is mostly independent of any policy or mechanism of the GC.
> > > 
> > > That sounds like a max over new_space.used_in_bytes +
> old_space.used_in_byte,
> > > which should be possible to track even without any changes to the VM
itself,
> > > since we already track these.
> > 
> > We can debate which values should have the maximum be tracked in the bug and
> not
> > this CL.
> 
> Sure, but the CL does not provide what we asked for.

Sorry, this is my bad - I also was under the impression that new_space.capacity
+ old_space.capacity was good enough, which is why I phrased the request that
way in the first place. I had not thought about the more precise measurement
that Stephen is suggesting, but it's true that it would be more precise and less
sensitive to vm changes in the future (e.g. if you were to change how the vm GC
works.)

[Cutch, 2015-10-06 15:36:26]

On 2015/10/06 15:32:25, Siggi Cherem (dart-lang) wrote:
> On 2015/10/06 03:34:39, sra1 wrote:
> > On 2015/10/05 23:42:23, Cutch wrote:
> > > On 2015/10/05 23:20:05, koda wrote:
> > > > On 2015/10/05 22:00:28, sra1 wrote:
> > > > > I'm not sure this "Fixes #24483", but I may be misreading the code.
> > > > > 
> > > > > For https://github.com/dart-lang/sdk/issues/24483 we want the max
memory
> > > used
> > > > in
> > > > > data structures of the program.
> > > > > 
> > > > > It would be nice if there was a metric that hid details like old or
new
> > > heaps.
> > > > > What we want is a max-used over all heaps (old, new, whatever you
invent
> > > > > tomorrow) that reflects the #bytes of data that the program can reach.

> > i.e.
> > > a
> > > > > metric that is a property of the program (at some arbitrary GC
> 'samples')
> > > that
> > > > > is mostly independent of any policy or mechanism of the GC.
> > > > 
> > > > That sounds like a max over new_space.used_in_bytes +
> > old_space.used_in_byte,
> > > > which should be possible to track even without any changes to the VM
> itself,
> > > > since we already track these.
> > > 
> > > We can debate which values should have the maximum be tracked in the bug
and
> > not
> > > this CL.
> > 
> > Sure, but the CL does not provide what we asked for.
> 
> Sorry, this is my bad - I also was under the impression that
new_space.capacity
> + old_space.capacity was good enough, which is why I phrased the request that
> way in the first place. I had not thought about the more precise measurement
> that Stephen is suggesting, but it's true that it would be more precise and
less
> sensitive to vm changes in the future (e.g. if you were to change how the vm
GC
> works.)

It should be easy to add a maximum post-gc used. It would still be
generationally split but it's more conservative than the maximum capacity.