Closed GoogleCodeExporter closed 9 years ago
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
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
[deleted comment]
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
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
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
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
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
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:
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
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
Ah. Indeed. Sorry for misunderstanding. Thanks again.
Original comment by alkondratenko
on 26 May 2015 at 6:48
Merged. Thanks a lot.
Original comment by alkondratenko
on 30 May 2015 at 5:52
Original issue reported on code.google.com by
lopre...@gmail.com
on 8 May 2015 at 6:07