Split Ticker into two samplers.

Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
an initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N

Committed: https://crrev.com/3ca49d9aec99e0d1f0efc8c97a221dcb70312648
Cr-Commit-Position: refs/heads/master@{#37506}

[lpy, 2016-06-30 01:08:14]

lpy@chromium.org changed reviewers:
+ alph@chromium.org, yangguo@chromium.org

[lpy, 2016-06-30 01:08:14]

PTAL.

[alph, 2016-06-30 01:21:02]

lgtm w/ nits

https://codereview.chromium.org/2108393002/diff/1/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/2108393002/diff/1/src/log.cc#newcode656
src/log.cc:656: if (is_counting_samples_ && !sample.timestamp.IsNull()) {
Is it used in the profiler tests? If not, you can drop it.

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

https://codereview.chromium.org/2108393002/diff/1/src/profiler/cpu-profiler.c...
src/profiler/cpu-profiler.cc:33: const_cast<v8::RegisterState*>(&state)))
const_cast is weird. please make another instance of state for the simulator
case.

https://codereview.chromium.org/2108393002/diff/1/src/profiler/cpu-profiler.h
File src/profiler/cpu-profiler.h (right):

https://codereview.chromium.org/2108393002/diff/1/src/profiler/cpu-profiler.h...
src/profiler/cpu-profiler.h:176: sampler::Sampler* sampler_;
let's make it unique_ptr as it's now owned by the processor.

[lpy, 2016-06-30 20:36:13]

Description was changed from

==========
Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
a initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N
==========

to

==========
Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
an initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N
==========

[alph, 2016-06-30 21:01:48]

some more nits

https://codereview.chromium.org/2108393002/diff/20001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/2108393002/diff/20001/src/log.cc#newcode647
src/log.cc:647: v8::Isolate* v8_isolate = isolate();
if (!profiler_) return;

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:33: regs.pc = state.pc;
you don't need to fill regs. It's an out parameters of FillRegisters.

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:39: if (sample == NULL) return;
nullptr

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:40: sample->Init(i_isolate, state,
TickSample::kIncludeCEntryFrame, true);
should be regs here

[lpy, 2016-06-30 21:13:04]

Thanks, I updated the CL.

Yang@, ptal.

https://codereview.chromium.org/2108393002/diff/1/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/2108393002/diff/1/src/log.cc#newcode656
src/log.cc:656: if (is_counting_samples_ && !sample.timestamp.IsNull()) {
On 2016/06/30 01:21:01, alph wrote:
> Is it used in the profiler tests? If not, you can drop it.

Done.

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

https://codereview.chromium.org/2108393002/diff/1/src/profiler/cpu-profiler.c...
src/profiler/cpu-profiler.cc:33: const_cast<v8::RegisterState*>(&state)))
On 2016/06/30 01:21:02, alph wrote:
> const_cast is weird. please make another instance of state for the simulator
> case.

Done.

https://codereview.chromium.org/2108393002/diff/1/src/profiler/cpu-profiler.h
File src/profiler/cpu-profiler.h (right):

https://codereview.chromium.org/2108393002/diff/1/src/profiler/cpu-profiler.h...
src/profiler/cpu-profiler.h:176: sampler::Sampler* sampler_;
On 2016/06/30 01:21:02, alph wrote:
> let's make it unique_ptr as it's now owned by the processor.

Done.

https://codereview.chromium.org/2108393002/diff/20001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/2108393002/diff/20001/src/log.cc#newcode647
src/log.cc:647: v8::Isolate* v8_isolate = isolate();
On 2016/06/30 21:01:48, alph wrote:
> if (!profiler_) return;

Done.

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:33: regs.pc = state.pc;
On 2016/06/30 21:01:48, alph wrote:
> you don't need to fill regs. It's an out parameters of FillRegisters.

Done.

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:39: if (sample == NULL) return;
On 2016/06/30 21:01:48, alph wrote:
> nullptr

StartTickSample returns NULL here.

https://codereview.chromium.org/2108393002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:40: sample->Init(i_isolate, state,
TickSample::kIncludeCEntryFrame, true);
On 2016/06/30 21:01:48, alph wrote:
> should be regs here

Done.

[alph, 2016-06-30 21:25:10]

https://codereview.chromium.org/2108393002/diff/40001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/2108393002/diff/40001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:35: regs.pc = state.pc;
Why don't you want keep it as it was before?
So in the simulator case you don't need const_cast and in the regular case when
we need speed there's no copying. I mean:

#if defined(USE_SIMULATOR)
  v8::RegisterState regs;
  if (!SimulatorHelper::FillRegisters(i_isolate, &regs)) return;
#else
  const v8::RegisterState& regs = state;
#endif

[lpy, 2016-06-30 21:34:46]

https://codereview.chromium.org/2108393002/diff/40001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/2108393002/diff/40001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.cc:35: regs.pc = state.pc;
On 2016/06/30 21:25:10, alph wrote:
> Why don't you want keep it as it was before?
> So in the simulator case you don't need const_cast and in the regular case
when
> we need speed there's no copying. I mean:
> 
> #if defined(USE_SIMULATOR)
>   v8::RegisterState regs;
>   if (!SimulatorHelper::FillRegisters(i_isolate, &regs)) return;
> #else
>   const v8::RegisterState& regs = state;
> #endif

Done.

[Yang, 2016-07-04 11:02:56]

On 2016/06/30 21:34:46, lpy wrote:
>
https://codereview.chromium.org/2108393002/diff/40001/src/profiler/cpu-profil...
> File src/profiler/cpu-profiler.cc (right):
> 
>
https://codereview.chromium.org/2108393002/diff/40001/src/profiler/cpu-profil...
> src/profiler/cpu-profiler.cc:35: regs.pc = state.pc;
> On 2016/06/30 21:25:10, alph wrote:
> > Why don't you want keep it as it was before?
> > So in the simulator case you don't need const_cast and in the regular case
> when
> > we need speed there's no copying. I mean:
> > 
> > #if defined(USE_SIMULATOR)
> >   v8::RegisterState regs;
> >   if (!SimulatorHelper::FillRegisters(i_isolate, &regs)) return;
> > #else
> >   const v8::RegisterState& regs = state;
> > #endif
> 
> Done.

lgtm

[alph, 2016-07-04 18:58:33]

The CQ bit was checked by alph@chromium.org

[alph, 2016-07-04 18:58:33]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org
Link to the patchset: https://codereview.chromium.org/2108393002/#ps60001
(title: "update")

[commit-bot: I haz the power, 2016-07-04 18:58:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 19:21:45]

Description was changed from

==========
Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
an initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N
==========

to

==========
Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
an initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N
==========

[commit-bot: I haz the power, 2016-07-04 19:21:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-04 19:23:13]

Description was changed from

==========
Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
an initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N
==========

to

==========
Split Ticker into two samplers.

Currently there are two logic in Ticker, one is to try to request a
pre-allocated TickSample from CpuProfiler and then initialize it, and if the
request fails, it will initialize a local TickSample. The other is it will pass
an initialized TickSample to Profiler to log into v8.log.

This patch splits Ticker into two samplers, the first one remains in log.cc to
collect samples and pass to Profiler for logging, the second one will be called
by ProfilerEventsProcessor, and only use the circular queue only.

BUG=v8:4789
LOG=N

Committed: https://crrev.com/3ca49d9aec99e0d1f0efc8c97a221dcb70312648
Cr-Commit-Position: refs/heads/master@{#37506}
==========

[commit-bot: I haz the power, 2016-07-04 19:23:14]

Patchset 4 (id:??) landed as
https://crrev.com/3ca49d9aec99e0d1f0efc8c97a221dcb70312648
Cr-Commit-Position: refs/heads/master@{#37506}