[PATCH] Improve CPUPROFILE_PER_THREAD_TIMERS

[GoogleCodeExporter]

This is a feature request.

We frequently want to use CPUPROFILE_REALTIME to see where our process is 
actually spending wall-clock time. Unfortunately, as far as I can tell, (a) 
this works strangely with multiple threads (I still have not figured out the 
details) and (b) if the process already uses SIGALRM, this interferes with 
gperftools.

The modern POSIX "timer_create" can solve both of these problems, since it can 
both target and individual thread and arrange to send an arbitrary signal at 
timer expiry. There is already an (undocumented) CPUPROFILE_PER_THREAD_TIMERS 
that uses timer_create to target the profiling signals to each thread.

This is a feature request to extend this to allow this environment variable to 
be set to a signal number:

  env CPUPROFILE_PER_THREAD_TIMERS=34

...which just means to use signal 34 internally for the sigev_signo field in 
the struct sigevent.

I would be happy to work up a patch if there is any reasonable chance the 
maintainer(s) would merge it. But I do not want to bother if you think this is 
a totally wrong-headed idea.

Thanks!

Original issue reported on code.google.com by lopre...@gmail.com on 8 May 2015 at 6:07

[GoogleCodeExporter]

I went ahead and implemented this. It does not break any unit tests and it 
seems to work fine on my system (RHEL 6.6).

Patch is attached. Let me know what I can do to make it suitable for merging.

Original comment by lopre...@gmail.com on 8 May 2015 at 10:19

Attachments:

• issue687.patch

[GoogleCodeExporter]

Looks good. Thanks.

My only little concern is that this patch changes semantic of 
CPUPROFILE_PER_THREAD_TIMERS environment variable. I.e. before this patch 
somebody could be setting it to 1 or 0. After this patch it'll be interpreted 
as signal number.

I think it would be even better to just add separate environment variable for 
new behavior (or new variable for signal number).

Original comment by alkondratenko on 9 May 2015 at 10:24

• Changed title: [PATCH] Improve CPUPROFILE_PER_THREAD_TIMERS

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Given that this feature was never even documented (except in the release 
announcement on the home page), and it never even had a unit test, I would 
argue it is simpler just to change the behavior and document the change... 
Also, note that setting the environment variable to the empty string still 
works the same as before.

But you are the maintainer, so your opinion wins :-). If you can make a 
detailed suggestion, I will implement it and send you a patch... Note that a 
separate environment variable for the new behavior would be mutually exclusive 
with CPUPROFILE_PER_THREAD_TIMERS. A new variable for the signal number would 
require (or imply) CPUPROFILE_PER_THREAD_TIMERS. It is not clear to me what 
approach is cleanest.

Let me know your preference. Thanks.

Original comment by lopre...@gmail.com on 10 May 2015 at 12:20

[GoogleCodeExporter]

Something like this:

* if only CPUPROFILE_PER_THREAD_TIMERS is given, default signal is used

* if both CPUPROFILE_PER_THREAD_TIMERS and new flag is given, new behavior 
happens

* if just new flag is given, new behavior happens

* if none of flags are given, traditional behavior happens (i.e. setitimer)

I think this would make most sense. Let me know if you disagree or if it's not 
clear.

And huge thanks for taking care of this.

Original comment by alkondratenko on 10 May 2015 at 1:35

[GoogleCodeExporter]

Updated patch is attached. This adds a "CPUPROFILE_TIMER_SIGNAL" environment 
variable with the semantics you describe. Note that this environment variable 
also enables "per-thread" timers as a side-effect, so it pretty much subsumes 
the other option.

Original comment by lopre...@gmail.com on 10 May 2015 at 9:52

Attachments:

• issue687.patch

[GoogleCodeExporter]

Thanks. Looks good. I'll take another look next Saturday and merge.

Thanks again.

Original comment by alkondratenko on 17 May 2015 at 6:55

[GoogleCodeExporter]

This looks good. But somehow it fails unit tests when I try using this new 
feature:

# CPUPROFILE_TIMER_SIGNAL=34 LD_PRELOAD=/lib/x86_64-linux-gnu/librt.so.1 sh -c 
'./profile_handler_unittest && ./profiler_unittest.sh '
threads have shared timers
Running RegisterUnregisterCallback
Check failed: FLAGS_test_profiler_enabled == linux_per_thread_timers_mode_ || 
IsTimerEnabled()
sh: line 1:  2395 Aborted                 ./profile_handler_unittest

Most likely this is easily fixable (perhaps in test itself).

(Very) Minor things are:

* signal_number_ is called "interrupt" in comment. I'd prefer calling it signal

* if (signal_number) needs to have body in braces. Per google c++ style 
guidelines we're trying to keep

* it would be most awesome if you could send me full commit (i.e. with 
authorship and commit message). I.e. via git format-patch HEAD^. That would 
allow me not to worry about correctly representing your authorship.

Thanks a lot.

Original comment by alkondratenko on 23 May 2015 at 8:54

[GoogleCodeExporter]

Yes, the unit test needed to be updated to understand the new environment 
variable.

"interrupt" is used all over profile-handler.cc to mean the same thing... But 
OK.

I have fixed the unit test and updated the patch as requested. New version is 
attached.

Thanks.

Original comment by lopre...@gmail.com on 25 May 2015 at 7:02

Attachments:

• issue687.patch

[GoogleCodeExporter]

Thanks. Can I get full commit as produced by git format-patch HEAD^ ?

Otherwise I would be unable to correctly represent your authorship in this 
commit and will have to write something like "This patch was contributed by 
user lopresti" in commit message.

Original comment by alkondratenko on 26 May 2015 at 2:15

[GoogleCodeExporter]

I used "git format-patch HEAD^" to create the last version of the patch... I 
gave the file a shorter name, but I did not touch the contents.

Original comment by lopre...@gmail.com on 26 May 2015 at 4:28

[GoogleCodeExporter]

Ah. Indeed. Sorry for misunderstanding. Thanks again.

Original comment by alkondratenko on 26 May 2015 at 6:48

[GoogleCodeExporter]

Merged. Thanks a lot.

Original comment by alkondratenko on 30 May 2015 at 5:52

• Changed state: Fixed