Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
283963002:
Android: bake GCC flags for perf into profiling=1 in GYP_DEFINES (Closed)
|
Created:
6 years, 7 months ago by pasko Modified:
6 years, 7 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAndroid: bake GCC flags for perf into profiling=1 in GYP_DEFINES
BUG=366632
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=271037
Patch Set 1 #
Total comments: 4
Patch Set 2 : vague comment added #Messages
Total messages: 10 (0 generated)
even though quite a minor change, it should simplify the process of dealing with perf on Android. More details in the bug ant its siblings.
https://codereview.chromium.org/283963002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3454 build/common.gypi:3454: '-fno-optimize-sibling-calls', Looks like this one is also set by profiling_full_stack_frames=1. What's the advantage of having it here too? https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3457 build/common.gypi:3457: '-fomit-frame-pointer', BTW, is this just a safe guard or did we end up removing frame pointers in some cases?
https://codereview.chromium.org/283963002/diff/1/build/common.gypi File build/common.gypi (right): https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3454 build/common.gypi:3454: '-fno-optimize-sibling-calls', On 2014/05/15 13:34:32, Sami wrote: > Looks like this one is also set by profiling_full_stack_frames=1. What's the > advantage of having it here too? I noticed too. The profiling=1 is supposed to be the default way to use perf. Without -fno-optimize-sibling-calls I found perf reports on Android to be often misleading. On the other hand, profiling_full_stack_frames=1 disallows inlining (sort of), which I never needed, so not enabling it to be closer to profiling=0 performance. profiling=1 *and* profiling_full_stack_frames=1 together may be useful in some cases (such as for memory profiling?), so let's have the two modes. https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3457 build/common.gypi:3457: '-fomit-frame-pointer', On 2014/05/15 13:34:32, Sami wrote: > BTW, is this just a safe guard or did we end up removing frame pointers in some > cases? We add the -fomit-frame-pointer flag on Android at line 3184 in this file.
lgtm modulo a comment. On 2014/05/15 14:34:53, pasko wrote: > https://codereview.chromium.org/283963002/diff/1/build/common.gypi > File build/common.gypi (right): > > https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3454 > build/common.gypi:3454: '-fno-optimize-sibling-calls', > On 2014/05/15 13:34:32, Sami wrote: > > Looks like this one is also set by profiling_full_stack_frames=1. What's the > > advantage of having it here too? > > I noticed too. The profiling=1 is supposed to be the default way to use perf. > Without -fno-optimize-sibling-calls I found perf reports on Android to be often > misleading. On the other hand, profiling_full_stack_frames=1 disallows inlining > (sort of), which I never needed, so not enabling it to be closer to profiling=0 > performance. > > profiling=1 *and* profiling_full_stack_frames=1 together may be useful in some > cases (such as for memory profiling?), so let's have the two modes. Ah, right, disabling inlining completely is probably too drastic for most cases. Would you mind adding a short comment here why this is needed? > https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3457 > build/common.gypi:3457: '-fomit-frame-pointer', > On 2014/05/15 13:34:32, Sami wrote: > > BTW, is this just a safe guard or did we end up removing frame pointers in > some > > cases? > > We add the -fomit-frame-pointer flag on Android at line 3184 in this file. I see. At least I haven't had issues with perf stack traces so I was wondering if this makes a difference, but better be safe I think.
On 2014/05/15 15:55:07, Sami wrote: > lgtm modulo a comment. > > On 2014/05/15 14:34:53, pasko wrote: > > https://codereview.chromium.org/283963002/diff/1/build/common.gypi > > File build/common.gypi (right): > > > > https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3454 > > build/common.gypi:3454: '-fno-optimize-sibling-calls', > > On 2014/05/15 13:34:32, Sami wrote: > > > Looks like this one is also set by profiling_full_stack_frames=1. What's the > > > advantage of having it here too? > > > > I noticed too. The profiling=1 is supposed to be the default way to use perf. > > Without -fno-optimize-sibling-calls I found perf reports on Android to be > often > > misleading. On the other hand, profiling_full_stack_frames=1 disallows > inlining > > (sort of), which I never needed, so not enabling it to be closer to > profiling=0 > > performance. > > > > profiling=1 *and* profiling_full_stack_frames=1 together may be useful in some > > cases (such as for memory profiling?), so let's have the two modes. > > Ah, right, disabling inlining completely is probably too drastic for most cases. > Would you mind adding a short comment here why this is needed? By "this" you probably mean "-fno-optimize-sibling-calls"? :) I made a very vague comment because now I cannot reproduce the exact scenarios that were confusing the "perf report" for me a few months ago, still feels safer this way. > > https://codereview.chromium.org/283963002/diff/1/build/common.gypi#newcode3457 > > build/common.gypi:3457: '-fomit-frame-pointer', > > On 2014/05/15 13:34:32, Sami wrote: > > > BTW, is this just a safe guard or did we end up removing frame pointers in > > some > > > cases? > > > > We add the -fomit-frame-pointer flag on Android at line 3184 in this file. > > I see. At least I haven't had issues with perf stack traces so I was wondering > if this makes a difference, but better be safe I think. It is not strictly necessary to remove this flag once we add -fno-omit-frame-pointer after it. I'd prefer not relying on the flag order.
On 2014/05/15 16:07:43, pasko wrote: > By "this" you probably mean "-fno-optimize-sibling-calls"? :) > I made a very vague comment because now I cannot reproduce the exact scenarios > that were confusing the "perf report" for me a few months ago, still feels safer > this way. Yes, that's right. Thanks for adding the comment even if it's vague. > It is not strictly necessary to remove this flag once we add > -fno-omit-frame-pointer after it. I'd prefer not relying on the flag order. Agreed.
The CQ bit was checked by pasko@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/pasko@chromium.org/283963002/20001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
Message was sent while issue was closed.
Change committed as 271037 |
