Do not record CPU profile samples when stack collection is failed.

Do not record CPU profile samples when stack collection is failed.

BUG=559304
LOG=N

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

[alph, 2016-02-18 02:24:11]

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

[commit-bot: I haz the power, 2016-02-18 02:24:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1709873002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1709873002/1

[commit-bot: I haz the power, 2016-02-18 02:50:45]

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

[commit-bot: I haz the power, 2016-02-18 02:50:46]

Dry run: This issue passed the CQ dry run.

[alph, 2016-02-18 03:21:35]

alph@chromium.org changed reviewers:
+ bmeurer@chromium.org

[alph, 2016-02-18 18:30:44]

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

[alph, 2016-02-18 18:30:44]

ptal

[titzer, 2016-02-19 09:28:37]

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.cc
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.c...
src/profiler/cpu-profiler.cc:117: if (record->sample.pc) {
Will this tick just be discarded? Is there a bucket for discarded ticks to avoid
showing skewed results?

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

https://codereview.chromium.org/1709873002/diff/1/src/profiler/profile-genera...
src/profiler/profile-generator.cc:639: for (const Address *stack_pos =
sample.stack,
Formatting artifact? git cl format should have fixed this.

[alph, 2016-02-19 16:24:29]

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.cc
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.c...
src/profiler/cpu-profiler.cc:117: if (record->sample.pc) {
On 2016/02/19 09:28:37, titzer wrote:
> Will this tick just be discarded? Is there a bucket for discarded ticks to
avoid
> showing skewed results?

It is just discarded, so effectively it adds to all other samples a bit.
There's no such a bucket. If I add one I'm not sure how that would be presented
to the user.
Actually currently the results are skewed as well because if we fail to collect
a stack the sample is not attributed to the function currently being executed,
but to a program node instead. So the function node becomes smaller than it is
in the reality.

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

https://codereview.chromium.org/1709873002/diff/1/src/profiler/profile-genera...
src/profiler/profile-generator.cc:639: for (const Address *stack_pos =
sample.stack,
On 2016/02/19 09:28:37, titzer wrote:
> Formatting artifact? git cl format should have fixed this.

So it did.
I hope you don't think I reformatted that manually? ;-)

[titzer, 2016-02-19 16:36:33]

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.cc
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.c...
src/profiler/cpu-profiler.cc:117: if (record->sample.pc) {
On 2016/02/19 16:24:29, alph wrote:
> On 2016/02/19 09:28:37, titzer wrote:
> > Will this tick just be discarded? Is there a bucket for discarded ticks to
> avoid
> > showing skewed results?
> 
> It is just discarded, so effectively it adds to all other samples a bit.
> There's no such a bucket. If I add one I'm not sure how that would be
presented
> to the user.
> Actually currently the results are skewed as well because if we fail to
collect
> a stack the sample is not attributed to the function currently being executed,
> but to a program node instead. So the function node becomes smaller than it is
> in the reality.

As long as the missing samples go somewhere, so as not to skew the existing
samples, then this sgtm.

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

https://codereview.chromium.org/1709873002/diff/1/src/profiler/profile-genera...
src/profiler/profile-generator.cc:639: for (const Address *stack_pos =
sample.stack,
On 2016/02/19 16:24:29, alph wrote:
> On 2016/02/19 09:28:37, titzer wrote:
> > Formatting artifact? git cl format should have fixed this.
> 
> So it did.
> I hope you don't think I reformatted that manually? ;-)

I just asked because it looks backwards to our normal formatting of the star
sticking to the left (i.e. to the type).

[alph, 2016-02-19 17:54:22]

Thanks for review.

ptal. Made the spoiled sample to still contribute to the stats, but not recorded
to the samples list.

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.cc
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/1709873002/diff/1/src/profiler/cpu-profiler.c...
src/profiler/cpu-profiler.cc:117: if (record->sample.pc) {
On 2016/02/19 16:36:33, titzer wrote:
> On 2016/02/19 16:24:29, alph wrote:
> > On 2016/02/19 09:28:37, titzer wrote:
> > > Will this tick just be discarded? Is there a bucket for discarded ticks to
> > avoid
> > > showing skewed results?
> > 
> > It is just discarded, so effectively it adds to all other samples a bit.
> > There's no such a bucket. If I add one I'm not sure how that would be
> presented
> > to the user.
> > Actually currently the results are skewed as well because if we fail to
> collect
> > a stack the sample is not attributed to the function currently being
executed,
> > but to a program node instead. So the function node becomes smaller than it
is
> > in the reality.
> 
> As long as the missing samples go somewhere, so as not to skew the existing
> samples, then this sgtm.

Agree.

[titzer, 2016-02-19 17:55:41]

lgtm

[alph, 2016-02-19 17:58:28]

The CQ bit was checked by alph@chromium.org

[commit-bot: I haz the power, 2016-02-19 17:58:42]

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

[commit-bot: I haz the power, 2016-02-19 18:49:18]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-02-19 18:49:44]

Description was changed from

==========
Do not record CPU profile samples when stack collection is failed.

BUG=559304
LOG=N
==========

to

==========
Do not record CPU profile samples when stack collection is failed.

BUG=559304
LOG=N

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

[commit-bot: I haz the power, 2016-02-19 18:49:45]

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