Moving cpu profiling into its own thread.

Moving cpu profiling into its own thread.

BUG=None

Committed: https://code.google.com/p/v8/source/detail?r=12649

[caseq, 2012-08-16 14:39:22]

http://codereview.chromium.org/10857035/diff/1/src/cpu-profiler.cc
File src/cpu-profiler.cc (left):

http://codereview.chromium.org/10857035/diff/1/src/cpu-profiler.cc#oldcode252
src/cpu-profiler.cc:252: YieldCPU();
So we're running the loop with 100% usage until it's time to do next sample,
then perform YieldCPU(). This does not seem to make sense. I guess if we're
looking to increase sampling frequency, we can't yield CPU as the scheduler
quantum is probably noticeable larger than sampling period.

http://codereview.chromium.org/10857035/diff/1/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10857035/diff/1/src/cpu-profiler.cc#newcode246
src/cpu-profiler.cc:246: while (OS::Ticks() - start < time_limit)
I would just use absolute time limit, so that we compute it based on when we
started previous sample, not when we started processing.

http://codereview.chromium.org/10857035/diff/1/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/10857035/diff/1/src/platform-linux.cc#newcode1085
src/platform-linux.cc:1085: InstallSignalHandler();
So SignalSender has nothing to do with CPU profiling after your change, yet you
install/remove signal handler within SignalSender.

http://codereview.chromium.org/10857035/diff/1/src/platform-linux.cc#newcode1269
src/platform-linux.cc:1269: void CpuProfilerThread::DoCpuProfile() {
DoCpuProfile -> DoSample?

http://codereview.chromium.org/10857035/diff/1/src/platform.h
File src/platform.h (right):

http://codereview.chromium.org/10857035/diff/1/src/platform.h#newcode776
src/platform.h:776: const int vm_tgid_;
That's a platform specific field, it should be in the implementation.

[caseq, 2012-08-17 12:56:35]

http://codereview.chromium.org/10857035/diff/9001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10857035/diff/9001/src/cpu-profiler.cc#newcode188
src/cpu-profiler.cc:188: bool
ProfilerEventsProcessor::ProcessCodeEvent(unsigned& dequeue_order) {
Output parameters should be pointers, not references:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Reference_Argu...

http://codereview.chromium.org/10857035/diff/9001/src/cpu-profiler.cc#newcode210
src/cpu-profiler.cc:210: bool ProfilerEventsProcessor::ProcessTicks(unsigned
dequeue_order, int64_t start, int64_t time_limit) {
just pass absolute time limit instead of start and interval length?

http://codereview.chromium.org/10857035/diff/9001/src/cpu-profiler.cc#newcode244
src/cpu-profiler.cc:244: void
ProfilerEventsProcessor::ProcessEventsQueue(unsigned& dequeue_order, int64_t
start, int64_t time_limit) {
ditto

http://codereview.chromium.org/10857035/diff/9001/src/cpu-profiler.cc#newcode519
src/cpu-profiler.cc:519: processor_ = new ProfilerEventsProcessor(generator_,
sampler, sampler->interval() * 1000);
Do we have to pass interval explicitly?

http://codereview.chromium.org/10857035/diff/9001/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/10857035/diff/9001/src/platform-linux.cc#newco...
src/platform-linux.cc:1337: syscall(SYS_tgkill, tgid, tid, SIGPROF);
looks like we could either use a tkill() or tgkill() with tgid of -1 instead.

[caseq, 2012-08-17 16:59:28]

http://codereview.chromium.org/10857035/diff/8002/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10857035/diff/8002/src/cpu-profiler.cc#newcode260
src/cpu-profiler.cc:260: if (FLAG_sample_stack_in_postprocessor_thread) {
if (sampler_ != NULL)

http://codereview.chromium.org/10857035/diff/12002/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10857035/diff/12002/src/cpu-profiler.cc#newcode49
src/cpu-profiler.cc:49: : Thread(Thread::Options("CpuProfiler",
kProfilerStackSize)),
I'd keep the v8 prefix

http://codereview.chromium.org/10857035/diff/12002/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:246: while (OS::Ticks() < stop_time)
style: use {} around statements that are longer than one line

[caseq, 2012-09-03 11:59:58]

http://codereview.chromium.org/10857035/diff/23002/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/10857035/diff/23002/src/platform-linux.cc#newc...
src/platform-linux.cc:1234: CpuProfilerSignalHandler::InstallSignalHandler();
Can we do this as necessary -- e.g. when a first profiler is being started?

http://codereview.chromium.org/10857035/diff/23002/src/platform-linux.cc#newc...
src/platform-linux.cc:1240: CpuProfilerSignalHandler::RestoreSignalHandler();
ditto

[Jakob Kummerow, 2012-09-04 11:10:51]

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.cc#newcode48
src/cpu-profiler.cc:48:
ProfilerEventsProcessor::ProfilerEventsProcessor(ProfileGenerator* generator,
Sampler* sampler, int interval_in_useconds)
long line

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:211: bool ProfilerEventsProcessor::ProcessTicks(int64_t
stop_time, unsigned dequeue_order) {
long line

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:245: void
ProfilerEventsProcessor::ProcessEventsQueue(int64_t stop_time, unsigned*
dequeue_order) {
long line

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:247: // Process ticks until we have any.
This comment doesn't make sense. "Process ticks until |stop_time|." maybe?

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:505: processor_ = new ProfilerEventsProcessor(generator_,
sampler, FLAG_cpu_profiler_sampling_interval);
long line.

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.h
File src/cpu-profiler.h (right):

http://codereview.chromium.org/10857035/diff/30001/src/cpu-profiler.h#newcode127
src/cpu-profiler.h:127: explicit ProfilerEventsProcessor(ProfileGenerator*
generator, Sampler* sampler, int interval_in_useconds);
long line.
Put each argument onto its own line:
explicit ProfilerEventsProcessor(ProfileGenerator* generator, 
                                 Sampler* sampler,
                                 int interval_in_useconds);

http://codereview.chromium.org/10857035/diff/30001/src/flag-definitions.h
File src/flag-definitions.h (right):

http://codereview.chromium.org/10857035/diff/30001/src/flag-definitions.h#new...
src/flag-definitions.h:337: DEFINE_int(cpu_profiler_sampling_interval, 1000,
"CPU profiler sampling interval in microseconds")
long line

http://codereview.chromium.org/10857035/diff/30001/src/platform-linux.cc
File src/platform-linux.cc (right):

http://codereview.chromium.org/10857035/diff/30001/src/platform-linux.cc#newc...
src/platform-linux.cc:1088: static bool signal_handler_installed() { return
signal_handler_installed_counter_ > 0; }
long line

http://codereview.chromium.org/10857035/diff/30001/src/platform-linux.cc#newc...
src/platform-linux.cc:1172: NULL)) {
fits on one line?

http://codereview.chromium.org/10857035/diff/30001/src/platform-macos.cc
File src/platform-macos.cc (right):

http://codereview.chromium.org/10857035/diff/30001/src/platform-macos.cc#newc...
src/platform-macos.cc:914: // TODO: implement
If you want to commit this before implementing it for all platforms (the old
code would still work on those, right?), please add either your username to the
TODO ("TODO(rogulenko): implement"), or file a bug
(code.google.com/p/v8/issues/list) and put the bug number here ("TODO(12345):
implement").

Same comment for the other platforms.

[Jakob Kummerow, 2012-09-04 13:39:52]

LGTM

caseq: Any more comments?

[caseq, 2012-09-04 13:42:31]

On 2012/09/04 13:39:52, Jakob wrote:
> LGTM
> 
> caseq: Any more comments?

LGTM as well. Thanks a lot, Jakob!