Produce more useful stack traces for heap profiling.

Produce more useful stack traces for heap profiling.

Previously, when the stack was too large, we'd keep the top part of the stack
[closer to main()]. This proves to be less useful than the the bottom part of
the stack. This CL changes the logic to keep the bottom part of the stack.

BUG=

Review-Url: https://codereview.chromium.org/2892273003
Cr-Commit-Position: refs/heads/master@{#473621}
Committed: https://chromium.googlesource.com/chromium/src/+/72f94d55762707bcd71caeacf6bf107718ed37f0

[erikchen, 2017-05-20 00:05:29]

Description was changed from

==========
Keep bottom-most frames instead of top-most farmes.

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

to

==========
Produce more useful stack traces for heap profiling.

Previously, when the stack was too large, we'd keep the top part of the stack
[closer to main()]. This proves to be less useful than the the bottom part of
the stack. This CL changes the logic to keep the bottom part of the stack.

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

[erikchen, 2017-05-20 00:05:44]

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

[erikchen, 2017-05-20 00:06:09]

erikchen@chromium.org changed reviewers:
+ primiano@chromium.org

[erikchen, 2017-05-20 00:06:09]

primiano: Please review.

[commit-bot: I haz the power, 2017-05-20 00:06:22]

Dry run: CQ is trying da patch.

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

[DmitrySkiba, 2017-05-20 00:23:23]

dskiba@chromium.org changed reviewers:
+ dskiba@chromium.org

[DmitrySkiba, 2017-05-20 00:23:23]

This is useful. Blink sometimes produces 100+ frame deep traces.

But I think we need to add a fake frame in the case when backtrace was truncated
to indicate there was a truncation. Otherwise the trace will be confusing.

I.e.

[Thread: foo]
  Blink::layout
    Blink::layoutNode
       ...

is confusing because it seems that Blink::layout is the root function, while

[Thread: foo]
  <truncated>
    Blink::layout
      Blink::layoutNode
         ...

looks better.

https://codereview.chromium.org/2892273003/diff/1/base/trace_event/heap_profi...
File base/trace_event/heap_profiler_allocation_context_tracker.cc (right):

https://codereview.chromium.org/2892273003/diff/1/base/trace_event/heap_profi...
base/trace_event/heap_profiler_allocation_context_tracker.cc:209: const void*
frames[Backtrace::kMaxFrameCount];
Indentation is wrong (same below).

[Primiano Tucci (use gerrit), 2017-05-20 02:28:49]

LGTM (I feel this is like the 3rd time that we change top vs bottom, but
whatever you guys agree on sgtm).

Re-indentation: just make sure it's git cl format-ed. git cl format wins :)

[commit-bot: I haz the power, 2017-05-20 04:02:59]

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

[commit-bot: I haz the power, 2017-05-20 04:03:00]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[erikchen, 2017-05-20 18:16:41]

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

[commit-bot: I haz the power, 2017-05-20 18:16:52]

Dry run: CQ is trying da patch.

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

[erikchen, 2017-05-20 18:17:12]

On 2017/05/20 02:28:49, Primiano (slow - traveling) wrote:
> LGTM (I feel this is like the 3rd time that we change top vs bottom, but
> whatever you guys agree on sgtm).
> 
> Re-indentation: just make sure it's git cl format-ed. git cl format wins :)

dskiba: PTAL

Note: All my CLs go through clang format.

[commit-bot: I haz the power, 2017-05-20 20:22:31]

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

[commit-bot: I haz the power, 2017-05-20 20:22:32]

Dry run: This issue passed the CQ dry run.

[keishi, 2017-05-22 02:37:03]

keishi@chromium.org changed reviewers:
+ keishi@chromium.org

[keishi, 2017-05-22 02:37:04]

cc +tasak

[DmitrySkiba, 2017-05-22 15:28:47]

On 2017/05/20 18:17:12, erikchen wrote:
> On 2017/05/20 02:28:49, Primiano (slow - traveling) wrote:
> > LGTM (I feel this is like the 3rd time that we change top vs bottom, but
> > whatever you guys agree on sgtm).
> > 
> > Re-indentation: just make sure it's git cl format-ed. git cl format wins :)
> 
> dskiba: PTAL
> 
> Note: All my CLs go through clang format.

Then it seems like a clang format bug - "// Backtrace contract requires us to
return bottom frames, i.e." clearly should be indented. What happens if you
indent manually - does presubmit complain about format?

Otherwise LGTM

[Primiano Tucci (use gerrit), 2017-05-22 15:39:32]

On 2017/05/22 15:28:47, DmitrySkiba wrote:
> On 2017/05/20 18:17:12, erikchen wrote:
> > On 2017/05/20 02:28:49, Primiano (slow - traveling) wrote:
> > > LGTM (I feel this is like the 3rd time that we change top vs bottom, but
> > > whatever you guys agree on sgtm).
> > > 
> > > Re-indentation: just make sure it's git cl format-ed. git cl format wins
:)
> > 
> > dskiba: PTAL
> > 
> > Note: All my CLs go through clang format.
> 
> Then it seems like a clang format bug - "// Backtrace contract requires us to
> return bottom frames, i.e." clearly should be indented. What happens if you
> indent manually - does presubmit complain about format?
> 
> Otherwise LGTM

aha I know what the problem is: that entire file is not clang-formatted. git cl
format will only format the changed lines.
So the changed lines are fine, is the rest od the file that is wrong. I wonder
how it happened, base now has a presubmit check that enforces formatting.
Anyways, my suggestions is to land this and mass clang-format everything in a
next step.

[erikchen, 2017-05-22 16:17:44]

The CQ bit was checked by erikchen@chromium.org

[erikchen, 2017-05-22 16:17:44]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/2892273003/#ps40001
(title: "Comments from dskiba.")

[commit-bot: I haz the power, 2017-05-22 16:18:16]

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, 2017-05-22 18:08:16]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1495469864257490,
"parent_rev": "e97f707738a7c04418efe00cd4c68d5ef2fea3c8", "commit_rev":
"72f94d55762707bcd71caeacf6bf107718ed37f0"}

[commit-bot: I haz the power, 2017-05-22 18:08:32]

Description was changed from

==========
Produce more useful stack traces for heap profiling.

Previously, when the stack was too large, we'd keep the top part of the stack
[closer to main()]. This proves to be less useful than the the bottom part of
the stack. This CL changes the logic to keep the bottom part of the stack.

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

to

==========
Produce more useful stack traces for heap profiling.

Previously, when the stack was too large, we'd keep the top part of the stack
[closer to main()]. This proves to be less useful than the the bottom part of
the stack. This CL changes the logic to keep the bottom part of the stack.

BUG=

Review-Url: https://codereview.chromium.org/2892273003
Cr-Commit-Position: refs/heads/master@{#473621}
Committed:
https://chromium.googlesource.com/chromium/src/+/72f94d55762707bcd71caeacf6bf...
==========

[commit-bot: I haz the power, 2017-05-22 18:08:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/72f94d55762707bcd71caeacf6bf...