Reimplement profiler sampler on Mac OS X to get it working under Chromium.

Reimplement profiler sampler on Mac OS X to get it working under Chromium.

Previous implementation of sampler for OS X was copied from the Linux one. But BSD (OS X) and Linux has a very important difference in signal handling. LinuxThreads doesn't support the notion of process-directed signals. So, the SIGPROF signal was directed to the thread that installed the handler---the V8 thread. But on BSD, signal handling is implemented according to POSIX spec, where process-directed signal is to be handled by an arbitrary selected thread. By a coincidence, in V8's sample shell and in Chromium's test shell, V8's thread was picked almost every time, so sampling seemed working. But not in case of Chromium.

So, I've changed the implementation of profiler sampler to use the same scheme as on Windows---a dedicated thread with high priority is used to periodically pause and sample V8's thread.

Committed: http://code.google.com/p/v8/source/detail?r=2315

[Mikhail Naganov, 2009-06-25 13:34:41]

Hi Ivan,

You seem to be the main expert in Mac OS X. Please look at this change list.

[iposva, 2009-06-26 07:48:34]

First set of comments. Please fix the race conditions and I will take another
look.

-Ivan

http://codereview.chromium.org/147150/diff/1/2
File src/platform-macos.cc (right):

http://codereview.chromium.org/147150/diff/1/2#newcode542
Line 542: thread_resume(profiled_thread_);
If the sampler_ is profiling, but thread_suspend above failed, then you can over
resume a thread in this case. Another race condition exists because you have two
independent checks for sampling without protection from concurrent modification
which can also lead to unbalanced suspend counts.

Please move this up into the scope guarded by the single check for sampler
profiling and thread suspension success. This has the additional benefit that
you allow running of the main thread while you are recording the tick.

http://codereview.chromium.org/147150/diff/1/2#newcode576
Line 576: data_->profiled_thread_ = mach_thread_self();
I forgot the exact life cycle rules for mach ports. Do you have to destroy the
result of mach_thread_self() or are you handed a singleton in this case?

http://codereview.chromium.org/147150/diff/1/2#newcode586
Line 586: pthread_attr_setschedpolicy(&sched_attr, SCHED_FIFO);
Do you know whether these scheduling settings are implemented and honored on Mac
OS X?

[Mikhail Naganov, 2009-06-26 09:56:39]

Thanks, Ivan! Very insightful comments!

http://codereview.chromium.org/147150/diff/1/2
File src/platform-macos.cc (right):

http://codereview.chromium.org/147150/diff/1/2#newcode542
Line 542: thread_resume(profiled_thread_);
On 2009/06/26 07:48:34, iposva wrote:
> If the sampler_ is profiling, but thread_suspend above failed, then you can
over
> resume a thread in this case. Another race condition exists because you have
two
> independent checks for sampling without protection from concurrent
modification
> which can also lead to unbalanced suspend counts.
> 
> Please move this up into the scope guarded by the single check for sampler
> profiling and thread suspension success. This has the additional benefit that
> you allow running of the main thread while you are recording the tick.

Thanks, fixed this. I need to keep the thread suspended during
sampler_->Tick(&sample) execution because it samples the stack.

IsProfiling is in fact a constant, because it is only inited, and not modified
further (I've enforced this by making it 'const' explicitly). But you're right
about thread_suspend. I also fixed the same problem in the Windows version.

http://codereview.chromium.org/147150/diff/1/2#newcode576
Line 576: data_->profiled_thread_ = mach_thread_self();
On 2009/06/26 07:48:34, iposva wrote:
> I forgot the exact life cycle rules for mach ports. Do you have to destroy the
> result of mach_thread_self() or are you handed a singleton in this case?

As docs say, calling mach_thread_self increases 'send right' counter, but it
seems that kernel doesn't care even if this counter overflows. To be on a safe
side, I've added deallocation code.

http://codereview.chromium.org/147150/diff/1/2#newcode586
Line 586: pthread_attr_setschedpolicy(&sched_attr, SCHED_FIFO);
On 2009/06/26 07:48:34, iposva wrote:
> Do you know whether these scheduling settings are implemented and honored on
Mac
> OS X?

Can't find neither confirming nor denying info on this. During my experiments, I
obtained about 10K ticks on a test running for 10 secs with 1 ms sampling
interval. So even if FIFO policy isn't implemented, at least the highest
priority works.

[Mikhail Naganov, 2009-06-29 10:59:31]

Ping!

[iposva, 2009-06-30 00:23:40]

Next round of comments. In short: You are still open to failure in case of a
thread_suspend failure.

-Ivan

http://codereview.chromium.org/147150/diff/1/2
File src/platform-macos.cc (right):

http://codereview.chromium.org/147150/diff/1/2#newcode542
Line 542: thread_resume(profiled_thread_);
On 2009/06/26 09:56:39, Mikhail Naganov wrote:
> On 2009/06/26 07:48:34, iposva wrote:
> > If the sampler_ is profiling, but thread_suspend above failed, then you can
> over
> > resume a thread in this case. Another race condition exists because you have
> two
> > independent checks for sampling without protection from concurrent
> modification
> > which can also lead to unbalanced suspend counts.
> > 
> > Please move this up into the scope guarded by the single check for sampler
> > profiling and thread suspension success. This has the additional benefit
that
> > you allow running of the main thread while you are recording the tick.
> 
> Thanks, fixed this. I need to keep the thread suspended during
> sampler_->Tick(&sample) execution because it samples the stack.
> 
> IsProfiling is in fact a constant, because it is only inited, and not modified
> further (I've enforced this by making it 'const' explicitly). But you're right
> about thread_suspend. I also fixed the same problem in the Windows version.

Much better. But I still do not get why you need to split the if into two parts.
You say yourself that you need to call sampler_->Tick(&sample) with the target
thread suspended, but if the call to thread_suspend fails, then you will call
this method with the target thread running, causing casualties with high
probability.

http://codereview.chromium.org/147150/diff/8/1008
File src/platform-win32.cc (right):

http://codereview.chromium.org/147150/diff/8/1008#newcode1800
Line 1800: sampler_->Tick(&sample);
Same issue here as with the Mac OS X version. You can call
sampler_->Tick(&sample) with the target thread running. I now understand how
this appears to be safe at the moment, but consider this scenario:
sampler_ is profiling. thread_suspend fails. StackTracer::Trace is called with
target thread running. This is not what was intended.

To me it would feel much safer if the collection of the information in the
sample provided by StackTracer::Trace was protected by the "if
(thread_suspended)" statement. Basically if you move the call from Tick() into
the code here, then you avoid all these issues. In this case Tick() would not
really collect any data making the API much more natural.

[Mikhail Naganov, 2009-06-30 12:33:59]

Thanks, Ivan! I hope now it's OK.

http://codereview.chromium.org/147150/diff/8/1008
File src/platform-win32.cc (right):

http://codereview.chromium.org/147150/diff/8/1008#newcode1800
Line 1800: sampler_->Tick(&sample);
On 2009/06/30 00:23:40, iposva wrote:
> Same issue here as with the Mac OS X version. You can call
> sampler_->Tick(&sample) with the target thread running. I now understand how
> this appears to be safe at the moment, but consider this scenario:
> sampler_ is profiling. thread_suspend fails. StackTracer::Trace is called with
> target thread running. This is not what was intended.
> 
> To me it would feel much safer if the collection of the information in the
> sample provided by StackTracer::Trace was protected by the "if
> (thread_suspended)" statement. Basically if you move the call from Tick() into
> the code here, then you avoid all these issues. In this case Tick() would not
> really collect any data making the API much more natural.

You're right about the problem here. However, I still don't want to move the
"Tick" call under the "if" branch because of "We always sample the VM state."
comment. To fix the issue, I've split the "Tick" implementation into stack
sampling and VM state logging activity. Now stack sampling goes under the "if"
branch and VM state logging remains on its place.

[iposva, 2009-07-01 05:54:49]

LGTM -ip