Improve Code object support in service and observatory

Change Code object serialization format to use 50% less space.
Update observatory to support new format
Cache code objects inside observatory isolate
Profiler updates cache of code objects
Profiler can display annotated disassembly objects
Code objects now display sample ticks
Fix error viewer
Start using the logging package.

R=turnidge@google.com

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

[Cutch, 2014-01-17 05:16:36]

Todd,

This grew a bit, sorry.

John

[turnidge, 2014-01-17 19:30:20]

LGTM w/ comments below.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/observatory/model.dart (right):

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:26: formattedTicks =
percent.toStringAsFixed(2);
Is it important to cache the formattedTicks?  Would it really matter if we built
this string each time on demand with a getter?  Similar question for
formattedAddress.

I ask because iI suspect we will have many instructions in memory but the number
that will actually be printed at a time is relatively small.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:92: for (var i = 0;
i < instructions.length; i++) {
does this work instead of using an index to iterate here:

  for (var instruction in instructions) {

If so, consider changing here and below, as applies.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:112: if
(instructionList == null) {
Does this happen?

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:222: 
Too many blank lines?

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:223: List<Code>
topExclusive(int N) {
N uses a non-standard variable casing.  Consider "n" or maybe "count"?

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart
(right):

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart:26:
setResponseError('$e $st');
What if we returned the error response and checked the type of the response in
the caller instead of checking for null?

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart:177: var m
= JSON.decode(response);
We don't check for JSON decoding errors as we do above.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
File
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_profile.dart
(right):

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_profile.dart:120:
}
This function looks wrong.  Where is the main return?  Ditto codeUrlName below.

https://codereview.chromium.org/135843006/diff/140004/runtime/vm/disassembler.cc
File runtime/vm/disassembler.cc (right):

https://codereview.chromium.org/135843006/diff/140004/runtime/vm/disassembler...
runtime/vm/disassembler.cc:46: uint8_t* pc_ptr = reinterpret_cast<uint8_t*>(pc);
Could use a comment here.  "Instructions are represented as blah blah blah"

https://codereview.chromium.org/135843006/diff/140004/runtime/vm/disassembler...
runtime/vm/disassembler.cc:67: }
Could use a comment here.

[Cutch, 2014-01-17 21:15:42]

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/observatory/model.dart (right):

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:26: formattedTicks =
percent.toStringAsFixed(2);
On 2014/01/17 19:30:21, turnidge wrote:
> Is it important to cache the formattedTicks?  Would it really matter if we
built
> this string each time on demand with a getter?  Similar question for
> formattedAddress.
> 
> I ask because iI suspect we will have many instructions in memory but the
number
> that will actually be printed at a time is relatively small.

I only cache the percentage now (and only if it's not 0.0).

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:92: for (var i = 0;
i < instructions.length; i++) {
On 2014/01/17 19:30:21, turnidge wrote:
> does this work instead of using an index to iterate here:
> 
>   for (var instruction in instructions) {
> 
> If so, consider changing here and below, as applies.

Done.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:112: if
(instructionList == null) {
On 2014/01/17 19:30:21, turnidge wrote:
> Does this happen?

Done.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:222: 
On 2014/01/17 19:30:21, turnidge wrote:
> Too many blank lines?

Done.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/model.dart:223: List<Code>
topExclusive(int N) {
On 2014/01/17 19:30:21, turnidge wrote:
> N uses a non-standard variable casing.  Consider "n" or maybe "count"?

Done.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart
(right):

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart:26:
setResponseError('$e $st');
On 2014/01/17 19:30:21, turnidge wrote:
> What if we returned the error response and checked the type of the response in
> the caller instead of checking for null?

I'm going to punt on this because I plan on revisiting this code post summit.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory/request_manager.dart:177: var m
= JSON.decode(response);
On 2014/01/17 19:30:21, turnidge wrote:
> We don't check for JSON decoding errors as we do above. 

Done.

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
File
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_profile.dart
(right):

https://codereview.chromium.org/135843006/diff/140004/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_profile.dart:120:
}
On 2014/01/17 19:30:21, turnidge wrote:
> This function looks wrong.  Where is the main return?  Ditto codeUrlName
below.

Both were dead code. Removed.

https://codereview.chromium.org/135843006/diff/140004/runtime/vm/disassembler.cc
File runtime/vm/disassembler.cc (right):

https://codereview.chromium.org/135843006/diff/140004/runtime/vm/disassembler...
runtime/vm/disassembler.cc:46: uint8_t* pc_ptr = reinterpret_cast<uint8_t*>(pc);
On 2014/01/17 19:30:21, turnidge wrote:
> Could use a comment here.  "Instructions are represented as blah blah blah"

Done.

https://codereview.chromium.org/135843006/diff/140004/runtime/vm/disassembler...
runtime/vm/disassembler.cc:67: }
On 2014/01/17 19:30:21, turnidge wrote:
> Could use a comment here.

Done.

[Cutch, 2014-01-17 21:17:00]

Committed patchset #4 manually as r31931 (presubmit successful).