Add Function based profile tree

Add Function based profile tree

Profiler improvements:
- Track Functions in profile and build Function based trie
- Associate code objects with functions
- Created cpu_profile.dart library
- Major speed improvements for disassembly view
- Fix truncation of disassembly comments
- Ability to get code object ticks from disassembly view
- Inlining mini-map in disassembly view.
- Remove a bunch of unused data from profile service response
- In some cases a caller PC that is better than the PC marker is inserted into the stack trace
- Inlined functions are expanded
- Ability to clear profile
- New flag '--keep_code' which keeps deoptimized code around for use by the profiler.

General fixes:
- Fix caching in service library
- Remove pubspec.yaml before running pub get

R=asiva@google.com, rmacnak@google.com

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

[Cutch, 2015-02-17 23:35:40]

johnmccutchan@google.com changed reviewers:
+ asiva@google.com, rmacnak@google.com

[rmacnak, 2015-02-19 23:49:54]

Please add an initializer for start_ in ScopeTimer.

https://codereview.chromium.org/928833003/diff/60001/runtime/observatory/lib/...
File runtime/observatory/lib/src/elements/code_view.dart (right):

https://codereview.chromium.org/928833003/diff/60001/runtime/observatory/lib/...
runtime/observatory/lib/src/elements/code_view.dart:54: new
CpuProfile()..load(isolate, response);
No need to cascade.

https://codereview.chromium.org/928833003/diff/60001/runtime/observatory/lib/...
runtime/observatory/lib/src/elements/code_view.dart:140: cell.title =
row.values[i].toString();
Why set the tooltip if it is the same?

[Cutch, 2015-02-20 01:49:50]

On 2015/02/19 23:49:54, rmacnak wrote:
> Please add an initializer for start_ in ScopeTimer.
> 
> 

Done.

[Cutch, 2015-02-20 01:49:57]

https://codereview.chromium.org/928833003/diff/60001/runtime/observatory/lib/...
File runtime/observatory/lib/src/elements/code_view.dart (right):

https://codereview.chromium.org/928833003/diff/60001/runtime/observatory/lib/...
runtime/observatory/lib/src/elements/code_view.dart:54: new
CpuProfile()..load(isolate, response);
On 2015/02/19 23:49:53, rmacnak wrote:
> No need to cascade.

Done.

https://codereview.chromium.org/928833003/diff/60001/runtime/observatory/lib/...
runtime/observatory/lib/src/elements/code_view.dart:140: cell.title =
row.values[i].toString();
On 2015/02/19 23:49:53, rmacnak wrote:
> Why set the tooltip if it is the same?

Acknowledged.

[rmacnak, 2015-02-23 23:44:47]

https://chromiumcodereview.appspot.com/928833003/diff/120001/runtime/vm/instr...
File runtime/vm/instructions_arm.h (right):

https://chromiumcodereview.appspot.com/928833003/diff/120001/runtime/vm/instr...
runtime/vm/instructions_arm.h:104: // lui; ori; jr; nop (in delay slot) = 4.
bx_lr = 1

https://chromiumcodereview.appspot.com/928833003/diff/120001/runtime/vm/instr...
File runtime/vm/instructions_arm64.h (right):

https://chromiumcodereview.appspot.com/928833003/diff/120001/runtime/vm/instr...
runtime/vm/instructions_arm64.h:113: // lui; ori; jr; nop (in delay slot) = 4.
bx_lr = 1

[Cutch, 2015-02-24 19:26:23]

PTAL Patch Set 8.

https://codereview.chromium.org/928833003/diff/120001/runtime/vm/instructions...
File runtime/vm/instructions_arm.h (right):

https://codereview.chromium.org/928833003/diff/120001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:104: // lui; ori; jr; nop (in delay slot) = 4.
On 2015/02/23 23:44:47, rmacnak wrote:
> bx_lr = 1

Done.

https://codereview.chromium.org/928833003/diff/120001/runtime/vm/instructions...
File runtime/vm/instructions_arm64.h (right):

https://codereview.chromium.org/928833003/diff/120001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:113: // lui; ori; jr; nop (in delay slot) = 4.
On 2015/02/23 23:44:47, rmacnak wrote:
> bx_lr = 1

Done.

[rmacnak, 2015-02-24 20:52:43]

https://codereview.chromium.org/928833003/diff/140001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/928833003/diff/140001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:49: intptr_t length_before = previous_.Length();
vm/cc/Service_Profile is hitting an assert here because previous_ is null. You
only allocate this list when the keep_code flag is set, but this code still runs
when the flag is not set.

[Cutch, 2015-02-24 20:54:37]

https://codereview.chromium.org/928833003/diff/140001/runtime/vm/profiler_ser...
File runtime/vm/profiler_service.cc (right):

https://codereview.chromium.org/928833003/diff/140001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:49: intptr_t length_before = previous_.Length();
On 2015/02/24 20:52:43, rmacnak wrote:
> vm/cc/Service_Profile is hitting an assert here because previous_ is null. You
> only allocate this list when the keep_code flag is set, but this code still
runs
> when the flag is not set.

Done.

[rmacnak, 2015-02-24 21:00:52]

LGTM

[siva, 2015-02-25 22:58:39]

LGTM with some comments.

https://codereview.chromium.org/928833003/diff/160001/runtime/observatory/lib...
File runtime/observatory/lib/src/cpu_profile/cpu_profile.dart (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:64:
addressTicks[address] = tick;
Does the key (address) also have to be in the CodeTick object?

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm.cc (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.cc:350: }
Why not inline this in the header file?

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:102: ReturnPattern(uword pc);
explicit ?

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:105: static const int kLengthInBytes =
1*Instr::kInstrSize;
spaces (1 * Instr)

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:107: int pattern_length_in_bytes() const {
This is not an accessor but it has the naming style of an accessor.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm64.cc (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.cc:370: : pc_(pc) {
Ditto comment about inlining this constructor.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm64.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:111: ReturnPattern(uword pc);
explicit ?

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:114: static const int kLengthInBytes =
1*Instr::kInstrSize;
Ditto comment about spaces.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:116: int pattern_length_in_bytes() const {
Ditto comment about name.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_ia32.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_ia32.h:103: virtual int pattern_length_in_bytes() const
{ return kLengthInBytes; }
I guess this name is coming from some existing code, not sure why it was done
like that ignore my comment on the name in other files.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_mips.cc (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_mips.cc:233: }
Inline this in the header file?

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_mips.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_mips.h:105: ReturnPattern(uword pc);
explicit

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_mips.h:108: static const int kLengthInBytes =
1*Instr::kInstrSize;
spaces ( 1 * Instr).

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

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1457: deoptimized_code_array_ = value.raw();
StorePointer(...);

I think we are missing this with some of the tag stuff too (e.g tag_table_,
current_tag_, default_tag_)

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1469: deoptimized_code.Add(code);
Can you add a TODO here that we will make this and the profiler samples GC aware
structures so that it can free up code objects which are not referenced in the
profile.

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

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12583: Smi& temp_smi = Smi::Handle();
This handle can be hoisted out of the for loop?

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

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:467: #endif
I am wondering if this target specific stuff should go into stack_frame_xxx.h

[Cutch, 2015-02-26 18:44:41]

https://codereview.chromium.org/928833003/diff/160001/runtime/observatory/lib...
File runtime/observatory/lib/src/cpu_profile/cpu_profile.dart (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/observatory/lib...
runtime/observatory/lib/src/cpu_profile/cpu_profile.dart:64:
addressTicks[address] = tick;
On 2015/02/25 22:58:38, siva wrote:
> Does the key (address) also have to be in the CodeTick object?

Done.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm.cc (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.cc:350: }
On 2015/02/25 22:58:38, siva wrote:
> Why not inline this in the header file?

Acknowledged.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:102: ReturnPattern(uword pc);
On 2015/02/25 22:58:38, siva wrote:
> explicit ?

Done.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:105: static const int kLengthInBytes =
1*Instr::kInstrSize;
On 2015/02/25 22:58:38, siva wrote:
> spaces (1 * Instr)

Done (here and elsewhere).

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm.h:107: int pattern_length_in_bytes() const {
On 2015/02/25 22:58:38, siva wrote:
> This is not an accessor but it has the naming style of an accessor.

Acknowledged.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_arm64.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:111: ReturnPattern(uword pc);
On 2015/02/25 22:58:38, siva wrote:
> explicit ?

Done.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:114: static const int kLengthInBytes =
1*Instr::kInstrSize;
On 2015/02/25 22:58:39, siva wrote:
> Ditto comment about spaces.

Done.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:114: static const int kLengthInBytes =
1*Instr::kInstrSize;
On 2015/02/25 22:58:39, siva wrote:
> Ditto comment about spaces.

Done.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:114: static const int kLengthInBytes =
1*Instr::kInstrSize;
On 2015/02/25 22:58:39, siva wrote:
> Ditto comment about spaces.

Done.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:116: int pattern_length_in_bytes() const {
On 2015/02/25 22:58:39, siva wrote:
> Ditto comment about name.

Acknowledged.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:116: int pattern_length_in_bytes() const {
On 2015/02/25 22:58:39, siva wrote:
> Ditto comment about name.

Acknowledged.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_arm64.h:116: int pattern_length_in_bytes() const {
On 2015/02/25 22:58:39, siva wrote:
> Ditto comment about name.

Acknowledged.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
File runtime/vm/instructions_ia32.h (right):

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/instructions...
runtime/vm/instructions_ia32.h:103: virtual int pattern_length_in_bytes() const
{ return kLengthInBytes; }
On 2015/02/25 22:58:39, siva wrote:
> I guess this name is coming from some existing code, not sure why it was done
> like that ignore my comment on the name in other files.

Acknowledged.

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

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1457: deoptimized_code_array_ = value.raw();
On 2015/02/25 22:58:39, siva wrote:
> StorePointer(...);
> 
> I think we are missing this with some of the tag stuff too (e.g tag_table_,
> current_tag_, default_tag_)

Acknowledged.

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/isolate.cc#n...
runtime/vm/isolate.cc:1469: deoptimized_code.Add(code);
On 2015/02/25 22:58:39, siva wrote:
> Can you add a TODO here that we will make this and the profiler samples GC
aware
> structures so that it can free up code objects which are not referenced in the
> profile.

Done.

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

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/object.cc#ne...
runtime/vm/object.cc:12583: Smi& temp_smi = Smi::Handle();
On 2015/02/25 22:58:39, siva wrote:
> This handle can be hoisted out of the for loop?

Done.

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

https://codereview.chromium.org/928833003/diff/160001/runtime/vm/profiler.cc#...
runtime/vm/profiler.cc:467: #endif
On 2015/02/25 22:58:39, siva wrote:
> I am wondering if this target specific stuff should go into stack_frame_xxx.h

Long term this code will need to move, but, I wonder if might make more sense to
factor it into instructions_xxx.h (which has other pattern matchers).

[Cutch, 2015-02-26 18:50:00]

Committed patchset #10 (id:180001) manually as 44067.