CpuProfiler: push the collected information about deopts to cpu profiler

CpuProfiler: push the collected information about deopts to cpu profiler

it is the last patch of https://codereview.chromium.org/1012633002

All that we need here is to push the collected info to the profiler
and convert it into actionable information about deopt.

On the Next: get the info accessible by embedder.

BUG=chromium:452067
LOG=n
TEST=DeoptAtFirstLevelInlinedSource, DeoptAtSecondLevelInlinedSource, DeoptUntrackedFunction

Committed: https://crrev.com/ae461b9ed0bbe51f4681902f2c3ad1bd16432986
Cr-Commit-Position: refs/heads/master@{#27403}

[loislo, 2015-03-18 11:57:50]

loislo@chromium.org changed reviewers:
+ alph@chromium.org, yurys@chromium.org

[loislo, 2015-03-18 13:05:37]

ptal

[alph, 2015-03-18 18:15:19]

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.c...
src/profile-generator.cc:123: info.AddInlineFrame(script_id_, position_ +
deopt_position_.position());
Looks strange. Why do you want to add an inline frame if there's no
inlined_functions_info?

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.c...
src/profile-generator.cc:226: indent + 10, "", 4, "", info.stack[i].script_id,
4, ""
why?

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.h
File src/profile-generator.h (right):

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.h...
src/profile-generator.h:40: struct DeoptInfo {
turn it into a class?

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.h...
src/profile-generator.h:90: DeoptInfo GetDeoptInfo();
seems to be quite a big object to return by value

[loislo, 2015-03-19 08:16:12]

comments addressed

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.c...
src/profile-generator.cc:123: info.AddInlineFrame(script_id_, position_ +
deopt_position_.position());
On 2015/03/18 at 18:15:18, alph wrote:
> Looks strange. Why do you want to add an inline frame if there's no
inlined_functions_info?

For example in case of a debug breakpoint you will see a stack where
each entry is a call-point and the top entry is the current position in
the top level function.

The same happens here. The top level frame points to the deopt point,
all other frames point to the inlined calls. So if you have no inlined
functions then you will see the only frame in the stack which points you
to the deopt location.

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.c...
src/profile-generator.cc:226: indent + 10, "", 4, "", info.stack[i].script_id,
On 2015/03/18 at 18:15:18, alph wrote:
> 4, ""
> why?

it is an indent for inlined function stack entries. See sample. 

      1      test 30 #3
                ;;; deopted at script_id: 29 position: 45 with reason 'not a
heap number'.
                ;;;     Inline point: script_id 30 position: 36.

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.h
File src/profile-generator.h (right):

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.h...
src/profile-generator.h:40: struct DeoptInfo {
On 2015/03/18 at 18:15:19, alph wrote:
> turn it into a class?

done

https://codereview.chromium.org/1013143003/diff/30001/src/profile-generator.h...
src/profile-generator.h:90: DeoptInfo GetDeoptInfo();
On 2015/03/18 at 18:15:19, alph wrote:
> seems to be quite a big object to return by value

done

[loislo, 2015-03-19 15:01:15]

loislo@chromium.org changed reviewers:
+ svenpanne@chromium.org

[loislo, 2015-03-19 15:01:16]

+svenpanne@

[loislo, 2015-03-19 15:01:31]

On 2015/03/19 at 15:01:16, loislo wrote:
> +svenpanne@

the patch needs a rebaseline

[loislo, 2015-03-19 15:23:07]

On 2015/03/19 at 15:01:31, loislo wrote:
> On 2015/03/19 at 15:01:16, loislo wrote:
> > +svenpanne@
> 
> the patch needs a rebaseline

rebaselined. PTAL

[alph, 2015-03-20 08:28:43]

lgtm

https://codereview.chromium.org/1013143003/diff/130001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1013143003/diff/130001/src/compiler.h#newcode100
src/compiler.h:100: std::vector<size_t> deopt_pc_offsets;
Why's the change? AFAICT v8 tends to use 32-bit types for storage whenever
possible to reduce memory footprint.

https://codereview.chromium.org/1013143003/diff/130001/src/compiler.h#newcode468
src/compiler.h:468: std::vector<InlinedFunctionInfo>* inlined_function_infos_;
why not using a smart pointer?

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator....
src/profile-generator.cc:232: for (size_t i = 1; i < info->stack.size(); ++i) {
could you please use a distinct variable for inner loop iterator.

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator.h
File src/profile-generator.h (right):

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator....
src/profile-generator.h:151: std::vector<InlinedFunctionInfo>*
inlined_function_infos_;
ditto

https://codereview.chromium.org/1013143003/diff/130001/test/cctest/test-cpu-p...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/1013143003/diff/130001/test/cctest/test-cpu-p...
test/cctest/test-cpu-profiler.cc:1859: //    
0.........1.........2.........3.........4.........5.........6.........7
nit: shifted a bit

https://codereview.chromium.org/1013143003/diff/130001/test/cctest/test-cpu-p...
test/cctest/test-cpu-profiler.cc:1927: //    
0.........1.........2.........3.........4.........5.........6.........7
ditto

[loislo, 2015-03-20 08:53:12]

comments addressed

https://codereview.chromium.org/1013143003/diff/130001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1013143003/diff/130001/src/compiler.h#newcode468
src/compiler.h:468: std::vector<InlinedFunctionInfo>* inlined_function_infos_;
On 2015/03/20 at 08:28:42, alph wrote:
> why not using a smart pointer?

done

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator.cc
File src/profile-generator.cc (right):

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator....
src/profile-generator.cc:232: for (size_t i = 1; i < info->stack.size(); ++i) {
On 2015/03/20 at 08:28:42, alph wrote:
> could you please use a distinct variable for inner loop iterator.

done

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator.h
File src/profile-generator.h (right):

https://codereview.chromium.org/1013143003/diff/130001/src/profile-generator....
src/profile-generator.h:151: std::vector<InlinedFunctionInfo>*
inlined_function_infos_;
On 2015/03/20 at 08:28:43, alph wrote:
> ditto

done

https://codereview.chromium.org/1013143003/diff/130001/test/cctest/test-cpu-p...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/1013143003/diff/130001/test/cctest/test-cpu-p...
test/cctest/test-cpu-profiler.cc:1859: //    
0.........1.........2.........3.........4.........5.........6.........7
On 2015/03/20 at 08:28:43, alph wrote:
> nit: shifted a bit

done

https://codereview.chromium.org/1013143003/diff/130001/test/cctest/test-cpu-p...
test/cctest/test-cpu-profiler.cc:1927: //    
0.........1.........2.........3.........4.........5.........6.........7
On 2015/03/20 at 08:28:43, alph wrote:
> ditto

done

[loislo, 2015-03-20 11:34:23]

loislo@chromium.org changed reviewers:
+ yangguo@chromium.org

[Sven Panne, 2015-03-23 10:13:27]

I'm a bit unhappy about the complicated ownership/lifetime handling in this CL.
It looks like premature optimization in an already horribly complicated and
bloated class (CompilationInfo). Unless we have hard measured numbers to justify
this, we should keep things as simple as possible, because heavy refactorings in
this area are coming our way.

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc
File src/compiler.cc (right):

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
src/compiler.cc:134: no_frame_ranges_.Reset(new List<OffsetRange>(2));
No need for this, see comment in the header.

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
src/compiler.cc:135: track_positions_ = true;
Instead of doing this assignment to track_positions, just do it in the
initializer list.

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
src/compiler.cc:138: inlined_function_infos_.Reset(new
std::vector<InlinedFunctionInfo>());
No need for this, see comment in the header.

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h#newcode352
src/compiler.h:352: return inlined_function_infos_.Detach();
This is horribly complicated: Can't we simply return the inlined_function_infos_
by value? How many entries are there typically in such a vector? AFAICT, there
should be only a handful.

The same holds for the Detach above, too.

https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h#newcode465
src/compiler.h:465: SmartPointer<std::vector<InlinedFunctionInfo>>
inlined_function_infos_;
Following the KISS principle, leave the instance fields just like they were
without any explicit dynamic allocation/SmartPointers/etc.  Don't nano-optimize
the CompilationInfo memory consumption when we burn tons of MBs later in the
compilation pipelines, anyway.

[loislo, 2015-03-23 13:01:47]

On 2015/03/23 at 10:13:27, svenpanne wrote:
> I'm a bit unhappy about the complicated ownership/lifetime handling in this
CL. It looks like premature optimization in an already horribly complicated and
bloated class (CompilationInfo). Unless we have hard measured numbers to justify
this, we should keep things as simple as possible, because heavy refactorings in
this area are coming our way.
> 
> https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc
> File src/compiler.cc (right):
> 
>
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
> src/compiler.cc:134: no_frame_ranges_.Reset(new List<OffsetRange>(2));
> No need for this, see comment in the header.
> 
>
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
> src/compiler.cc:135: track_positions_ = true;
> Instead of doing this assignment to track_positions, just do it in the
initializer list.
> 
>
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
> src/compiler.cc:138: inlined_function_infos_.Reset(new
std::vector<InlinedFunctionInfo>());
> No need for this, see comment in the header.
> 
> https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h
> File src/compiler.h (right):
> 
>
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h#newcode352
> src/compiler.h:352: return inlined_function_infos_.Detach();
> This is horribly complicated: Can't we simply return the
inlined_function_infos_ by value? How many entries are there typically in such a
vector? AFAICT, there should be only a handful.
> 
> The same holds for the Detach above, too.
> 
>
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h#newcode465
> src/compiler.h:465: SmartPointer<std::vector<InlinedFunctionInfo>>
inlined_function_infos_;
> Following the KISS principle, leave the instance fields just like they were
without any explicit dynamic allocation/SmartPointers/etc.  Don't nano-optimize
the CompilationInfo memory consumption when we burn tons of MBs later in the
compilation pipelines, anyway.

comments addressed

[loislo, 2015-03-24 10:05:40]

On 2015/03/23 at 13:01:47, loislo wrote:
> On 2015/03/23 at 10:13:27, svenpanne wrote:
> > I'm a bit unhappy about the complicated ownership/lifetime handling in this
CL. It looks like premature optimization in an already horribly complicated and
bloated class (CompilationInfo). Unless we have hard measured numbers to justify
this, we should keep things as simple as possible, because heavy refactorings in
this area are coming our way.
> > 
> > https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc
> > File src/compiler.cc (right):
> > 
> >
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
> > src/compiler.cc:134: no_frame_ranges_.Reset(new List<OffsetRange>(2));
> > No need for this, see comment in the header.
> > 
> >
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
> > src/compiler.cc:135: track_positions_ = true;
> > Instead of doing this assignment to track_positions, just do it in the
initializer list.
> > 
> >
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.cc#newcod...
> > src/compiler.cc:138: inlined_function_infos_.Reset(new
std::vector<InlinedFunctionInfo>());
> > No need for this, see comment in the header.
> > 
> > https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h
> > File src/compiler.h (right):
> > 
> >
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h#newcode352
> > src/compiler.h:352: return inlined_function_infos_.Detach();
> > This is horribly complicated: Can't we simply return the
inlined_function_infos_ by value? How many entries are there typically in such a
vector? AFAICT, there should be only a handful.
> > 
> > The same holds for the Detach above, too.
> > 
> >
https://codereview.chromium.org/1013143003/diff/150001/src/compiler.h#newcode465
> > src/compiler.h:465: SmartPointer<std::vector<InlinedFunctionInfo>>
inlined_function_infos_;
> > Following the KISS principle, leave the instance fields just like they were
without any explicit dynamic allocation/SmartPointers/etc.  Don't nano-optimize
the CompilationInfo memory consumption when we burn tons of MBs later in the
compilation pipelines, anyway.
> 
> comments addressed

ping

[Sven Panne, 2015-03-24 10:25:30]

https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h#newcode352
src/compiler.h:352: const std::vector<InlinedFunctionInfo>&
inlined_function_infos() {
Hmmm, returning this by reference is not nice: Looking at the use sites of this
function, I fail to see why it is even correct, i.e. why the CompilationInfo is
still alive at that point. Even if it *was* correct, let's not do this,
reasoning globally about tricky lifetimes is a recipe for bugs later: Just
return this by value, there should only be a tiny handful of entries in it
(AFAICT).

[loislo, 2015-03-24 10:46:34]

https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h#newcode352
src/compiler.h:352: const std::vector<InlinedFunctionInfo>&
inlined_function_infos() {
On 2015/03/24 at 10:25:30, Sven Panne wrote:
> Hmmm, returning this by reference is not nice: Looking at the use sites of
this function, I fail to see why it is even correct, i.e. why the
CompilationInfo is still alive at that point. Even if it *was* correct, let's
not do this, reasoning globally about tricky lifetimes is a recipe for bugs
later: Just return this by value, there should only be a tiny handful of entries
in it (AFAICT).

1) We pass CompilationInfo to logger when we finished compilation, so it is
alive. 

2) Profiler copies the inlining info into CodeCreateEvent (it used to take the
ownership in the previous versions of the patch).

3) event processor processes all the events (CodeCreate, CodeDeopt, Sample with
stacktrace, etc) and copies the inlining info into CodeEntry objects when it
processes CodeCreateEvents. (it used to take the ownership in the previous
versions of the patch). JFYI: CodeEntry maps to Code object one to one.

4) event processor resolves a stack of addresses (stacktrace) into a stack of
CodeEntries.

5) event processor appends the stack of CodeEntries to the profiler tree,
construct new ProfileNodes if necessary, creates DeoptInfo with help of
pc_offset and the InliningInfo and puts it into top-level ProfileNode. We always
have a stack which finishes in the deopted code because we explicitly create
StackTrace in order of logging the deopt event.

[Sven Panne, 2015-03-24 10:56:55]

https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h
File src/compiler.h (right):

https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h#newcode352
src/compiler.h:352: const std::vector<InlinedFunctionInfo>&
inlined_function_infos() {
On 2015/03/24 10:46:34, loislo wrote:
> 1) We pass CompilationInfo to logger when we finished compilation, so it is
> alive. 
> 
> 2) Profiler copies the inlining info into CodeCreateEvent (it used to take the
> ownership in the previous versions of the patch).

To be exact: It doesn't copy the inlining info at all, it just puts a reference
to CompilationInfo's internal field into the event (i.e. basically copying a
pointer).

> 3) event processor processes all the events (CodeCreate, CodeDeopt, Sample
with
> stacktrace, etc) and copies the inlining info into CodeEntry objects when it
> processes CodeCreateEvents. (it used to take the ownership in the previous
> versions of the patch). JFYI: CodeEntry maps to Code object one to one.

Hopefully the CompilationInfo is still alive here. :-)

> 4) event processor resolves a stack of addresses (stacktrace) into a stack of
> CodeEntries.
> 
> 5) event processor appends the stack of CodeEntries to the profiler tree,
> construct new ProfileNodes if necessary, creates DeoptInfo with help of
> pc_offset and the InliningInfo and puts it into top-level ProfileNode. We
always
> have a stack which finishes in the deopted code because we explicitly create
> StackTrace in order of logging the deopt event.

I still don't like this, but I don't want to hold up your progress and
CompilationInfo needs to be refactored heavily, anyway (e.g. the current part
we're talking about is totally Crankshaft-specific), so let's land this, but at
least put your reasoning as a comment here if you still want to keep using the
reference.

[loislo, 2015-03-24 12:09:18]

On 2015/03/24 at 10:56:55, svenpanne wrote:
> https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h
> File src/compiler.h (right):
> 
>
https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h#newcode352
> src/compiler.h:352: const std::vector<InlinedFunctionInfo>&
inlined_function_infos() {
> On 2015/03/24 10:46:34, loislo wrote:
> > 1) We pass CompilationInfo to logger when we finished compilation, so it is
> > alive. 
> > 
> > 2) Profiler copies the inlining info into CodeCreateEvent (it used to take
the
> > ownership in the previous versions of the patch).
> 
> To be exact: It doesn't copy the inlining info at all, it just puts a
reference to CompilationInfo's internal field into the event (i.e. basically
copying a pointer).

We takes a const ref to vector<InliningInfo> and pass it to
set_inlining_function_infos method.
In this method we do the copy of the content of the infos argument into
CodeEvent::inlined_function_infos_

See http://en.cppreference.com/w/cpp/container/vector/operator%3D

> 
> > 3) event processor processes all the events (CodeCreate, CodeDeopt, Sample
with
> > stacktrace, etc) and copies the inlining info into CodeEntry objects when it
> > processes CodeCreateEvents. (it used to take the ownership in the previous
> > versions of the patch). JFYI: CodeEntry maps to Code object one to one.
> 
> Hopefully the CompilationInfo is still alive here. :-)
> 
> > 4) event processor resolves a stack of addresses (stacktrace) into a stack
of
> > CodeEntries.
> > 
> > 5) event processor appends the stack of CodeEntries to the profiler tree,
> > construct new ProfileNodes if necessary, creates DeoptInfo with help of
> > pc_offset and the InliningInfo and puts it into top-level ProfileNode. We
always
> > have a stack which finishes in the deopted code because we explicitly create
> > StackTrace in order of logging the deopt event.
> 
> I still don't like this, but I don't want to hold up your progress and
CompilationInfo needs to be refactored heavily, anyway (e.g. the current part
we're talking about is totally Crankshaft-specific), so let's land this, but at
least put your reasoning as a comment here if you still want to keep using the
reference.

[loislo, 2015-03-24 12:22:41]

On 2015/03/24 at 12:09:18, loislo wrote:
> On 2015/03/24 at 10:56:55, svenpanne wrote:
> > https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h
> > File src/compiler.h (right):
> > 
> >
https://codereview.chromium.org/1013143003/diff/190001/src/compiler.h#newcode352
> > src/compiler.h:352: const std::vector<InlinedFunctionInfo>&
inlined_function_infos() {
> > On 2015/03/24 10:46:34, loislo wrote:
> > > 1) We pass CompilationInfo to logger when we finished compilation, so it
is
> > > alive. 
> > > 
> > > 2) Profiler copies the inlining info into CodeCreateEvent (it used to take
the
> > > ownership in the previous versions of the patch).
> > 
> > To be exact: It doesn't copy the inlining info at all, it just puts a
reference to CompilationInfo's internal field into the event (i.e. basically
copying a pointer).
> 
> We takes a const ref to vector<InliningInfo> and pass it to
set_inlining_function_infos method.
> In this method we do the copy of the content of the infos argument into
CodeEvent::inlined_function_infos_
> 
> See http://en.cppreference.com/w/cpp/container/vector/operator%3D
> 
> > 
> > > 3) event processor processes all the events (CodeCreate, CodeDeopt, Sample
with
> > > stacktrace, etc) and copies the inlining info into CodeEntry objects when
it
> > > processes CodeCreateEvents. (it used to take the ownership in the previous
> > > versions of the patch). JFYI: CodeEntry maps to Code object one to one.
> > 
> > Hopefully the CompilationInfo is still alive here. :-)
> > 
> > > 4) event processor resolves a stack of addresses (stacktrace) into a stack
of
> > > CodeEntries.
> > > 
> > > 5) event processor appends the stack of CodeEntries to the profiler tree,
> > > construct new ProfileNodes if necessary, creates DeoptInfo with help of
> > > pc_offset and the InliningInfo and puts it into top-level ProfileNode. We
always
> > > have a stack which finishes in the deopted code because we explicitly
create
> > > StackTrace in order of logging the deopt event.
> > 
> > I still don't like this, but I don't want to hold up your progress and
CompilationInfo needs to be refactored heavily, anyway (e.g. the current part
we're talking about is totally Crankshaft-specific), so let's land this, but at
least put your reasoning as a comment here if you still want to keep using the
reference.


Actually turbofan part of code is a terra incognita for me, but I hope I'd be
able to get the inlining info from it. :)

[Sven Panne, 2015-03-24 12:44:26]

On 2015/03/24 12:09:18, loislo wrote:
> [...]and pass it to set_inlining_function_infos method.
> In this method we do the copy of the content of the infos argument [...]

Ooops, I missed that copy. :-}

[Sven Panne, 2015-03-24 12:44:34]

The CQ bit was checked by svenpanne@chromium.org

[Sven Panne, 2015-03-24 12:44:35]

lgtm

[Sven Panne, 2015-03-24 12:44:35]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/1013143003/#ps190001
(title: "comments addressed")

[commit-bot: I haz the power, 2015-03-24 12:44:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1013143003/190001

[commit-bot: I haz the power, 2015-03-24 12:46:17]

Committed patchset #11 (id:190001)

[commit-bot: I haz the power, 2015-03-24 12:46:32]

Patchset 11 (id:??) landed as
https://crrev.com/ae461b9ed0bbe51f4681902f2c3ad1bd16432986
Cr-Commit-Position: refs/heads/master@{#27403}