[turbofan] Fix source position integration with Linux perf

[turbofan] Fix source position integration with Linux perf

Committed: https://crrev.com/94fb89040b24e336337f47012e435fe9c2ad8bcf
Cr-Commit-Position: refs/heads/master@{#41637}

[danno, 2016-12-09 11:04:29]

danno@chromium.org changed reviewers:
+ jarin@chromium.org, tebbi@chromium.org, yangguo@chromium.org

[danno, 2016-12-09 11:04:29]

PTAL

[Tobias Tebbi, 2016-12-09 13:01:06]

On 2016/12/09 11:04:29, danno wrote:
> PTAL

As addressed in Yang's CL (https://codereview.chromium.org/2562973002/),
PerfJitLogger::LogWriteDebugInfo must be unhandlified. Unfortunately,
SourcePosition::InliningStack might allocate line ends. Line ends should be
initialized when isolate->NeedsSourcePositionsForProfiling() is true, but this
is probably to fragile to rely on. At the moment, source-position.h only
provides an unhandlified method to print source positions for --trace-deopt. I
guess we have to add a more general unhandlified interface to inlining stacks.

[Yang, 2016-12-09 14:28:54]

On 2016/12/09 13:01:06, Tobias Tebbi wrote:
> On 2016/12/09 11:04:29, danno wrote:
> > PTAL
> 
> As addressed in Yang's CL (https://codereview.chromium.org/2562973002/),
> PerfJitLogger::LogWriteDebugInfo must be unhandlified. Unfortunately,
> SourcePosition::InliningStack might allocate line ends. Line ends should be
> initialized when isolate->NeedsSourcePositionsForProfiling() is true, but this
> is probably to fragile to rely on. At the moment, source-position.h only
> provides an unhandlified method to print source positions for --trace-deopt. I
> guess we have to add a more general unhandlified interface to inlining stacks.

Yes. I think my CL should fix the issue with line ends, since we would then
eagerly compute and allocate them for profiling.

[Yang, 2016-12-09 14:29:18]

On 2016/12/09 14:28:54, Yang wrote:
> On 2016/12/09 13:01:06, Tobias Tebbi wrote:
> > On 2016/12/09 11:04:29, danno wrote:
> > > PTAL
> > 
> > As addressed in Yang's CL (https://codereview.chromium.org/2562973002/),
> > PerfJitLogger::LogWriteDebugInfo must be unhandlified. Unfortunately,
> > SourcePosition::InliningStack might allocate line ends. Line ends should be
> > initialized when isolate->NeedsSourcePositionsForProfiling() is true, but
this
> > is probably to fragile to rely on. At the moment, source-position.h only
> > provides an unhandlified method to print source positions for --trace-deopt.
I
> > guess we have to add a more general unhandlified interface to inlining
stacks.
> 
> Yes. I think my CL should fix the issue with line ends, since we would then
> eagerly compute and allocate them for profiling.

And I think it should be fine to use InliningStack here and simply guard with a
DisallowHeapAllocation scope.

[Tobias Tebbi, 2016-12-09 14:57:51]

lgtm

https://codereview.chromium.org/2558283002/diff/40001/src/perf-jit.cc
File src/perf-jit.cc (right):

https://codereview.chromium.org/2558283002/diff/40001/src/perf-jit.cc#newcode274
src/perf-jit.cc:274: return SourcePositionInfo(pos, function);
This does not compute the line and column. Why not just always use
pos.InliningStack(code)[0]?

https://codereview.chromium.org/2558283002/diff/40001/src/perf-jit.cc#newcode300
src/perf-jit.cc:300: // Add the size of the name after the each entry.
typo: after each entry.

[danno, 2016-12-12 09:35:59]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 09:36:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2016-12-12 09:36:33]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 09:36:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 09:44:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 09:44:53]

Dry run: Try jobs failed on following builders:
  v8_linux64_gyp_rel_ng on master.tryserver.v8 (JOB_FAILED,
http://build.chromium.org/p/tryserver.v8/builders/v8_linux64_gyp_rel_ng/build...)

[danno, 2016-12-12 09:47:57]

The CQ bit was checked by danno@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-12 09:48:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[danno, 2016-12-12 09:53:57]

Feedback addressed, landing.

https://codereview.chromium.org/2558283002/diff/40001/src/perf-jit.cc
File src/perf-jit.cc (right):

https://codereview.chromium.org/2558283002/diff/40001/src/perf-jit.cc#newcode274
src/perf-jit.cc:274: return SourcePositionInfo(pos, function);
On 2016/12/09 14:57:51, Tobias Tebbi wrote:
> This does not compute the line and column. Why not just always use
> pos.InliningStack(code)[0]?

Good question. Full code gen uses the deopt input data vector slot in the code
object for something else rather than deopt info, so calling InliningStack
crashes on FGC code. Therefore, since there is no inlining, the source position
needs to be generated by hand.

https://codereview.chromium.org/2558283002/diff/40001/src/perf-jit.cc#newcode300
src/perf-jit.cc:300: // Add the size of the name after the each entry.
On 2016/12/09 14:57:51, Tobias Tebbi wrote:
> typo: after each entry.

Done.

[commit-bot: I haz the power, 2016-12-12 10:11:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-12 10:11:21]

Dry run: This issue passed the CQ dry run.

[danno, 2016-12-12 12:22:54]

The CQ bit was checked by danno@chromium.org

[danno, 2016-12-12 12:22:54]

The patchset sent to the CQ was uploaded after l-g-t-m from tebbi@chromium.org
Link to the patchset: https://codereview.chromium.org/2558283002/#ps140001
(title: "Remove dead code")

[commit-bot: I haz the power, 2016-12-12 12:22:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-12 12:24:53]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481545374313540,
"parent_rev": "3340097784cc5e4dd83596ad07369b584916ad5f", "commit_rev":
"4ffb7aaed9b6e07d4312500e89f1a746d30a43d3"}

[commit-bot: I haz the power, 2016-12-12 12:24:58]

Description was changed from

==========
[turbofan] Fix source position integration with Linux perf
==========

to

==========
[turbofan] Fix source position integration with Linux perf

Review-Url: https://codereview.chromium.org/2558283002
==========

[commit-bot: I haz the power, 2016-12-12 12:24:59]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-12 12:26:04]

Description was changed from

==========
[turbofan] Fix source position integration with Linux perf

Review-Url: https://codereview.chromium.org/2558283002
==========

to

==========
[turbofan] Fix source position integration with Linux perf

Committed: https://crrev.com/94fb89040b24e336337f47012e435fe9c2ad8bcf
Cr-Commit-Position: refs/heads/master@{#41637}
==========

[commit-bot: I haz the power, 2016-12-12 12:26:05]

Patchset 8 (id:??) landed as
https://crrev.com/94fb89040b24e336337f47012e435fe9c2ad8bcf
Cr-Commit-Position: refs/heads/master@{#41637}