Support stacktrace and objecthistogram service commands

Support stacktrace and objecthistogram service commands

R=asiva@google.com

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

[Cutch, 2013-07-31 21:55:57]

PTAL

[siva, 2013-08-01 18:16:18]

LGTM with some comments.

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/run...
File runtime/bin/vmservice/running_isolate.dart (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/run...
runtime/bin/vmservice/running_isolate.dart:51: name = response['name'];
why not move this line into the try block.

response could then become a local variable in that block.

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
runtime/bin/vmservice/server.dart:43: serviceRequest.setErrorResponse('No route
for: $path');
This seems to override the error response set in the
'if (!r)' block. Maybe you could restructure this code:

if (!r) {
  serviceRequest.setErrorResponse('Invalid request uri: ${request.uri}');
} else {
  var f = service.runningIsolates.route(serviceRequest);
  if (f != null) {
    f.then((_) { ... });
    return;
  }
}
// Send error response back over HTTP.
.....

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
File runtime/bin/vmservice/service_request.dart (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
runtime/bin/vmservice/service_request.dart:38:
m.add(parameters.values.toList());
Might be more readable and more efficient as:

return [pathSegments, parameters.keys.toList(), parameters.values.toList()];

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/disassembler.h
File runtime/vm/disassembler.h (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/disassembler.h...
runtime/vm/disassembler.h:62: stream_(stream) {}
{ } to stick to the style in this file.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram.cc
File runtime/vm/heap_histogram.cc (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:120: for (int pos = 0; pos < length; pos++) {
Ditto question about 'int' here.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:176: count_sum += e->count_ / major_gc_count_;
(e->size_ / major_gc_count_) and (e->count_ / major_gc_count_) for
better readability.

The security review folks are going to ask about overflow here.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram.h
File runtime/vm/heap_histogram.h (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:59: Element** GetSortedArray(int* array_length);
why 'int' and not 'intptr_t'?

We should also document thath this function returns a malloced array and the
caller is responsible for deleting the array.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:7285: const Class& klass =
Class::Handle(class_iter.GetNextClass());
Hoist the handle creation outside the loop.

Class& klass = Class::Handle();
while (class_iter.HasNext()) {
  klass = class....;
  ...
}

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:7291: const Library& lib =
Library::Handle(ImportLibraryAt(i));
Ditto comment about handle creation to be hoisted out.

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

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:47: ASSERT(msg.IsGrowableObjectArray());
probably would become IsObjectArray() if you make the suggested change in dart
code.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:47: ASSERT(msg.IsGrowableObjectArray());
would become IsObjectArray() if you made the suggested change in the
corresponding dart code.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:57: GrowableObjectArray& option_values =
GrowableObjectArray::Handle();
option_keys and option_values seem to be only used in ASSERTs below,
should we just put all of this and the ASSERTs under
#if defined(DEBUG)
...
#endif.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:171: 
Extra blank line.

[Cutch, 2013-08-01 22:22:04]

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/run...
File runtime/bin/vmservice/running_isolate.dart (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/run...
runtime/bin/vmservice/running_isolate.dart:51: name = response['name'];
On 2013/08/01 18:16:18, siva wrote:
> why not move this line into the try block.
> 
> response could then become a local variable in that block.

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
File runtime/bin/vmservice/server.dart (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
runtime/bin/vmservice/server.dart:43: serviceRequest.setErrorResponse('No route
for: $path');
On 2013/08/01 18:16:18, siva wrote:
> This seems to override the error response set in the
> 'if (!r)' block. Maybe you could restructure this code:
> 
> if (!r) {
>   serviceRequest.setErrorResponse('Invalid request uri: ${request.uri}');
> } else {
>   var f = service.runningIsolates.route(serviceRequest);
>   if (f != null) {
>     f.then((_) { ... });
>     return;
>   }
> }
> // Send error response back over HTTP.
> .....

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
File runtime/bin/vmservice/service_request.dart (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/bin/vmservice/ser...
runtime/bin/vmservice/service_request.dart:38:
m.add(parameters.values.toList());
On 2013/08/01 18:16:18, siva wrote:
> Might be more readable and more efficient as:
> 
> return [pathSegments, parameters.keys.toList(), parameters.values.toList()];

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/disassembler.h
File runtime/vm/disassembler.h (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/disassembler.h...
runtime/vm/disassembler.h:62: stream_(stream) {}
On 2013/08/01 18:16:18, siva wrote:
> { } to stick to the style in this file.

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram.cc
File runtime/vm/heap_histogram.cc (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:120: for (int pos = 0; pos < length; pos++) {
On 2013/08/01 18:16:18, siva wrote:
> Ditto question about 'int' here.

Legacy code used int. I'll change it to intptr_t.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.cc:176: count_sum += e->count_ / major_gc_count_;
On 2013/08/01 18:16:18, siva wrote:
> (e->size_ / major_gc_count_) and (e->count_ / major_gc_count_) for
> better readability.
> 
> The security review folks are going to ask about overflow here.

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram.h
File runtime/vm/heap_histogram.h (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/heap_histogram...
runtime/vm/heap_histogram.h:59: Element** GetSortedArray(int* array_length);
On 2013/08/01 18:16:18, siva wrote:
> why 'int' and not 'intptr_t'?
> 
> We should also document thath this function returns a malloced array and the
> caller is responsible for deleting the array.

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/object.cc
File runtime/vm/object.cc (right):

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:7285: const Class& klass =
Class::Handle(class_iter.GetNextClass());
On 2013/08/01 18:16:18, siva wrote:
> Hoist the handle creation outside the loop.
> 
> Class& klass = Class::Handle();
> while (class_iter.HasNext()) {
>   klass = class....;
>   ...
> }

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/object.cc#newc...
runtime/vm/object.cc:7291: const Library& lib =
Library::Handle(ImportLibraryAt(i));
On 2013/08/01 18:16:18, siva wrote:
> Ditto comment about handle creation to be hoisted out.

Done.

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

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:47: ASSERT(msg.IsGrowableObjectArray());
On 2013/08/01 18:16:18, siva wrote:
> probably would become IsObjectArray() if you make the suggested change in dart
> code.

Done.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:57: GrowableObjectArray& option_values =
GrowableObjectArray::Handle();
option_keys and option_values will be used in a future CL.

https://codereview.chromium.org/19870006/diff/14001/runtime/vm/service.cc#new...
runtime/vm/service.cc:171: 
On 2013/08/01 18:16:18, siva wrote:
> Extra blank line.

Done.

[Cutch, 2013-08-02 00:24:38]

Committed patchset #7 manually as r25726 (presubmit successful).