Add profile data to getSourceReport

Add profile data to getSourceReport

- Add new getSourceReport kind 'Profile'.
- Add "profileTicks" to source ranges that have profile data.
- "profileTicks" is organized as three arrays: positions, inclusive ticks, and exclusive ticks. These arrays are indexed together.
- Keep source positions in profile sorted by token position value.
- Misc improvements to debug printing in profiler.
- Add unit test for profile getSourceReport.

R=turnidge@google.com

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

[Cutch, 2016-03-11 00:57:35]

Description was changed from

==========
Add profile data to getSourceReport

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

to

==========
Add profile data to getSourceReport

- Add new getSourceReport kind 'Profile'.
- Add "profileTicks" to source ranges that have profile data.
- "profileTicks" is organized as three arrays: positions, inclusive ticks, and
exclusive ticks. These arrays are indexed together.
- Keep source positions in profile sorted by token position value.
- Misc improvements to debug printing in profiler.
- Add unit test for profile getSourceReport.
==========

[Cutch, 2016-03-11 00:57:35]

johnmccutchan@google.com changed reviewers:
+ turnidge@google.com

[siva, 2016-03-11 15:03:41]

asiva@google.com changed reviewers:
+ asiva@google.com

[siva, 2016-03-11 15:03:42]

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

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:2135: }
Is this accessor here and not in the header file to avoid some include issue?

[Cutch, 2016-03-11 17:16:20]

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

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_ser...
runtime/vm/profiler_service.cc:2135: }
On 2016/03/11 15:03:42, siva wrote:
> Is this accessor here and not in the header file to avoid some include issue?

Yes, ProfileFunctionTable is only defined inside the source file.

[turnidge, 2016-03-11 18:31:58]

Do you want to update service.md too?

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2318: sample->SetAt(i, pc_offsets[i] + slop);
So for the top frame on the stack, we put in an unadjusted pc, but for callers,
we add one to the pc to mark them somehow?  Could the profiler mask off the low
bit instead of subtracting so that we don't need to do this here?

Maybe rename slop to something else... slop implies we are trying to be loose
with something.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2321: sample->SetAt(i, 0);
0 is a sentinel for the SampleBuffer?  Maybe a named constant?

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2379: // Query the code object for main and
determine the PC some token positions.
"PC some" -> "PC at some"?

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2491: EXPECT_SUBSTRING("\"inclusiveTicks\":[1,2]",
js.ToCString());
Nice test.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/source_repor...
File runtime/vm/source_report.cc (right):

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/source_repor...
runtime/vm/source_report.cc:40: SampleFilter
samplesForIsolate(Isolate::Current(), -1, -1);
Is thread_ the current thread?  If so, can we use isolate() here instead of
Isolate::Current()?

[Cutch, 2016-03-11 20:20:00]

I don't plan on updating service.md yet and I've made the source report key
private for now (_Profile).

PTAL.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
File runtime/vm/profiler_test.cc (right):

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2318: sample->SetAt(i, pc_offsets[i] + slop);
On 2016/03/11 18:31:57, turnidge wrote:
> So for the top frame on the stack, we put in an unadjusted pc, but for
callers,
> we add one to the pc to mark them somehow?  Could the profiler mask off the
low
> bit instead of subtracting so that we don't need to do this here?
> 

When querying the inline tables for a Code object, we subtract 1 from the PC for
all frames below the executing frame. This is done because all PCs in the sample
aside from the executing PC are *return addresses* and so we need to subtract 1
to move the PC into the correct range.

> Maybe rename slop to something else... slop implies we are trying to be loose
> with something.

I've renamed it and added a bunch of documentation.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2321: sample->SetAt(i, 0);
On 2016/03/11 18:31:57, turnidge wrote:
> 0 is a sentinel for the SampleBuffer?  Maybe a named constant?

Done.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2379: // Query the code object for main and
determine the PC some token positions.
On 2016/03/11 18:31:57, turnidge wrote:
> "PC some" -> "PC at some"?

Done.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/profiler_tes...
runtime/vm/profiler_test.cc:2491: EXPECT_SUBSTRING("\"inclusiveTicks\":[1,2]",
js.ToCString());
On 2016/03/11 18:31:57, turnidge wrote:
> Nice test.

Acknowledged.

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/source_repor...
File runtime/vm/source_report.cc (right):

https://codereview.chromium.org/1779333004/diff/20001/runtime/vm/source_repor...
runtime/vm/source_report.cc:40: SampleFilter
samplesForIsolate(Isolate::Current(), -1, -1);
On 2016/03/11 18:31:57, turnidge wrote:
> Is thread_ the current thread?  If so, can we use isolate() here instead of
> Isolate::Current()?

Done.

[turnidge, 2016-03-11 22:17:07]

lgtm

[Cutch, 2016-03-14 14:04:30]

Description was changed from

==========
Add profile data to getSourceReport

- Add new getSourceReport kind 'Profile'.
- Add "profileTicks" to source ranges that have profile data.
- "profileTicks" is organized as three arrays: positions, inclusive ticks, and
exclusive ticks. These arrays are indexed together.
- Keep source positions in profile sorted by token position value.
- Misc improvements to debug printing in profiler.
- Add unit test for profile getSourceReport.
==========

to

==========
Add profile data to getSourceReport

- Add new getSourceReport kind 'Profile'.
- Add "profileTicks" to source ranges that have profile data.
- "profileTicks" is organized as three arrays: positions, inclusive ticks, and
exclusive ticks. These arrays are indexed together.
- Keep source positions in profile sorted by token position value.
- Misc improvements to debug printing in profiler.
- Add unit test for profile getSourceReport.

R=turnidge@google.com

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

[Cutch, 2016-03-14 14:04:31]

Committed patchset #3 (id:40001) manually as
ffbd0b763ae012530660705963ef4c8e936fd95d (presubmit successful).