CPUProfiler: Push deopt reason further to ProfileNode.

CPUProfiler: Push deopt reason further to ProfileNode.

1) create beefy RelocInfo table when cpu profiler is active, so if a function
was optimized when profiler was active RelocInfo would get separate DeoptInfo
for the each deopt case.

2) push DeoptInfo from CodeEntry to ProfileNode.
When deopt happens we put the info collected on #1 into CodeEntry and record stack sample.
On the sampling thread we grab the deopt data and append it to the corresponding ProfileNode deopts list.

Sample profile dump.
[Top down]:
    0  (root) 0 #1
    1     29 #2
    1      test 29 #3
    2        opt_function 29 #4
    2          opt_function 29 #5
                   deopted at 118 with reason 'not a heap number'
                   deopted at 137 with reason 'division by zero'

BUG=452067
LOG=n

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

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

[loislo, 2015-02-12 07:32:11]

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

[loislo, 2015-02-12 07:32:12]

PTAL

[Sven Panne, 2015-02-12 08:42:31]

https://codereview.chromium.org/919953002/diff/1/src/deoptimizer.h
File src/deoptimizer.h (left):

https://codereview.chromium.org/919953002/diff/1/src/deoptimizer.h#oldcode218
src/deoptimizer.h:218: (!FLAG_trace_deopt || deopt_info == other.deopt_info);
Why has this been removed? In earlier versions we needed this to distinguish
deopt reasons for the same ID, which would otherwise be merged. How is this
handled now?

[loislo, 2015-02-12 08:54:05]

https://codereview.chromium.org/919953002/diff/1/src/deoptimizer.h
File src/deoptimizer.h (left):

https://codereview.chromium.org/919953002/diff/1/src/deoptimizer.h#oldcode218
src/deoptimizer.h:218: (!FLAG_trace_deopt || deopt_info == other.deopt_info);
On 2015/02/12 08:42:31, Sven Panne wrote:
> Why has this been removed? In earlier versions we needed this to distinguish
> deopt reasons for the same ID, which would otherwise be merged. How is this
> handled now?

I need to push different entries if FLAG_trace_deopt is True or cpu profiler is
active. For the second check I need a pointer to isolate. Also such kind of
checks looks artificial here so I decided to move it to the callers. So I do
this check in lithium-codegen-*.cc. From the other hand if we have trace_deopt
or profiler flags enabled then we don't need to compare deopt_info in assumption
that we never push deopt_info with the same reason, bailout_id and location
twice in a row.

[Sven Panne, 2015-02-12 09:12:03]

LGTM from my side, but somebody else should have a look at the src/profile-*
part's, I can't really comment on those.

[yurys, 2015-02-12 13:00:30]

lgtm

[loislo, 2015-02-12 13:10:14]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2015-02-12 13:10:50]

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

[alph, 2015-02-12 13:19:46]

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

https://codereview.chromium.org/919953002/diff/1/src/profile-generator.cc#new...
src/profile-generator.cc:242: if (entry->has_deopt_info())
node->CollectDeoptInfo(entry);
how to you ensure the deopt info goes into the right node? There may be a number
of nodes in profile tree that correspond to a single code entry.

https://codereview.chromium.org/919953002/diff/1/src/x87/assembler-x87.cc
File src/x87/assembler-x87.cc (right):

https://codereview.chromium.org/919953002/diff/1/src/x87/assembler-x87.cc#new...
src/x87/assembler-x87.cc:1922: void Assembler::RecordDeoptReason(const int
reason, const int raw_position) {
All these functions seems to be equal. I wonder if you can move them into a base
assembler.cc ?

https://codereview.chromium.org/919953002/diff/1/test/cctest/test-cpu-profile...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/919953002/diff/1/test/cctest/test-cpu-profile...
test/cctest/test-cpu-profiler.cc:1773: CHECK_EQ(2,
iopt_function->deopt_infos().length());
can you also check the reasons and positions please?

[commit-bot: I haz the power, 2015-02-12 13:25:05]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-02-12 13:25:36]

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

[loislo, 2015-02-12 14:35:28]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/915173005/ by loislo@chromium.org.

The reason for reverting is: static initializers broke the build.

[loislo, 2015-02-12 15:20:01]

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

https://codereview.chromium.org/919953002/diff/1/src/profile-generator.cc#new...
src/profile-generator.cc:242: if (entry->has_deopt_info())
node->CollectDeoptInfo(entry);
On 2015/02/12 13:19:46, alph wrote:
> how to you ensure the deopt info goes into the right node? There may be a
number
> of nodes in profile tree that correspond to a single code entry.

Done.

https://codereview.chromium.org/919953002/diff/1/src/x87/assembler-x87.cc
File src/x87/assembler-x87.cc (right):

https://codereview.chromium.org/919953002/diff/1/src/x87/assembler-x87.cc#new...
src/x87/assembler-x87.cc:1922: void Assembler::RecordDeoptReason(const int
reason, const int raw_position) {
On 2015/02/12 13:19:46, alph wrote:
> All these functions seems to be equal. I wonder if you can move them into a
base
> assembler.cc ?

I'll do that in another patch

https://codereview.chromium.org/919953002/diff/1/test/cctest/test-cpu-profile...
File test/cctest/test-cpu-profiler.cc (right):

https://codereview.chromium.org/919953002/diff/1/test/cctest/test-cpu-profile...
test/cctest/test-cpu-profiler.cc:1773: CHECK_EQ(2,
iopt_function->deopt_infos().length());
On 2015/02/12 13:19:46, alph wrote:
> can you also check the reasons and positions please?

Done.

[loislo, 2015-02-12 15:20:59]

New patchsets have been uploaded after l-g-t-m from
svenpanne@chromium.org,yurys@chromium.org

[alph, 2015-02-12 15:49:43]

lgtm

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

https://codereview.chromium.org/919953002/diff/1/src/profile-generator.cc#new...
src/profile-generator.cc:242: if (entry->has_deopt_info())
node->CollectDeoptInfo(entry);
On 2015/02/12 15:20:01, loislo wrote:
> On 2015/02/12 13:19:46, alph wrote:
> > how to you ensure the deopt info goes into the right node? There may be a
> number
> > of nodes in profile tree that correspond to a single code entry.
> 
> Done.

Cool! thanks

[loislo, 2015-02-12 19:50:11]

The CQ bit was checked by loislo@chromium.org

[commit-bot: I haz the power, 2015-02-12 19:51:13]

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

[commit-bot: I haz the power, 2015-02-12 19:51:35]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-12 19:51:48]

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