Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650, 541419
Committed: https://crrev.com/c383cfebcbf223980034223788973dff2f382d43
Cr-Commit-Position: refs/heads/master@{#362526}

[Mike Wittman, 2015-11-23 18:30:59]

Description was changed from

==========
Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650
==========

to

==========
Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650, 541419
==========

[Mike Wittman, 2015-11-23 18:32:15]

wittman@chromium.org changed reviewers:
+ brucedawson@chromium.org

[Mike Wittman, 2015-11-23 18:32:15]

PTAL

[brucedawson, 2015-11-23 19:15:28]

On 2015/11/23 18:32:15, Mike Wittman wrote:
> PTAL

What's causing the false positives? i.e.; why is SanityCheckRuntimeFunction
triggering when it shouldn't? Is that understood?

[Mike Wittman, 2015-11-23 19:40:42]

On 2015/11/23 19:15:28, brucedawson wrote:
> On 2015/11/23 18:32:15, Mike Wittman wrote:
> > PTAL
> 
> What's causing the false positives? i.e.; why is SanityCheckRuntimeFunction
> triggering when it shouldn't? Is that understood?

Yes, there are cases where the code for functions legitimately does not lie
between the RUNTIME_FUNCTION begin and end addresses. I've seen this
particularly in some Windows DLLs where functions contain jumps to other
supporting code. I'm assuming this is due to either LTCG deduplication or
hand-coded assembly implementations.

[brucedawson, 2015-11-23 19:49:15]

On 2015/11/23 19:40:42, Mike Wittman wrote:
> On 2015/11/23 19:15:28, brucedawson wrote:
> > On 2015/11/23 18:32:15, Mike Wittman wrote:
> > > PTAL
> > 
> > What's causing the false positives? i.e.; why is SanityCheckRuntimeFunction
> > triggering when it shouldn't? Is that understood?
> 
> Yes, there are cases where the code for functions legitimately does not lie
> between the RUNTIME_FUNCTION begin and end addresses. I've seen this
> particularly in some Windows DLLs where functions contain jumps to other
> supporting code. I'm assuming this is due to either LTCG deduplication or
> hand-coded assembly implementations.

Windows binaries go through a post-link step that aggressively rearranges basic
blocks in order to separate hot and cold code, and this causes functions to be
scattered wildly.

It is unfortunate that the begin and end addresses are a lie. I guess the
alternative is having many overlapping begin/end addresses. I have no idea how
this actually works in practice.

[brucedawson, 2015-11-23 19:49:21]

lgtm

[Mike Wittman, 2015-11-23 21:52:12]

On 2015/11/23 19:49:15, brucedawson wrote:
> On 2015/11/23 19:40:42, Mike Wittman wrote:
> > On 2015/11/23 19:15:28, brucedawson wrote:
> > > On 2015/11/23 18:32:15, Mike Wittman wrote:
> > > > PTAL
> > > 
> > > What's causing the false positives? i.e.; why is
SanityCheckRuntimeFunction
> > > triggering when it shouldn't? Is that understood?
> > 
> > Yes, there are cases where the code for functions legitimately does not lie
> > between the RUNTIME_FUNCTION begin and end addresses. I've seen this
> > particularly in some Windows DLLs where functions contain jumps to other
> > supporting code. I'm assuming this is due to either LTCG deduplication or
> > hand-coded assembly implementations.
> 
> Windows binaries go through a post-link step that aggressively rearranges
basic
> blocks in order to separate hot and cold code, and this causes functions to be
> scattered wildly.
> 
> It is unfortunate that the begin and end addresses are a lie. I guess the
> alternative is having many overlapping begin/end addresses. I have no idea how
> this actually works in practice.

That makes sense. It may be that there's some kind of table maintained for these
regions behind the scenes, with a custom function table callback to map them to
the associated RUNTIME_FUNCTIONs.

[Mike Wittman, 2015-11-23 21:55:18]

wittman@chromium.org changed reviewers:
+ cpu@chromium.org

[Mike Wittman, 2015-11-23 21:55:18]

Carlos, can you approve the profiler enable percentage decrease from 50% to 15%?

Out of caution I'd like to keep this at 15% while I'm addressing the remaining
profiler issues.

[cpu_(ooo_6.6-7.5), 2015-11-23 23:50:53]

https://codereview.chromium.org/1465273002/diff/1/chrome/browser/stack_sampli...
File chrome/browser/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/1465273002/diff/1/chrome/browser/stack_sampli...
chrome/browser/stack_sampling_configuration.cc:144: DCHECK_EQ(100,
total_weight);
does not sum to 100

[Mike Wittman, 2015-11-24 00:02:24]

https://codereview.chromium.org/1465273002/diff/1/chrome/browser/stack_sampli...
File chrome/browser/stack_sampling_configuration.cc (right):

https://codereview.chromium.org/1465273002/diff/1/chrome/browser/stack_sampli...
chrome/browser/stack_sampling_configuration.cc:144: DCHECK_EQ(100,
total_weight);
On 2015/11/23 23:50:53, cpu wrote:
> does not sum to 100

Fixed.

[Mike Wittman, 2015-12-01 01:11:17]

Carlos, can you approve the enable percentage change? I'd like to commit this
after tomorrow's dev release.

[cpu_(ooo_6.6-7.5), 2015-12-01 20:28:56]

lgtm

[Mike Wittman, 2015-12-01 20:31:18]

The CQ bit was checked by wittman@chromium.org

[Mike Wittman, 2015-12-01 20:31:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
brucedawson@chromium.org
Link to the patchset: https://codereview.chromium.org/1465273002/#ps20001
(title: "make variations add to 100")

[commit-bot: I haz the power, 2015-12-01 20:34:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1465273002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1465273002/20001

[commit-bot: I haz the power, 2015-12-01 22:18:50]

Description was changed from

==========
Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650, 541419
==========

to

==========
Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650, 541419
==========

[commit-bot: I haz the power, 2015-12-01 22:18:50]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-01 22:21:23]

Description was changed from

==========
Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650, 541419
==========

to

==========
Stack sampling profiler: remove RUNTIME_FUNCTION sanity check

This heuristic is causing lots of false positives resulting in
prematurely-truncated stacks. It shouldn't be necessary any longer, as
the check that the instruction pointer is in a valid module should fail
for this case before executing this test.

Also, bump the profiler enable percentage down to 15% while evaluating
this and future incremental changes. This is large enough to detect an
incrementally increased crash rate, while small enough that an
unexpected crasher will not cause a huge problem.

BUG=555770, 541650, 541419

Committed: https://crrev.com/c383cfebcbf223980034223788973dff2f382d43
Cr-Commit-Position: refs/heads/master@{#362526}
==========

[commit-bot: I haz the power, 2015-12-01 22:21:24]

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