Chromium Code Reviews
Help | Chromium Project | Sign in
(790)

Issue 9416072: Fix gperftools to not arm the profiling timer by default.

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 2 months ago by Mattias Nissler
Modified:
2 years, 1 month ago
Reviewers:
willchan
CC:
chromium-reviews_chromium.org
Visibility:
Public.

Description

Fix gperftools to not arm the profiling timer by default.

In the upstream implementation, the profiling timer would be armed when
initializing the profiler, resulting in SIGPROF signals being generated
unconditionally every 10 milliseconds. Obviously, we don't want that in
Chromium :)

BUG=chromium:115149
TEST=Run chrome in gdb, with "handle SIGPROF stop", verify that no SIGPROF signals show up.

Patch Set 1 #

Patch Set 2 : Rebase #

Patch Set 3 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+157 lines, -171 lines) Lint Patch
M third_party/tcmalloc/chromium/src/gperftools/profiler.h View 1 1 chunk +4 lines, -0 lines 0 comments 0 errors Download
M third_party/tcmalloc/chromium/src/profile-handler.h View 1 1 chunk +12 lines, -0 lines 0 comments 0 errors Download
M third_party/tcmalloc/chromium/src/profile-handler.cc View 1 18 chunks +133 lines, -171 lines 0 comments 3 errors Download
M third_party/tcmalloc/chromium/src/profiler.cc View 1 2 chunks +5 lines, -0 lines 0 comments 0 errors Download
M third_party/tcmalloc/chromium/src/tests/profile-handler_unittest.cc View 1 1 chunk +1 line, -0 lines 0 comments 0 errors Download
M third_party/tcmalloc/chromium/src/tests/profiler_unittest.cc View 1 1 chunk +2 lines, -0 lines 0 comments 0 errors Download
Trybot results: Sign in to try more bots
Commit:

Issue must be closed to Revert

Messages

Total messages: 6
Mattias Nissler
Hey Will, here's a stab at solving the SIGPROF issue I've discussed with you a ...
2 years, 2 months ago #1
willchan
Is it possible for me to let the upstream maintainer review this and just have ...
2 years, 1 month ago #2
Mattias Nissler
On 2012/02/23 19:22:15, willchan wrote: > Is it possible for me to let the upstream ...
2 years, 1 month ago #3
willchan
On Thu, Feb 23, 2012 at 11:39 AM, <mnissler@chromium.org> wrote: > On 2012/02/23 19:22:15, willchan ...
2 years, 1 month ago #4
Mattias Nissler
On Thu, Feb 23, 2012 at 8:41 PM, William Chan (陈智昌) <willchan@chromium.org>wrote: > On Thu, ...
2 years, 1 month ago #5
willchan
2 years, 1 month ago #6
On Thu, Feb 23, 2012 at 11:49 AM, Mattias Nissler <mnissler@chromium.org>wrote:

>
>
> On Thu, Feb 23, 2012 at 8:41 PM, William Chan (陈智昌) <willchan@chromium.org
> > wrote:
>
>> On Thu, Feb 23, 2012 at 11:39 AM, <mnissler@chromium.org> wrote:
>>
>>> On 2012/02/23 19:22:15, willchan wrote:
>>>
>>>> Is it possible for me to let the upstream maintainer review this and
>>>> just have
>>>> us merge in the change later on? I suspect he will do a better review
>>>> than I.
>>>>
>>>
>>> Absolutely :) I have also uploaded the change to upstream, but I haven't
>>> heard
>>> back yet, so it might be a while. Also note that the chromium version of
>>> gperftools diverges quite a bit from upstream, so it's not trivial to
>>> roll in a
>>> new gperftools version.
>>>
>>
>> Yeah, I suspect he's busy, but it's best to wait for him. And yeah,
>> merging is nontrivial, but is still the right thing to do. Thanks for doing
>> the right thing.
>>
>
> I'll just ping the maintainer to find out whether there's somebody
> listening at all :)
>

> Note: if we feel like this bug is bad, I can review a Chromium specific
>> change first. But I'm inclined to endure (or more accurately, ask you to
>> endure, sorry!) the pain of getting it done upstream first, since I don't
>> feel it's particularly urgent. Maybe I'm wrong.
>>
>
> Unfortunately, I don't have any data on how bad it is either. It might be
> quite bad in terms of power consumption, assuming it prevents putting the
> CPU to sleep... I don't know how I would verify/measure this effect though.
> I guess I should ask a few people.
>

In absence of data, and the fact that it's been this way for awhile
already, I'm inclined to say it's not urgent (but should definitely be
fixed). But yes, please ask folks, especially the Android folks I guess.


>
>
>
>>
>>>
>>>
http://codereview.chromium.**org/9416072/<http://codereview.chromium.org/9416...
>>>
>>
>>
>
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6