Reland of Extend CPU profiler with mapping ticks to source lines

The idea behind of this solution is to use the existing "relocation info" instead of consumption the CodeLinePosition events emitted by the V8 compilers.
During generation code and relocation info are generated simultaneously.
When code generation is done you each code object has associated "relocation info".
Relocation information lets V8 to mark interesting places in the generated code: the pointers that might need to be relocated (after garbage collection),
correspondences between the machine program counter and source locations for stack walking.

This patch:
1. Add more source positions info in reloc info to make it suitable for source level mapping.
The amount of data should not be increased dramatically because (1) V8 already marks interesting places in the generated code and
(2) V8 does not write redundant information (it writes a pair (pc_offset, pos) only if pos is changed and skips other).
I measured it on Octane benchmark - for unoptimized code the number of source positions may achieve 2x ('lin_solve' from NavierStokes benchmark).

2. When a sample happens, CPU profiler finds a code object by pc, then use its reloc info to match the sample to a source line.
If a source line is found that hit counter is increased by one for this line.

3. Add a new public V8 API to get the hit source lines by CDT CPU profiler.
Note that it's expected a minor patch in Blink to pack the source level info in JSON to be shown.

4.Add a test that checks how the samples are distributed through source lines.
It tests two cases: (1) relocation info created during code generation and (2) relocation info associated with precompiled function's version.

Patch from Denis Pravdin <denis.pravdin@intel.com>;

R=svenpanne@chromium.org, yurys@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=25182

[Weiliang, 2014-10-29 05:09:14]

weiliang.lin@intel.com changed reviewers:
+ alph@chromium.org, danno@chromium.org, svenpanne@chromium.org,
yurys@chromium.org

[Weiliang, 2014-10-29 05:09:15]

Hi yurys and all

Could you please take a look this relanding patch?

The failure reason of layout test (fast/events/window-onerror-02.html) is due to
redundant/wrong position info. 
It is introduced by the change in full-codegen.cc as below
Index: src/full-codegen.cc
diff --git a/src/full-codegen.cc b/src/full-codegen.cc. We revert this change
and now all of the test pass (see the trybot result.)
index
35d51d982ed44f9edffc77ff8f249338fe42cca2..edb43ef7e898bf87964c1f1a4a3c531677581c75
100644
--- a/src/full-codegen.cc
+++ b/src/full-codegen.cc
@@ -866,6 +866,7 @@ void FullCodeGenerator::SetExpressionPosition(Expression*
expr) {
 void FullCodeGenerator::SetSourcePosition(int pos) {
   if (pos != RelocInfo::kNoPosition) {
     masm_->positions_recorder()->RecordPosition(pos);
+    masm_->positions_recorder()->WriteRecordedPositions();
   }
 }

PTAL and help landing

Thanks
-Weiliang

[yurys, 2014-10-29 14:52:08]

lgtm

[yurys, 2014-10-29 14:52:56]

On 2014/10/29 05:09:15, Weiliang wrote:
> Hi yurys and all
> 
> Could you please take a look this relanding patch?
> 
> The failure reason of layout test (fast/events/window-onerror-02.html) is due
to
> redundant/wrong position info. 
> It is introduced by the change in full-codegen.cc as below
> Index: src/full-codegen.cc
> diff --git a/src/full-codegen.cc b/src/full-codegen.cc. We revert this change
> and now all of the test pass (see the trybot result.)
> index
>
35d51d982ed44f9edffc77ff8f249338fe42cca2..edb43ef7e898bf87964c1f1a4a3c531677581c75
> 100644
> --- a/src/full-codegen.cc
> +++ b/src/full-codegen.cc
> @@ -866,6 +866,7 @@ void FullCodeGenerator::SetExpressionPosition(Expression*
> expr) {
>  void FullCodeGenerator::SetSourcePosition(int pos) {
>    if (pos != RelocInfo::kNoPosition) {
>      masm_->positions_recorder()->RecordPosition(pos);
> +    masm_->positions_recorder()->WriteRecordedPositions();
>    }
>  }
> 
> PTAL and help landing
> 
> Thanks
> -Weiliang

I wonder why WriteRecordedPositions was needed before?

[Weiliang, 2014-10-30 07:29:12]

On 2014/10/29 14:52:56, yurys wrote:
> On 2014/10/29 05:09:15, Weiliang wrote:
> > Hi yurys and all
> > 
> > Could you please take a look this relanding patch?
> > 
> > The failure reason of layout test (fast/events/window-onerror-02.html) is
due
> to
> > redundant/wrong position info. 
> > It is introduced by the change in full-codegen.cc as below
> > Index: src/full-codegen.cc
> > diff --git a/src/full-codegen.cc b/src/full-codegen.cc. We revert this
change
> > and now all of the test pass (see the trybot result.)
> > index
> >
>
35d51d982ed44f9edffc77ff8f249338fe42cca2..edb43ef7e898bf87964c1f1a4a3c531677581c75
> > 100644
> > --- a/src/full-codegen.cc
> > +++ b/src/full-codegen.cc
> > @@ -866,6 +866,7 @@ void
FullCodeGenerator::SetExpressionPosition(Expression*
> > expr) {
> >  void FullCodeGenerator::SetSourcePosition(int pos) {
> >    if (pos != RelocInfo::kNoPosition) {
> >      masm_->positions_recorder()->RecordPosition(pos);
> > +    masm_->positions_recorder()->WriteRecordedPositions();
> >    }
> >  }
> > 
> > PTAL and help landing
> > 
> > Thanks
> > -Weiliang
> 
> I wonder why WriteRecordedPositions was needed before?

Thanks Yurys. The origin purpose was to emit more source position information
for the JIT code of full codegen. But it is wrong, so we revert it.

Thanks
-Weiliang

[yurys, 2014-10-30 08:35:55]

Sven, can you do OWNERS review please. It is the same CL as
https://codereview.chromium.org/616963005 but without aforementioned change in
src/full-codegen.cc.

[Weiliang, 2014-11-03 06:00:08]

On 2014/10/30 08:35:55, yurys wrote:
> Sven, can you do OWNERS review please. It is the same CL as
> https://codereview.chromium.org/616963005 but without aforementioned change in
> src/full-codegen.cc.

Rebased.

There are two exceptions for v8_linux_layout_dgb. 

But refer to
http://build.chromium.org/p/tryserver.v8/builders/v8_linux_layout_dbg. 

It is not caused by this patch. 

Thanks for review and help landing.

Thanks
-Weiliang

[yurys, 2014-11-04 17:24:35]

Still waiting for OWNERS review.

[Weiliang, 2014-11-05 02:36:39]

On 2014/11/04 17:24:35, yurys wrote:
> Still waiting for OWNERS review.

@danno, @seven

Could you please do a OWNERS review?

Thanks
-Weiliang

[Sven Panne, 2014-11-05 10:41:25]

LGTM with 2 nits.

https://codereview.chromium.org/682143003/diff/20001/src/profile-generator-inl.h
File src/profile-generator-inl.h (right):

https://codereview.chromium.org/682143003/diff/20001/src/profile-generator-in...
src/profile-generator-inl.h:42: static bool LineTickMatch(void* a, void* b) {
return a == b; }
Move this into the ProfileNode class as a private static function.

https://codereview.chromium.org/682143003/diff/20001/test/cctest/test-cpu-pro...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/682143003/diff/20001/test/cctest/test-cpu-pro...
test/cctest/test-cpu-profiler.cc:1111: SmartPointer<ProfilerEventsProcessor>
processor(new ProfilerEventsProcessor(
I don't think we need a SmartPointer here at all, just a plain
ProfileEventsProcessor.

[Weiliang, 2014-11-05 15:15:08]

Thanks for review. 

Please have a look at the patch set 4 and help landing.

https://codereview.chromium.org/682143003/diff/20001/src/profile-generator-inl.h
File src/profile-generator-inl.h (right):

https://codereview.chromium.org/682143003/diff/20001/src/profile-generator-in...
src/profile-generator-inl.h:42: static bool LineTickMatch(void* a, void* b) {
return a == b; }
On 2014/11/05 10:41:25, Sven Panne wrote:
> Move this into the ProfileNode class as a private static function.

Done.

https://codereview.chromium.org/682143003/diff/20001/test/cctest/test-cpu-pro...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/682143003/diff/20001/test/cctest/test-cpu-pro...
test/cctest/test-cpu-profiler.cc:1111: SmartPointer<ProfilerEventsProcessor>
processor(new ProfilerEventsProcessor(
On 2014/11/05 10:41:25, Sven Panne wrote:
> I don't think we need a SmartPointer here at all, just a plain
> ProfileEventsProcessor.

Done.

[Weiliang, 2014-11-06 07:16:54]

On 2014/11/05 15:15:08, Weiliang wrote:
> Thanks for review. 
> 
> Please have a look at the patch set 4 and help landing.
> 
>
https://codereview.chromium.org/682143003/diff/20001/src/profile-generator-inl.h
> File src/profile-generator-inl.h (right):
> 
>
https://codereview.chromium.org/682143003/diff/20001/src/profile-generator-in...
> src/profile-generator-inl.h:42: static bool LineTickMatch(void* a, void* b) {
> return a == b; }
> On 2014/11/05 10:41:25, Sven Panne wrote:
> > Move this into the ProfileNode class as a private static function.
> 
> Done.
> 
>
https://codereview.chromium.org/682143003/diff/20001/test/cctest/test-cpu-pro...
> File test/cctest/test-cpu-profiler.cc (right):
> 
>
https://codereview.chromium.org/682143003/diff/20001/test/cctest/test-cpu-pro...
> test/cctest/test-cpu-profiler.cc:1111: SmartPointer<ProfilerEventsProcessor>
> processor(new ProfilerEventsProcessor(
> On 2014/11/05 10:41:25, Sven Panne wrote:
> > I don't think we need a SmartPointer here at all, just a plain
> > ProfileEventsProcessor.
> 
> Done.

Please help landing. 

The failure of v8_linux_nosnap_dbg is caused by other patch and fixed in this
entry (https://codereview.chromium.org/702893002).

Thanks
-Weiliang

[Sven Panne, 2014-11-06 07:51:41]

NOT LGTM

https://codereview.chromium.org/682143003/diff/60001/test/cctest/test-cpu-pro...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/682143003/diff/60001/test/cctest/test-cpu-pro...
test/cctest/test-cpu-profiler.cc:1111: ProfilerEventsProcessor* processor = new
ProfilerEventsProcessor(
Now we've got a space leak...

[Weiliang, 2014-11-06 08:27:06]

On 2014/11/06 07:51:41, Sven Panne wrote:
> NOT LGTM
> 
>
https://codereview.chromium.org/682143003/diff/60001/test/cctest/test-cpu-pro...
> File test/cctest/test-cpu-profiler.cc (right):
> 
>
https://codereview.chromium.org/682143003/diff/60001/test/cctest/test-cpu-pro...
> test/cctest/test-cpu-profiler.cc:1111: ProfilerEventsProcessor* processor =
new
> ProfilerEventsProcessor(
> Now we've got a space leak...

Hi Sven,

Actually I don't quite understand what your old comment mean here. 

In my understanding, ProfilerEventsProcessor pointer is passed to create
CpuProfiler and CpuProfiler may delete this pointer
(CpuProfiler::StopProcessor()). So a normal way is to allocate a smart pointer
for ProfilerEventsProcessor.

Thanks
-Weiliang

[Sven Panne, 2014-11-06 09:14:16]

On 2014/11/06 08:27:06, Weiliang wrote:
> Actually I don't quite understand what your old comment mean here. 
> 
> In my understanding, ProfilerEventsProcessor pointer is passed to create
> CpuProfiler and CpuProfiler may delete this pointer
> (CpuProfiler::StopProcessor()). So a normal way is to allocate a smart pointer
> for ProfilerEventsProcessor.

Uh, oh... :-P I've just recognized that CpuProfiler is taking ownership of its
arguments (apart from Isolate*), but of course this can't be seen from the naked
pointers, and there's no comment indicating this. In a perfect C++11 world, we
would use unique_ptr for those arguments, but we can' use that (yet). As it is,
there is (probably :-) no space leak in your last patch set then, and I'll land
it. Nevertheless, the API is horrible...

Regarding SmartPointer: This would have been wrong, because the CpuProfiler
would have deleted the ProfilerEventsProcessor, and then the destructor of
SmartPointer would have deleted it again.

[Sven Panne, 2014-11-06 09:16:45]

Committed patchset #4 (id:60001) manually as 25182 (presubmit successful).