Fix gperftools to not arm the profiling timer by default.

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.

[Mattias Nissler (ping if slow), 2012-02-21 21:14:44]

Hey Will,

here's a stab at solving the SIGPROF issue I've discussed with you a while ago,
which I'd like to get your input on.

The issue with the timer setup code is that older kernels and pthread
implementations (linuxthreads) incorrectly implemented timers per-thread instead
of per-process as mandated by POSIX. Arming timers is much harder in that model,
since the setitimer call must actually be issued by each thread that's supposed
to be profiled.

The current gperftools code would just arm the timers on thread startup, no
matter whether profiling was enabled or not, which obviously isn't an option for
us.

The idea behind my patch is to keep timers disabled initially. In order to get
them armed, the code sends SIGPROF signals to the individual threads via
pthread_kill(), at which point the thread check whether they need to arm their
timers.

In theory, this works just fine, but there is a caveat: In order to send
SIGPROFs, I need the thread identifiers of the registered threads. It's no big
deal to record them when they register. However, a problem arises at thread
termination. In theory, the pthread identifier for the thread can become invalid
when the thread terminates (in case of detached threads) or when the
corresponding pthread_join() call completes (for non-detached threads). However,
the profiling code doesn't have a simple way to learn about thread terminating
and/or thread identifiers becoming invalid. So I have added an UnregisterThread
function that threads should call at termination.

In practice, UnregisterThread is mostly not required, since pthread_kill() will
return ESRCH if it doesn't know the thread. That's assuming that the thread
identifier is still usable at that point though, which pthread doesn't
guarantee. For linuxthreads, that assumption holds. For NPTL, it doesn't hold in
all cases, and I have indeed been able to crash pthread_kill for an exotic
scenario using user-supplied stack memory that I unmap after terminating the
thread. Similarly, for recycled thread stacks the thread identifiers might get
recycled, which can lead to a thread being profiled that did never register for
it (I don't think that's much of an issue though since we assume all threads
should register).

This CL is against the chromium branch of gperftools, I've also upload a patch
against vanilla gperftools at
http://code.google.com/p/gperftools/issues/detail?id=406 which I have tested on
a modern NPTL-based system (kernel 2.6.38.8 / eglibc 2.11.1) and an older
LinuxThreads-based system (kernel 2.4.27 / glibc-2.3.2). Unit tests are passing,
and the signal handlers seem to fire at the right times.

Note that a much simpler option for solving the problem would be that we have a
proper per-process setitimer implementation (that should work on kernels 2.6.10
and later), in which case we could just delete the timer mode detection code and
not do the signal trick. That might be good enough for chromium, not sure
whether that'd be upstream-able.

So far for now, let me know if you have questions!

[willchan no longer on Chromium, 2012-02-23 19:22:15]

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.

[Mattias Nissler (ping if slow), 2012-02-23 19:39:05]

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.

[willchan no longer on Chromium, 2012-02-23 19:41:48]

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.

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.


>
>
http://codereview.chromium.**org/9416072/<http://codereview.chromium.org/9416...
>

[Mattias Nissler (ping if slow), 2012-02-23 19:49:55]

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.



>
>>
>>
http://codereview.chromium.**org/9416072/<http://codereview.chromium.org/9416...
>>
>
>

[willchan no longer on Chromium, 2012-02-23 19:53:22]

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...
>>>
>>
>>
>