Move *BSD and Solaris Sampler implementations into sampler.cc

Move *BSD and Solaris Sampler implementation into sampler.cc

This is a first step to having profiler sampler implementation in one file with platform capabilities guarded with #ifdef. Otherwise we have very similar implementations scattered over platform-*.cc files which makes it hard to see differences and make changes.

The next steps will be to merge win32, Mac OS X and Cygwin implementations into sampler.cc They suspend profiled thread instead of sending a signal but apart from that the logic is pretty much the same. Then I'm going to move sampler-related code from log.* into sampler.*


BUG=None

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=14265

[yurys, 2013-04-14 06:17:58]

Note that in this patch I use preprocessor variables described at
http://sourceforge.net/p/predef/wiki/OperatingSystems/ to detect underlying OS.
Alternatively I could modyfy our .gyp file to generate appropriate variables.
Let me know if you prefer the latter approach.

[Sven Panne, 2013-04-15 07:08:40]

I really, really like the direction we're going here, I just have some small
comments.

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode29
src/sampler.cc:29: || defined(__NetBSD__) || defined(__sun)
In general, I'd like to move towards feature-based #ifdefs, not some obscure
combination of platform #ifdefs. In this case, I think the intention is
something like HAVE_UCONTEXT_H. It would nice if we could #define this somewhere
(even directly here, perhaps somewhere else later), and then use it like:

#if HAVE_UCONTEXT_H
...
#include <ucontext.h>
#endif

The same would be good for other #ifdefs here and elsewhere. We don't need to do
this in a single step now, but we should keep this in mind as a general goal
where we want to go: Have no platform #ifdefs sprinkled through all the code,
just some easy, readable combinations of feautre-based ones.

We can discuss how and where to define those feature-based #defines later, there
are various ways to do this with various pros and cons, e.g.:

 * Central "config-<platform>.h" headers with all #defines together

 * Feature-base #defines computed from platform-based #defines in the places
which need them.

 * Doing it via the build system.

We don't need to decide now, we can easily refactor this later depending on our
needs.

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode373
src/sampler.cc:373: #if defined(ANDROID)
Just another good example for feautre-based #ifdefs:

#if HAVE_PTHREAD_KILL
...
#else
...
#endif

Regarding the different constant, an idea might be:

#ifndef SYS_tgkill
#deinfe SYS_tgkill __NR_tgkill
#endif

Hmmm, or *does* Android have pthread_kill and we don't use it for some reason? I
don't know, but it would be good to find out and document it here if it is the
case. USE_PTHREAD_KILL is probably a better name than HAVE_PTHREAD_KILL then.

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode381
src/sampler.cc:381: ASSERT(result == 0);
Do we really want to assert this here and ignore the return value on Android? I
am not sure if this intentional.

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode469
src/sampler.cc:469: #else
Just out of curiosity: Do we need this dummy section at all? What would go wrong
if we just used the code above? It would be good if we could reduce the #ifdef
logic as much as possible.

https://codereview.chromium.org/13852005/diff/1/src/sampler.h
File src/sampler.h (right):

https://codereview.chromium.org/13852005/diff/1/src/sampler.h#newcode48
src/sampler.h:48: public:
Style nit: Use a struct instead of class+public, it makes it immediately
crystal-clear that this is a POD (with a convenience constructor).

https://codereview.chromium.org/13852005/diff/1/src/sampler.h#newcode97
src/sampler.h:97: Isolate* isolate() { return isolate_; }
Add "const". Move it just above interval(), this makes it a bit clearer that
those 2 functions just return the constructor arguments right above them.

https://codereview.chromium.org/13852005/diff/1/src/sampler.h#newcode106
src/sampler.h:106: PlatformData* platform_data() { return data_; }
Why do we need 2 accessors? "const" would be nice, too.

[yurys, 2013-04-15 11:37:55]

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode29
src/sampler.cc:29: || defined(__NetBSD__) || defined(__sun)
On 2013/04/15 07:08:40, Sven Panne wrote:
> In general, I'd like to move towards feature-based #ifdefs, not some obscure
> combination of platform #ifdefs. In this case, I think the intention is
> something like HAVE_UCONTEXT_H. It would nice if we could #define this
somewhere
> (even directly here, perhaps somewhere else later), and then use it like:
> 
> #if HAVE_UCONTEXT_H
> ...
> #include <ucontext.h>
> #endif
> 
> The same would be good for other #ifdefs here and elsewhere. We don't need to
do
> this in a single step now, but we should keep this in mind as a general goal
> where we want to go: Have no platform #ifdefs sprinkled through all the code,
> just some easy, readable combinations of feautre-based ones.
> 
> We can discuss how and where to define those feature-based #defines later,
there
> are various ways to do this with various pros and cons, e.g.:
> 
>  * Central "config-<platform>.h" headers with all #defines together
> 
>  * Feature-base #defines computed from platform-based #defines in the places
> which need them.
> 
>  * Doing it via the build system.
> 
> We don't need to decide now, we can easily refactor this later depending on
our
> needs.

I'd first merge all implementations and have them in one file and after that
introduce capability defines you mentioned. WDYT?

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode373
src/sampler.cc:373: #if defined(ANDROID)
On 2013/04/15 07:08:40, Sven Panne wrote:
> Just another good example for feautre-based #ifdefs:
> 
> #if HAVE_PTHREAD_KILL
> ...
> #else
> ...
> #endif
> 
> Regarding the different constant, an idea might be:
> 
> #ifndef SYS_tgkill
> #deinfe SYS_tgkill __NR_tgkill
> #endif
> 
> Hmmm, or *does* Android have pthread_kill and we don't use it for some reason?
I
> don't know, but it would be good to find out and document it here if it is the
> case. USE_PTHREAD_KILL is probably a better name than HAVE_PTHREAD_KILL then.

Let's do this in a separate change?

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode381
src/sampler.cc:381: ASSERT(result == 0);
On 2013/04/15 07:08:40, Sven Panne wrote:
> Do we really want to assert this here and ignore the return value on Android?
I
> am not sure if this intentional.

I have no idea whether it was intentional or not but I'd prefer to keep the
number of potentially breaking changes minimal during the refactoring. We can
easily unify implementations later.

https://codereview.chromium.org/13852005/diff/1/src/sampler.cc#newcode469
src/sampler.cc:469: #else
On 2013/04/15 07:08:40, Sven Panne wrote:
> Just out of curiosity: Do we need this dummy section at all? What would go
wrong
> if we just used the code above? It would be good if we could reduce the #ifdef
> logic as much as possible.

I will remove this section when Mac and Win implementations are merged. The
problem here is that the thread class is called SignalSender on *NIX platforms
and SamplerThread on the rest.

https://codereview.chromium.org/13852005/diff/1/src/sampler.h
File src/sampler.h (right):

https://codereview.chromium.org/13852005/diff/1/src/sampler.h#newcode48
src/sampler.h:48: public:
On 2013/04/15 07:08:40, Sven Panne wrote:
> Style nit: Use a struct instead of class+public, it makes it immediately
> crystal-clear that this is a POD (with a convenience constructor).

Done.

https://codereview.chromium.org/13852005/diff/1/src/sampler.h#newcode97
src/sampler.h:97: Isolate* isolate() { return isolate_; }
On 2013/04/15 07:08:40, Sven Panne wrote:
> Add "const". Move it just above interval(), this makes it a bit clearer that
> those 2 functions just return the constructor arguments right above them.

Done.

https://codereview.chromium.org/13852005/diff/1/src/sampler.h#newcode106
src/sampler.h:106: PlatformData* platform_data() { return data_; }
On 2013/04/15 07:08:40, Sven Panne wrote:
> Why do we need 2 accessors? "const" would be nice, too.

Removed data() accessor, it wasn't used. Added const to platform_data accessor.

[Sven Panne, 2013-04-15 12:20:38]

LGTM. As discussed offline, the plan is to merge the other platforms in a
separate CL to keep the diffs manageable, and then clean things up a bit.

[yurys, 2013-04-15 13:57:59]

Committed patchset #3 manually as r14265 (presubmit successful).