Changes to support dprof and Observatory profiler UIs

Build map of code regions and address ticks for profile report.
Profile pauses should be less than 500 ms even when handling 120,000 samples (2 minutes @ 1000 hz).
Code objects are referenced by hex address: "code/cafebabe".
Test for retrieving code objects over the service.
Remove old profile tool scripts. Replaced by dprof (https://github.com/dart-lang/dprof) and Observatory UI for profiler.
Support for stable service refs to implicit closures and dispatcher functions.

R=srdjan@google.com

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

[Cutch, 2013-12-30 17:41:31]

Added Srdjan for the CodePatcher changes.

[srdjan, 2013-12-30 17:54:46]

LGTM with comments for code patching part.

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/code_patcher.cc
File runtime/vm/code_patcher.cc (right):

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/code_patcher...
runtime/vm/code_patcher.cc:69: const uword patch_addr =
code.GetPcForDeoptId(Isolate::kNoDeoptId,
weird indent

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

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9710: }
Add comment that restore/repatch is necesseary to prevent disassmbler from
choking on illegal instructions.

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/service_test...
runtime/vm/service_test.cc:97: */
Remove dead code above.

[Cutch, 2013-12-30 21:32:39]

srdjan comments addressed + function reference fixes.

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/code_patcher.cc
File runtime/vm/code_patcher.cc (right):

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/code_patcher...
runtime/vm/code_patcher.cc:69: const uword patch_addr =
code.GetPcForDeoptId(Isolate::kNoDeoptId,
On 2013/12/30 17:54:46, srdjan wrote:
> weird indent

Done.

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

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9710: }
On 2013/12/30 17:54:46, srdjan wrote:
> Add comment that restore/repatch is necesseary to prevent disassmbler from
> choking on illegal instructions.

Done.

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/service_test.cc
File runtime/vm/service_test.cc (right):

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/service_test...
runtime/vm/service_test.cc:97: */
On 2013/12/30 17:54:46, srdjan wrote:
> Remove dead code above.

Done.

[Ivan Posva, 2013-12-30 23:06:08]

https://codereview.chromium.org/100103011/diff/270001/runtime/bin/vmservice/c...
File
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_summary.html
(right):

https://codereview.chromium.org/100103011/diff/270001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_summary.html:8:
<img src="img/isolate_icon.png" class="img-polaroid">
TABS

https://codereview.chromium.org/100103011/diff/270001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/pubspec.yaml (right):

https://codereview.chromium.org/100103011/diff/270001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/pubspec.yaml:6: git:
https://github.com/dart-lang/dprof
We cannot have external dependencies here. It will become unmanageable.

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

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5725: bool object_ref = id < 0;
Please handle implicit closure functions as discussed. They are tied to their
"defining" function and thus discoverable through the functions array.

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:409: if (dart_symbol_.InDartCodeSpace(pc)) {
Restructuring this as discussed will avoid iterating over all pages twice.

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:419: new_code ^= Code::LookupCode(pc);
Here you are walking through the heap again. Looking for the same code you have
already found earlier.

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

https://codereview.chromium.org/100103011/diff/270001/runtime/vm/service.cc#n...
runtime/vm/service.cc:578: { "profile", HandleProfile },
We will want to separate profile from profile ticks here eventually. One being
useful for the online use case, where as the other will be useful for the
offline profiling tool.

[Cutch, 2014-01-02 22:12:52]

https://codereview.chromium.org/100103011/diff/650001/runtime/bin/vmservice/c...
File runtime/bin/vmservice/client/pubspec.yaml (right):

https://codereview.chromium.org/100103011/diff/650001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/pubspec.yaml:5: dprof:
NOTE: Temporary. This will go away in the near future. But I would like to use
it for now.

[Ivan Posva, 2014-01-03 01:52:16]

First set of comments...

-Ivan

https://codereview.chromium.org/100103011/diff/650001/runtime/bin/vmservice/c...
File
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_summary.html
(right):

https://codereview.chromium.org/100103011/diff/650001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_summary.html:8:
<img src="img/isolate_icon.png" class="img-polaroid">
TABS

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

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5829: if (IsNonImplicitClosureFunction()) {
Please consolidate this lookup with the one below. For example:

intptr_t id = -1;
const char* selector = NULL;
if (IsNonImplicitClosureFunction()) {
  id = cls.FindClosureIndex(*this);
  selector = "closures";
} else if ... {
}
...
jsobj.AddPropertyF("id", "classes/%"Pd"/%s/%"Pd, cid, selector, id);

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9795: void Code::Disassemble(DisassemblyFormatter*
formatter) const {
Can you consolidate these two identical functions and use a default value such
as NULL for a non-existing formatter?

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object_test....
runtime/vm/object_test.cc:3471: // Tests both FindFunctionIndex and
FindImplicitClosureFunctionIndex.
Can you add the dispatcher lookups to the tests as well?

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:486: UNREACHABLE();
You mix two styles in this function here:
if (native_name == NULL) {
  return new CodeRegion(...);
}
...
return new CodeRevion(...);

and

if (native_name == NULL) {
  return new CodeRegion(...);
} else {
  ...
  return new CodeRegion(...);
}
UNREACHABLE();
return NULL;

Personally, I find the first more readable in general especially if the cases
are short. Unless there is some potential shared processing at which point I try
to make all branches return at the end returning a result variable.

In the concrete example I would slightly restructure it to:

if (!code.IsNull()) {
  return new CodeRegion(kDartCode, code.EntryPoint(), code.EntryPoint() +
code.Size());
}

if (heap_->CodeContains(pc) {
  ...
  return new CodeRegion(kCollectedCode, ...);
}

...
return new CodeRegion(kNativeCode, ...);

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:499: ASSERT(region->kind() != CodeRegion::kDartCode);
How can kCollectedCode regions overlap?

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:504: delete code_region;
Why don't you make all of this data zone allocated and then you do not have to
deal with cleanup? A lot of it is already zone allocated already.

[Cutch, 2014-01-03 16:01:55]

All comments addressed.
Tests added.

https://codereview.chromium.org/100103011/diff/650001/runtime/bin/vmservice/c...
File
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_summary.html
(right):

https://codereview.chromium.org/100103011/diff/650001/runtime/bin/vmservice/c...
runtime/bin/vmservice/client/lib/src/observatory_elements/isolate_summary.html:8:
<img src="img/isolate_icon.png" class="img-polaroid">
On 2014/01/03 01:52:17, Ivan Posva wrote:
> TABS

Done.

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

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:5829: if (IsNonImplicitClosureFunction()) {
On 2014/01/03 01:52:17, Ivan Posva wrote:
> Please consolidate this lookup with the one below. For example:
> 
> intptr_t id = -1;
> const char* selector = NULL;
> if (IsNonImplicitClosureFunction()) {
>   id = cls.FindClosureIndex(*this);
>   selector = "closures";
> } else if ... {
> }
> ...
> jsobj.AddPropertyF("id", "classes/%"Pd"/%s/%"Pd, cid, selector, id);

Done.

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:9795: void Code::Disassemble(DisassemblyFormatter*
formatter) const {
On 2014/01/03 01:52:17, Ivan Posva wrote:
> Can you consolidate these two identical functions and use a default value such
> as NULL for a non-existing formatter?

Done.

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object_test.cc
File runtime/vm/object_test.cc (right):

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/object_test....
runtime/vm/object_test.cc:3471: // Tests both FindFunctionIndex and
FindImplicitClosureFunctionIndex.
On 2014/01/03 01:52:17, Ivan Posva wrote:
> Can you add the dispatcher lookups to the tests as well?

Done.

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:486: UNREACHABLE();
On 2014/01/03 01:52:17, Ivan Posva wrote:
> You mix two styles in this function here:
> if (native_name == NULL) {
>   return new CodeRegion(...);
> }
> ...
> return new CodeRevion(...);
> 
> and
> 
> if (native_name == NULL) {
>   return new CodeRegion(...);
> } else {
>   ...
>   return new CodeRegion(...);
> }
> UNREACHABLE();
> return NULL;
> 
> Personally, I find the first more readable in general especially if the cases
> are short. Unless there is some potential shared processing at which point I
try
> to make all branches return at the end returning a result variable.
> 
> In the concrete example I would slightly restructure it to:
> 
> if (!code.IsNull()) {
>   return new CodeRegion(kDartCode, code.EntryPoint(), code.EntryPoint() +
> code.Size());
> }
> 
> if (heap_->CodeContains(pc) {
>   ...
>   return new CodeRegion(kCollectedCode, ...);
> }
> 
> ...
> return new CodeRegion(kNativeCode, ...);

Done.

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:499: ASSERT(region->kind() != CodeRegion::kDartCode);
On 2014/01/03 01:52:17, Ivan Posva wrote:
> How can kCollectedCode regions overlap?

Fixed assertion to only expect native code objects to overlap.

https://codereview.chromium.org/100103011/diff/650001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:504: delete code_region;
On 2014/01/03 01:52:17, Ivan Posva wrote:
> Why don't you make all of this data zone allocated and then you do not have to
> deal with cleanup? A lot of it is already zone allocated already.

Done.

[Ivan Posva, 2014-01-03 19:24:53]

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_android.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_android.cc:32: *start =
reinterpret_cast<uintptr_t>(info.dli_saddr);
if (start != NULL) {
  *start = ...
}

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_linux.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_linux.cc:33: *start =
reinterpret_cast<uintptr_t>(info.dli_saddr);
ditto

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_macos.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_macos.cc:33: *start =
reinterpret_cast<uintptr_t>(info.dli_saddr);
ditto

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_win.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_win.cc:70: return strdup(pSymbol->Name);
if (start != NULL) {
  *start = NULL;  // Or something reasonable.
}

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:246: ~CodeRegion() {
ZoneAllocated and thus CodeRegion destructors are never called. Please remove
this constructor, as it gives the wrong impression.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:437: (*code_region_table_)[index]->AddTickAtAddress(pc);
This will distort the exclusive ticks with too many ticks at the call sites.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:604: code_region_table->AddTick(sample->pcs[i], false);
Double counting the exclusive ticks as inclusive ticks here.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:49: SampleBuffer* sample_buffer);
It is unclear why you have to pass the sample_buffer as an argument here.

[Cutch, 2014-01-03 19:42:29]

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_android.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_android.cc:32: *start =
reinterpret_cast<uintptr_t>(info.dli_saddr);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> if (start != NULL) {
>   *start = ...
> }

Done.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_linux.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_linux.cc:33: *start =
reinterpret_cast<uintptr_t>(info.dli_saddr);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_macos.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_macos.cc:33: *start =
reinterpret_cast<uintptr_t>(info.dli_saddr);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> ditto

Done.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
File runtime/vm/native_symbol_win.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/native_symbo...
runtime/vm/native_symbol_win.cc:70: return strdup(pSymbol->Name);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> if (start != NULL) {
>   *start = NULL;  // Or something reasonable.
> }

Done.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc
File runtime/vm/profiler.cc (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:246: ~CodeRegion() {
On 2014/01/03 19:24:54, Ivan Posva wrote:
> ZoneAllocated and thus CodeRegion destructors are never called. Please remove
> this constructor, as it gives the wrong impression.

Done.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:437: (*code_region_table_)[index]->AddTickAtAddress(pc);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> This will distort the exclusive ticks with too many ticks at the call sites.

Good catch. Fixed by conditionally ticking the address.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:604: code_region_table->AddTick(sample->pcs[i], false);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> Double counting the exclusive ticks as inclusive ticks here.

Giving all code regions an inclusive tick  was intentional based on our time
looking at profile data at my desk. I agree that it is technically incorrect but
it seemed to give slightly more readable profiles.

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.h
File runtime/vm/profiler.h (right):

https://codereview.chromium.org/100103011/diff/740001/runtime/vm/profiler.h#n...
runtime/vm/profiler.h:49: SampleBuffer* sample_buffer);
On 2014/01/03 19:24:54, Ivan Posva wrote:
> It is unclear why you have to pass the sample_buffer as an argument here.

I don't have to but then I would have to grab it out of the isolate again.

[Ivan Posva, 2014-01-03 19:52:45]

Let's commit what you have and iterate on the rest.

-Ivan

[Cutch, 2014-01-03 19:56:10]

Committed patchset #18 manually as r31447 (presubmit successful).