Added isolate + thread high watermark tracking to Observatory

Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory usage is kept track of using a high watermark for both the threads and the isolates. Isolate high watermark information is updated when a thread exits the isolate. The isolate high watermark consists of the sum of all thread high watermarks (including the high watermark of the exiting thread). High watermark information for both threads and isolates is now visible in the isolate view in the Observatory.

BUG=
R=johnmccutchan@google.com

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

[bkonyi, 2017-01-03 19:40:01]

Description was changed from

==========
Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory
usage is kept track of using a high watermark for both the threads and the
isolates. Isolate high watermark information is updated when a thread exits the
isolate. The isolate high watermark consists of the sum of all thread high
watermarks (including the high watermark of the exiting thread). High watermark
information for both threads and isolates is now visible in the isolate view in
the Observatory.

BUG=
R=asiva@google.com

Review-Url: https://codereview.chromium.org/2608463002 .
==========

to

==========
Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory
usage is kept track of using a high watermark for both the threads and the
isolates. Isolate high watermark information is updated when a thread exits the
isolate. The isolate high watermark consists of the sum of all thread high
watermarks (including the high watermark of the exiting thread). High watermark
information for both threads and isolates is now visible in the isolate view in
the Observatory.

BUG=

Review-Url: https://codereview.chromium.org/2608463002 .
==========

[bkonyi, 2017-01-03 19:40:01]

bkonyi@google.com changed reviewers:
+ johnmccutchan@google.com, zra@google.com

[bkonyi, 2017-01-03 19:40:20]

Description was changed from

==========
Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory
usage is kept track of using a high watermark for both the threads and the
isolates. Isolate high watermark information is updated when a thread exits the
isolate. The isolate high watermark consists of the sum of all thread high
watermarks (including the high watermark of the exiting thread). High watermark
information for both threads and isolates is now visible in the isolate view in
the Observatory.

BUG=

Review-Url: https://codereview.chromium.org/2608463002 .
==========

to

==========
Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory
usage is kept track of using a high watermark for both the threads and the
isolates. Isolate high watermark information is updated when a thread exits the
isolate. The isolate high watermark consists of the sum of all thread high
watermarks (including the high watermark of the exiting thread). High watermark
information for both threads and isolates is now visible in the isolate view in
the Observatory.

BUG=
==========

[bkonyi, 2017-01-03 19:46:32]

This is an updated version of the commit reviewed here:
https://codereview.chromium.org/2608463002. This should fix failing tests caused
by an uninitialized variable in Isolate and incorrect logic when reseting the
high watermark in a thread after it has been unscheduled.

[Cutch, 2017-01-03 20:58:54]

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/isolate_view.dart (right):

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/isolate_view.dart:285: ..text = 'high
watermark',
This needs more context and probably a title attribute.

What precisely is this a high watermark of?

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2107: jsobj.AddProperty("isolateMemoryHighWatermark",
Please make this private.

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2109: jsobj.AddProperty("threads", thread_registry_);
this too.

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2659: thread->ResetHighWatermark();
why would we be resetting the high watermark when it's scheduled? Doesn't this
lose information?

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2729: intptr_t thread_watermarks_total =
const intptr_t ...

Also, try and use more descriptive names that are consistent with each other.

For example:

UpdateIsolateHighWatermark() and isolate_memory_high_watermark_

The field `isolate_memory_high_watermark_` is already inside the isolate so drop
the isolate_: `memory_high_watermark_`.

Similarly with the function name. It's already defined on the Isolate class so
maybe call it UpdateMemoryHighWatermark()

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:181: intptr_t GetIsolateHighWatermark() const {
style guide for simple getters says this should be:

intptr_t isolate_memory_high_watermark() const {
  return isolate_memory_high_watermark_;
}

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:723: intptr_t isolate_memory_high_watermark_;
uint?

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/thread.cc#newcod...
runtime/vm/thread.cc:220: jsobj.AddProperty("threadMemoryHighWatermark",
thread_memory_high_watermark_);
The value of 'thread_memory_high_watermark_' seems to get reset whenever a
thread enters an isolate. So this can't be the memory high watermark.

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/thread.h#newcode279
runtime/vm/thread.h:279: intptr_t GetThreadHighWatermark() const {
simple getters use lower_case_names()

[bkonyi, 2017-01-03 22:00:38]

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/isolate_view.dart (right):

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/isolate_view.dart:285: ..text = 'high
watermark',
On 2017/01/03 at 20:58:54, Cutch wrote:
> This needs more context and probably a title attribute.
> 
> What precisely is this a high watermark of?

I'll go ahead and change this to "memory high watermark".

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2107: jsobj.AddProperty("isolateMemoryHighWatermark",
On 2017/01/03 at 20:58:54, Cutch wrote:
> Please make this private.

Done.

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2109: jsobj.AddProperty("threads", thread_registry_);
On 2017/01/03 at 20:58:54, Cutch wrote:
> this too.

Done.

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2659: thread->ResetHighWatermark();
On 2017/01/03 at 20:58:54, Cutch wrote:
> why would we be resetting the high watermark when it's scheduled? Doesn't this
lose information?

I was thinking once a thread was no longer being used and is part of the free
thread list, we would want to clear the watermark information before the thread
is reused later for a new task. If that's not correct, should I ever be clearing
the watermarks?

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2729: intptr_t thread_watermarks_total =
On 2017/01/03 at 20:58:54, Cutch wrote:
> const intptr_t ...
> 
> Also, try and use more descriptive names that are consistent with each other.
> 
> For example:
> 
> UpdateIsolateHighWatermark() and isolate_memory_high_watermark_
> 
> The field `isolate_memory_high_watermark_` is already inside the isolate so
drop the isolate_: `memory_high_watermark_`.
> 
> Similarly with the function name. It's already defined on the Isolate class so
maybe call it UpdateMemoryHighWatermark()

Done.

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:181: intptr_t GetIsolateHighWatermark() const {
On 2017/01/03 at 20:58:54, Cutch wrote:
> style guide for simple getters says this should be:
> 
> intptr_t isolate_memory_high_watermark() const {
>   return isolate_memory_high_watermark_;
> }

Done. Would you mind telling me which design doc this is in? I have a feeling I
don't have the correct one.

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:723: intptr_t isolate_memory_high_watermark_;
On 2017/01/03 at 20:58:54, Cutch wrote:
> uint?

Since this is an amount of memory used, should it not be intptr_t?

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/thread.h#newcode279
runtime/vm/thread.h:279: intptr_t GetThreadHighWatermark() const {
On 2017/01/03 at 20:58:54, Cutch wrote:
> simple getters use lower_case_names()

Done.

[Cutch, 2017-01-03 22:28:00]

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/isolate_view.dart (right):

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/isolate_view.dart:285: ..text = 'high
watermark',
On 2017/01/03 22:00:37, bkonyi wrote:
> On 2017/01/03 at 20:58:54, Cutch wrote:
> > This needs more context and probably a title attribute.
> > 
> > What precisely is this a high watermark of?
> 
> I'll go ahead and change this to "memory high watermark".

"memory high watermark" also isn't precise enough. "high watermark of native
memory usage" (or something like that) will help people understand that it isn't
a high watermark of the Dart program's memory usage.

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2659: thread->ResetHighWatermark();
On 2017/01/03 22:00:38, bkonyi wrote:
> On 2017/01/03 at 20:58:54, Cutch wrote:
> > why would we be resetting the high watermark when it's scheduled? Doesn't
this
> lose information?
> 
> I was thinking once a thread was no longer being used and is part of the free
> thread list, we would want to clear the watermark information before the
thread
> is reused later for a new task. If that's not correct, should I ever be
clearing
> the watermarks?

No this was me remembering the way the threads *used* to be structured. I went
and read the code again and I believe your code here is correct.

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:723: intptr_t isolate_memory_high_watermark_;
On 2017/01/03 22:00:38, bkonyi wrote:
> On 2017/01/03 at 20:58:54, Cutch wrote:
> > uint?
> 
> Since this is an amount of memory used, should it not be intptr_t?

1) Memory usage can never be negative.

2) Memory usage can exceed the positive range of an intptr_t. Think of a 32-bit
machine with 3GB of memory allocated. A 32-bit intptr_t can only represent up to
positive ~2 billion.

[bkonyi, 2017-01-04 00:31:55]

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
File runtime/observatory/lib/src/elements/isolate_view.dart (right):

https://codereview.chromium.org/2609253002/diff/1/runtime/observatory/lib/src...
runtime/observatory/lib/src/elements/isolate_view.dart:285: ..text = 'high
watermark',
On 2017/01/03 at 22:28:00, Cutch wrote:
> On 2017/01/03 22:00:37, bkonyi wrote:
> > On 2017/01/03 at 20:58:54, Cutch wrote:
> > > This needs more context and probably a title attribute.
> > > 
> > > What precisely is this a high watermark of?
> > 
> > I'll go ahead and change this to "memory high watermark".
> 
> "memory high watermark" also isn't precise enough. "high watermark of native
memory usage" (or something like that) will help people understand that it isn't
a high watermark of the Dart program's memory usage.

Done.

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/isolate.h#newcod...
runtime/vm/isolate.h:723: intptr_t isolate_memory_high_watermark_;
On 2017/01/03 at 22:28:00, Cutch wrote:
> On 2017/01/03 22:00:38, bkonyi wrote:
> > On 2017/01/03 at 20:58:54, Cutch wrote:
> > > uint?
> > 
> > Since this is an amount of memory used, should it not be intptr_t?
> 
> 1) Memory usage can never be negative.
> 
> 2) Memory usage can exceed the positive range of an intptr_t. Think of a
32-bit machine with 3GB of memory allocated. A 32-bit intptr_t can only
represent up to positive ~2 billion.

Done.

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

https://codereview.chromium.org/2609253002/diff/1/runtime/vm/thread.cc#newcod...
runtime/vm/thread.cc:220: jsobj.AddProperty("threadMemoryHighWatermark",
thread_memory_high_watermark_);
On 2017/01/03 at 20:58:54, Cutch wrote:
> The value of 'thread_memory_high_watermark_' seems to get reset whenever a
thread enters an isolate. So this can't be the memory high watermark.

Is this still a concern given your previous comments in isolate.cc?

[Cutch, 2017-01-04 21:12:38]

LGTM

[bkonyi, 2017-01-04 23:08:12]

Description was changed from

==========
Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory
usage is kept track of using a high watermark for both the threads and the
isolates. Isolate high watermark information is updated when a thread exits the
isolate. The isolate high watermark consists of the sum of all thread high
watermarks (including the high watermark of the exiting thread). High watermark
information for both threads and isolates is now visible in the isolate view in
the Observatory.

BUG=
==========

to

==========
Added isolate + thread high watermark tracking to Observatory

Added tracking of memory usage inside of threads. In addition, the max memory
usage is kept track of using a high watermark for both the threads and the
isolates. Isolate high watermark information is updated when a thread exits the
isolate. The isolate high watermark consists of the sum of all thread high
watermarks (including the high watermark of the exiting thread). High watermark
information for both threads and isolates is now visible in the isolate view in
the Observatory.

BUG=
R=johnmccutchan@google.com

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

[bkonyi, 2017-01-04 23:08:13]

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

[siva, 2017-01-21 00:30:51]

https://codereview.chromium.org/2609253002/diff/40001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.cc (right):

https://codereview.chromium.org/2609253002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:194: return memory_high_watermarks_total;
Why is it necessary to sum up the high water marks of the threads here and add
it up in this fashion. This code is racy and triggers an error from a tsan run.


Is tracking the high water mark for the isolate critical, could we
not just display the high water mark of each thread?