Created methods to surface zone memory information for each isolate and thread in JSON.

Created methods to surface zone memory information for each isolate and thread in JSON.

BUG=
R=asiva@google.com, johnmccutchan@google.com, zra@google.com

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

[bkonyi, 2016-12-06 19:03:23]

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

[bkonyi, 2016-12-06 19:03:24]

This is all the functionality required to move forward with passing the zone
memory information to the observatory. For context, the information that is
collected is the following:
-Addresses of each isolate.
-Threads associated with each isolate, both free and active.
-Zones associated with each thread, their size, and the total size for all zones
in the thread.

Added John to see if he feels any other information associated with zone memory
allocation should be surfaced as well.

[Cutch, 2016-12-06 19:11:15]

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2500: void
Isolate::PrintAllIsolatesMemoryInfoToJSONLocked(JSONStream* stream) {
The suffix of 'Locked' is usually applied to functions which assume the lock is
already taken. I would recommend dropping the 'Locked' suffix.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2504: while (current != NULL) {
Please use Isolates::VisitIsolates instead of walking the list yourself.  It
also takes care of the locking too.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:250: #ifndef RELEASE
this doesn't look right.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:257: JSONObject obj(&stream);
These should probably be rewritten as service tests (see service_test.cc)

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:209: #ifndef RELEASE
dito

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:218: obj->AddPropertyF("thread_address", "0x%" Px "",
we use dartCamelCase for service protocol property names.

Not sure why we are printing the thread address here.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:650: #ifndef RELEASE
This doesn't look like the right #ifdef

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.h (right):

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:15: #ifndef RELEASE
here too.

We typically write:

#if DEFINED(DEBUG)
#endif

or 

#ifndef PRODUCT
#endif

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:267: obj->AddPropertyF("address", "0x%" Px "",
reinterpret_cast<uword>(zone));
don't forget "type" and "id" fields.

[zra, 2016-12-06 19:20:53]

High level comment: We build in three modes, DEBUG, RELEASE, and PRODUCT.
PRODUCT mode is the only one that doesn't include the vm service and observatory
and so doesn't need the code to print to JSON. So all the places where you have:

#ifndef RELEASE

should probably be

#ifndef PRODUCT

If you want something to be only in DEBUG you would say:

#if defined(DEBUG)

[bkonyi, 2016-12-06 19:23:43]

On 2016/12/06 19:20:53, zra wrote:
> High level comment: We build in three modes, DEBUG, RELEASE, and PRODUCT.
> PRODUCT mode is the only one that doesn't include the vm service and
observatory
> and so doesn't need the code to print to JSON. So all the places where you
have:
> 
> #ifndef RELEASE
> 
> should probably be
> 
> #ifndef PRODUCT
> 
> If you want something to be only in DEBUG you would say:
> 
> #if defined(DEBUG)

That makes sense. I had seen a lot of similar code using #ifndef RELEASE, so I
made the assumption that was the correct way to do it. Thanks for clearing it
up!

[siva, 2016-12-07 22:12:53]

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2098: reinterpret_cast<uword>(this));
Can we use isolate id instead of isolate address here. I think the service
protocol uses isolate ids to track isolates.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:471: void PrintThreadsInfoToJSONObject(JSONObject* obj);
Can these two methods PrintJSON and PrintThreadsInf... be const methods?

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:265: reinterpret_cast<uword>(isolate));
We use OS::SNPrint and not snprintf in order to make the code portable.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:267: 
How does this verify all the threads info that was added to the JSON stream.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:300: }
Why does this have to use so many monitors one for each thread started.

Should be able to do have a single monitor with a tasks_started_ counter.

Here you would do
{
  MonitorLocker ml(tasks_started_monitor);
  while (tasks_started_ != kTaskCount) {
    ml.Wait();
  }
}

an in each of the tasks once they are started you could do:

{
  MonitorLocker ml(tasks_started_monitor);
  tasks_started_ += 1;
}

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:355: }
Ditto comment about waiting for tasks to exit, could be done with one monitor.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:206: snprintf(size_buf, sizeof(size_buf),
"\"size\":%ld", zone.SizeInBytes());
Ditto comment about using OS::SNPrint instead of snprintf

[bkonyi, 2016-12-08 18:04:14]

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2098: reinterpret_cast<uword>(this));
On 2016/12/07 22:12:52, siva wrote:
> Can we use isolate id instead of isolate address here. I think the service
> protocol uses isolate ids to track isolates.

Yes, I've gone ahead and switched from addresses to ids for isolates and
threads.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2500: void
Isolate::PrintAllIsolatesMemoryInfoToJSONLocked(JSONStream* stream) {
On 2016/12/06 19:11:14, Cutch wrote:
> The suffix of 'Locked' is usually applied to functions which assume the lock
is
> already taken. I would recommend dropping the 'Locked' suffix.

Done.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:2504: while (current != NULL) {
On 2016/12/06 19:11:14, Cutch wrote:
> Please use Isolates::VisitIsolates instead of walking the list yourself.  It
> also takes care of the locking too.

Done.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate.h#ne...
runtime/vm/isolate.h:471: void PrintThreadsInfoToJSONObject(JSONObject* obj);
On 2016/12/07 22:12:52, siva wrote:
> Can these two methods PrintJSON and PrintThreadsInf... be const methods?

ServiceEvent requires an Isolate* argument and has a method isolate() which
returns an Isolate* as well. I'm not sure we could easily make changes to pass
an argument of type 'const Isolate* const' instead.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:250: #ifndef RELEASE
On 2016/12/06 19:11:14, Cutch wrote:
> this doesn't look right.

Yes, you're right. I've switched to if defined(DEBUG).

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:257: JSONObject obj(&stream);
On 2016/12/06 19:11:14, Cutch wrote:
> These should probably be rewritten as service tests (see service_test.cc)

As discussed offline, will add a test once I start using the service to
communicate with the Observatory.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:265: reinterpret_cast<uword>(isolate));
On 2016/12/07 22:12:52, siva wrote:
> We use OS::SNPrint and not snprintf in order to make the code portable.

Acknowledged.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:267: 
On 2016/12/07 22:12:52, siva wrote:
> How does this verify all the threads info that was added to the JSON stream.

Acknowledged. I'll be removing this test either way.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:209: #ifndef RELEASE
On 2016/12/06 19:11:15, Cutch wrote:
> dito

Done.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:218: obj->AddPropertyF("thread_address", "0x%" Px "",
On 2016/12/06 19:11:15, Cutch wrote:
> we use dartCamelCase for service protocol property names.
> 
> Not sure why we are printing the thread address here.

Done.

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

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread.h#new...
runtime/vm/thread.h:650: #ifndef RELEASE
On 2016/12/06 19:11:15, Cutch wrote:
> This doesn't look like the right #ifdef

Done.

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.h (right):

https://codereview.chromium.org/2554983002/diff/20001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:15: #ifndef RELEASE
On 2016/12/06 19:11:15, Cutch wrote:
> here too.
> 
> We typically write:
> 
> #if DEFINED(DEBUG)
> #endif
> 
> or 
> 
> #ifndef PRODUCT
> #endif

Done.

[zra, 2016-12-08 18:54:14]

We probably want this in both DEBUG and RELEASE builds, so we should use #ifndef
PRODUCT instead of #if defined(DEBUG).

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/isolate_test.cc
File runtime/vm/isolate_test.cc (left):

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:248: 
Did clang-format remove this? I think we usually keep a space after a function
here.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/json_stream....
runtime/vm/json_stream.cc:554: 
Please keep two newlines between functions in source files.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/json_stream....
runtime/vm/json_stream.cc:559: 
ditto

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3761: explicit ServiceIsolateVisitor(JSONArray* jsarr,
bool ref = true)
Unless I'm missing something, this class only has one instantiation, so I think
we don't need to use an optional argument here.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:209: #if defined(DEBUG)
#ifndef PRODUCT?

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:211: void Thread::PrintJSON(JSONStream* stream, bool ref)
const {
ref is unused.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.h#new...
runtime/vm/thread.h:650: #if defined(DEBUG)
Don't we also want this in a RELEASE build? Should this be #ifndef PRODUCT?

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.h#new...
runtime/vm/thread.h:651: void PrintJSON(JSONStream* stream, bool ref = true)
const;
Is this ever called with anything overriding the default for 'ref'? If not,
let's remove the argument.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:95: #if defined(DEBUG)
ditto

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:15: #if defined(DEBUG)
#ifndef PRODUCT?

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:36: #if defined(DEBUG)
ditto

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:37: void PrintJSON(JSONStream* stream, bool ref =
true) const;
ditto

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:51: #if defined(DEBUG)
ditto

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:208: #if defined(DEBUG)
ditto

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:278: }
missing newline.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:301: for (int i = 0; i < kTaskCount; i++) {
intptr_t i

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:320: for (int i = 0; i < kTaskCount + 1; i++) {
intptr_t i

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:263: #if defined(DEBUG)
ditto

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:273: 
two newlines between functions.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:81: #if defined(DEBUG)
ditto

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:82: void PrintJSON(JSONStream* stream, bool ref = true) const;
ditto

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:171: 
two newlines between functions.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:172: #if defined(DEBUG)
ditto

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:225: #endif
missing newline

[bkonyi, 2016-12-08 20:58:33]

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/isolate_test.cc
File runtime/vm/isolate_test.cc (left):

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/isolate_test...
runtime/vm/isolate_test.cc:248: 
On 2016/12/08 18:54:13, zra wrote:
> Did clang-format remove this? I think we usually keep a space after a function
> here.

I must have accidentally removed it when I deleted some tests I had written that
weren't useful. I'll add it back.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/json_stream....
runtime/vm/json_stream.cc:554: 
On 2016/12/08 18:54:13, zra wrote:
> Please keep two newlines between functions in source files.

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/json_stream....
runtime/vm/json_stream.cc:559: 
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3761: explicit ServiceIsolateVisitor(JSONArray* jsarr,
bool ref = true)
On 2016/12/08 18:54:13, zra wrote:
> Unless I'm missing something, this class only has one instantiation, so I
think
> we don't need to use an optional argument here.

You're right, it's only instantiated once. I've removed the ref parameter since
it should always be false given the implementation of PrintJSONForVM

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:209: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> #ifndef PRODUCT?

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.cc#ne...
runtime/vm/thread.cc:211: void Thread::PrintJSON(JSONStream* stream, bool ref)
const {
On 2016/12/08 18:54:13, zra wrote:
> ref is unused.

Removed ref.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.h#new...
runtime/vm/thread.h:650: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> Don't we also want this in a RELEASE build? Should this be #ifndef PRODUCT?

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread.h#new...
runtime/vm/thread.h:651: void PrintJSON(JSONStream* stream, bool ref = true)
const;
On 2016/12/08 18:54:13, zra wrote:
> Is this ever called with anything overriding the default for 'ref'? If not,
> let's remove the argument.

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:95: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:15: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> #ifndef PRODUCT?

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:36: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:37: void PrintJSON(JSONStream* stream, bool ref =
true) const;
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:51: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:208: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:278: }
On 2016/12/08 18:54:13, zra wrote:
> missing newline.

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:301: for (int i = 0; i < kTaskCount; i++) {
On 2016/12/08 18:54:13, zra wrote:
> intptr_t i

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:320: for (int i = 0; i < kTaskCount + 1; i++) {
On 2016/12/08 18:54:13, zra wrote:
> intptr_t i

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:263: #if defined(DEBUG)
On 2016/12/08 18:54:13, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.cc#newc...
runtime/vm/zone.cc:273: 
On 2016/12/08 18:54:14, zra wrote:
> two newlines between functions.

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:81: #if defined(DEBUG)
On 2016/12/08 18:54:14, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone.h#newco...
runtime/vm/zone.h:82: void PrintJSON(JSONStream* stream, bool ref = true) const;
On 2016/12/08 18:54:14, zra wrote:
> ditto

Done.

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

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:171: 
On 2016/12/08 18:54:14, zra wrote:
> two newlines between functions.

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:172: #if defined(DEBUG)
On 2016/12/08 18:54:14, zra wrote:
> ditto

Done.

https://codereview.chromium.org/2554983002/diff/40001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:225: #endif
On 2016/12/08 18:54:14, zra wrote:
> missing newline

Done.

[Cutch, 2016-12-08 22:25:54]

Final comments.

https://codereview.chromium.org/2554983002/diff/60001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2554983002/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3767: jsarr_->AddValue(isolate, false);
why is this necessary?

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

https://codereview.chromium.org/2554983002/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:100: PrintJSONHelper(&threads, free_list_);
We shouldn't include the free list. These are Thread objects which aren't
actually owned anymore.

[bkonyi, 2016-12-08 22:42:01]

https://codereview.chromium.org/2554983002/diff/60001/runtime/vm/service.cc
File runtime/vm/service.cc (right):

https://codereview.chromium.org/2554983002/diff/60001/runtime/vm/service.cc#n...
runtime/vm/service.cc:3767: jsarr_->AddValue(isolate, false);
On 2016/12/08 22:25:54, Cutch wrote:
> why is this necessary?

Currently, thread information is only being included when its parent isolate is
of type Isolate, not @Isolate. Also, since ServiceIsolateVisitor is only ever
used when ref == false in the VM print to JSON code, it seemed logical to
hardcode the ref parameter for printing the isolate information as false.

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

https://codereview.chromium.org/2554983002/diff/60001/runtime/vm/thread_regis...
runtime/vm/thread_registry.cc:100: PrintJSONHelper(&threads, free_list_);
On 2016/12/08 22:25:54, Cutch wrote:
> We shouldn't include the free list. These are Thread objects which aren't
> actually owned anymore.

I'll go ahead and remove this then.

[bkonyi, 2016-12-09 01:56:41]

On 2016/12/08 22:42:03, bkonyi wrote:

I talked with John and I've addressed all the feedback he has at the moment.
@siva and @zra, is there anything else you see that needs to be changed?

[siva, 2016-12-09 02:49:39]

lgtm

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (right):

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:326: char isolate_address_buf[64];
Instead of using a local buffer with a number that might overflow if something
changes you could do the following:

set up :
Thread* current_thread = Thread::Current();
StackZone stack_zone(current_thread);
Zone* zone = current_thread->zone();

and replace the OS:;SNPrint with OS::SCreate...
char* zone_info_buf = OS::SCreate(zone, "\"type\" .....);

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/zone_test.cc
File runtime/vm/zone_test.cc (right):

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:177: #endif
trace_zones does not seem to be used in this method, looks like some debug stuff
you added and could be removed.

[zra, 2016-12-09 17:51:37]

lgtm with nit.

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.h (right):

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:16: class JSONStream;
These should probably go inside 'namespace dart' below.

[bkonyi, 2016-12-09 17:52:57]

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_regis...
File runtime/vm/thread_registry.h (right):

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_regis...
runtime/vm/thread_registry.h:16: class JSONStream;
On 2016/12/09 17:51:36, zra wrote:
> These should probably go inside 'namespace dart' below.

Done.

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_test.cc
File runtime/vm/thread_test.cc (right):

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/thread_test....
runtime/vm/thread_test.cc:326: char isolate_address_buf[64];
On 2016/12/09 02:49:39, siva wrote:
> Instead of using a local buffer with a number that might overflow if something
> changes you could do the following:
> 
> set up :
> Thread* current_thread = Thread::Current();
> StackZone stack_zone(current_thread);
> Zone* zone = current_thread->zone();
> 
> and replace the OS:;SNPrint with OS::SCreate...
> char* zone_info_buf = OS::SCreate(zone, "\"type\" .....);

Done.

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/zone_test.cc
File runtime/vm/zone_test.cc (right):

https://codereview.chromium.org/2554983002/diff/80001/runtime/vm/zone_test.cc...
runtime/vm/zone_test.cc:177: #endif
On 2016/12/09 02:49:39, siva wrote:
> trace_zones does not seem to be used in this method, looks like some debug
stuff
> you added and could be removed.

I added this since it appears in all of the zone tests include this block for
debug mode. I'm assuming it's referenced somewhere else for debugging purposes?

[Cutch, 2016-12-09 17:55:30]

lgtm

[bkonyi, 2016-12-12 17:56:55]

Description was changed from

==========
Created methods to surface zone memory information for each isolate and thread
in JSON.

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

to

==========
Created methods to surface zone memory information for each isolate and thread
in JSON.

BUG=
R=asiva@google.com, johnmccutchan@google.com, zra@google.com

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

[bkonyi, 2016-12-12 17:56:56]

Committed patchset #6 (id:100001) manually as
35a49bc1ff57ce8405db80f766edaa6ba6126328 (presubmit successful).