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=asiva@google.com

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

[bkonyi, 2016-12-27 20:30:26]

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

[siva, 2016-12-27 22:09:26]

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2107: isolate_memory_high_watermark_);
Do we have a comment section somewhere were we document that these strings like
"isolateMemoryHighWatermark" etc. should match up with the strings used in the
corresponding Dart code?

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/zone.cc#newcode49
runtime/vm/zone.cc:49: }
Please hoist Thread:Current() outside the loop into a local variable
instead of accessing thread local storage at every iteration of the loop.

Thread* thread = Thread::Current();
while (...)
  if (thread != NULL) {
    thread->Decrement....(...);
  }
  ....
}

When is Thread::Current() null? (is it just unit tests?)

[bkonyi, 2016-12-28 00:09:58]

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2107: isolate_memory_high_watermark_);
On 2016/12/27 22:09:26, siva wrote:
> Do we have a comment section somewhere were we document that these strings
like
> "isolateMemoryHighWatermark" etc. should match up with the strings used in the
> corresponding Dart code?

There is the Dart VM service protocol document under runtime/vm/service/ which
specifies the types of objects and properties found in the JSON. However, I
haven't made any changes to that for the previous changes that surfaced zone
information, so I didn't think to do it for this change. Should I add the high
watermark information at least?

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/zone.cc#newcode49
runtime/vm/zone.cc:49: }
On 2016/12/27 22:09:26, siva wrote:
> Please hoist Thread:Current() outside the loop into a local variable
> instead of accessing thread local storage at every iteration of the loop.
> 
> Thread* thread = Thread::Current();
> while (...)
>   if (thread != NULL) {
>     thread->Decrement....(...);
>   }
>   ....
> }
> 
> When is Thread::Current() null? (is it just unit tests?)

Done.

There's a case when running the unit tests where it seems a zone is allocated
and not associated with a thread, so we get a segfault when we delete segments
from that zone.

[siva, 2016-12-28 17:52:35]

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2107: isolate_memory_high_watermark_);
On 2016/12/28 00:09:58, bkonyi wrote:
> On 2016/12/27 22:09:26, siva wrote:
> > Do we have a comment section somewhere were we document that these strings
> like
> > "isolateMemoryHighWatermark" etc. should match up with the strings used in
the
> > corresponding Dart code?
> 
> There is the Dart VM service protocol document under runtime/vm/service/ which
> specifies the types of objects and properties found in the JSON. However, I
> haven't made any changes to that for the previous changes that surfaced zone
> information, so I didn't think to do it for this change. Should I add the high
> watermark information at least?

Please check with John/Todd on the protocol for updating this document.

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/zone.cc#newcode49
runtime/vm/zone.cc:49: }
On 2016/12/28 00:09:58, bkonyi wrote:
> On 2016/12/27 22:09:26, siva wrote:
> > Please hoist Thread:Current() outside the loop into a local variable
> > instead of accessing thread local storage at every iteration of the loop.
> > 
> > Thread* thread = Thread::Current();
> > while (...)
> >   if (thread != NULL) {
> >     thread->Decrement....(...);
> >   }
> >   ....
> > }
> > 
> > When is Thread::Current() null? (is it just unit tests?)
> 
> Done.
> 
> There's a case when running the unit tests where it seems a zone is allocated
> and not associated with a thread, so we get a segfault when we delete segments
> from that zone.

It can be Null when the native IO isolate is allocating memory, see
Dart_ScopeAllocate in dart_api_impl.cc

We need to account for this in a special category called native IO isolate, the
recent OOM issue that we ran into during the Fuchsia port was from this entity.

[bkonyi, 2016-12-28 19:00:50]

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/isolate.cc#newco...
runtime/vm/isolate.cc:2107: isolate_memory_high_watermark_);
On 2016/12/28 17:52:35, siva wrote:
> On 2016/12/28 00:09:58, bkonyi wrote:
> > On 2016/12/27 22:09:26, siva wrote:
> > > Do we have a comment section somewhere were we document that these strings
> > like
> > > "isolateMemoryHighWatermark" etc. should match up with the strings used in
> the
> > > corresponding Dart code?
> > 
> > There is the Dart VM service protocol document under runtime/vm/service/
which
> > specifies the types of objects and properties found in the JSON. However, I
> > haven't made any changes to that for the previous changes that surfaced zone
> > information, so I didn't think to do it for this change. Should I add the
high
> > watermark information at least?
> 
> Please check with John/Todd on the protocol for updating this document.

Done.

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

https://codereview.chromium.org/2608463002/diff/1/runtime/vm/zone.cc#newcode49
runtime/vm/zone.cc:49: }
On 2016/12/28 17:52:35, siva wrote:
> On 2016/12/28 00:09:58, bkonyi wrote:
> > On 2016/12/27 22:09:26, siva wrote:
> > > Please hoist Thread:Current() outside the loop into a local variable
> > > instead of accessing thread local storage at every iteration of the loop.
> > > 
> > > Thread* thread = Thread::Current();
> > > while (...)
> > >   if (thread != NULL) {
> > >     thread->Decrement....(...);
> > >   }
> > >   ....
> > > }
> > > 
> > > When is Thread::Current() null? (is it just unit tests?)
> > 
> > Done.
> > 
> > There's a case when running the unit tests where it seems a zone is
allocated
> > and not associated with a thread, so we get a segfault when we delete
segments
> > from that zone.
> 
> It can be Null when the native IO isolate is allocating memory, see
> Dart_ScopeAllocate in dart_api_impl.cc
> 
> We need to account for this in a special category called native IO isolate,
the
> recent OOM issue that we ran into during the Fuchsia port was from this
entity.

I've added a TODO to handle the special case when the native IO isolate in a
separate change.

[siva, 2016-12-28 22:55:38]

lgtm

[bkonyi, 2016-12-28 22:59:31]

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=asiva@google.com

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

[bkonyi, 2016-12-28 22:59:32]

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