Expose TickSample and its APIs in v8-profiler.h

Expose TickSample and its APIs in v8-profiler.h

We want to eventually move the profiling functionality out of V8 as library,
this patch exposes TickSample and its APIs in v8-profiler.h so that when
embedders use library, they can have more details.

Minor change: Rename tick-sample.[h|cc] to simulator-helper.[h|cc].

BUG=v8:4789
LOG=N

Committed: https://crrev.com/3172f6a9ce72e7318dd55239677dd542a3fab9e6
Cr-Commit-Position: refs/heads/master@{#37564}

[lpy, 2016-06-28 21:04:03]

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

[lpy, 2016-06-28 21:04:04]

PTAL.

One question is, how can we mark Isolate::GetStackSample as "will be
deprecated", since we are going to use TickSample::GetStackSample directly?

Thanks.

[alph, 2016-06-28 22:09:02]

https://codereview.chromium.org/2105943002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2105943002/diff/1/src/api.cc#newcode8331
src/api.cc:8331: DISABLE_ASAN void TickSample::Init(Isolate* v8_isolate,
Don't move that much logic into api.cc
Keep it in a file under profiler. Also you can put SimulatorHelper there rather
than making a dedicated file for it.

https://codereview.chromium.org/2105943002/diff/1/src/profiler/simulator-help...
File src/profiler/simulator-helper.h (right):

https://codereview.chromium.org/2105943002/diff/1/src/profiler/simulator-help...
src/profiler/simulator-helper.h:5: #ifndef V8_PROFILER_SIMULATOR_HELPER_H_
weird diff. Could you please play with --similarity

[lpy, 2016-06-28 23:02:54]

Thanks, I made an update. PTAL.

https://codereview.chromium.org/2105943002/diff/1/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2105943002/diff/1/src/api.cc#newcode8331
src/api.cc:8331: DISABLE_ASAN void TickSample::Init(Isolate* v8_isolate,
On 2016/06/28 22:09:01, alph wrote:
> Don't move that much logic into api.cc
> Keep it in a file under profiler. Also you can put SimulatorHelper there
rather
> than making a dedicated file for it.

Done.

https://codereview.chromium.org/2105943002/diff/1/src/profiler/simulator-help...
File src/profiler/simulator-helper.h (right):

https://codereview.chromium.org/2105943002/diff/1/src/profiler/simulator-help...
src/profiler/simulator-helper.h:5: #ifndef V8_PROFILER_SIMULATOR_HELPER_H_
On 2016/06/28 22:09:01, alph wrote:
> weird diff. Could you please play with --similarity

Done.

[alph, 2016-06-29 19:39:09]

https://codereview.chromium.org/2105943002/diff/20001/include/v8-profiler.h
File include/v8-profiler.h (right):

https://codereview.chromium.org/2105943002/diff/20001/include/v8-profiler.h#n...
include/v8-profiler.h:50: struct TickSample {
Why did you remove the timestamp from TickSample?

https://codereview.chromium.org/2105943002/diff/20001/include/v8-profiler.h#n...
include/v8-profiler.h:58: pc(NULL),
nullptr

https://codereview.chromium.org/2105943002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2105943002/diff/20001/src/api.cc#newcode7571
src/api.cc:7571: regs.pc = state.pc;
Hmm, how these got here??? Can you please drop these three lines, as regs is an
out parameter of FillRegisters.

https://codereview.chromium.org/2105943002/diff/20001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler-inl.h (right):

https://codereview.chromium.org/2105943002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler-inl.h:75: evt->timestamp = evt->sample.pc == nullptr
It's not its business. FinishTickSample should not update the record. Also I'd
rename it to CommitTickSample

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/test-profil...
File test/cctest/test-profile-generator.cc (right):

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/test-profil...
test/cctest/test-profile-generator.cc:46: using v8::TickSample;
sort it?

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/trace-exten...
File test/cctest/trace-extension.cc (right):

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/trace-exten...
test/cctest/trace-extension.cc:93: static struct { v8::TickSample* sample; }
trace_env = {NULL};
nullptr

[lpy, 2016-06-29 20:17:40]

Comments addressed, ptal.

https://codereview.chromium.org/2105943002/diff/20001/include/v8-profiler.h
File include/v8-profiler.h (right):

https://codereview.chromium.org/2105943002/diff/20001/include/v8-profiler.h#n...
include/v8-profiler.h:50: struct TickSample {
On 2016/06/29 19:39:09, alph wrote:
> Why did you remove the timestamp from TickSample?

Can the files in include/ directory include files in src/base?

https://codereview.chromium.org/2105943002/diff/20001/include/v8-profiler.h#n...
include/v8-profiler.h:58: pc(NULL),
On 2016/06/29 19:39:09, alph wrote:
> nullptr

Done.

https://codereview.chromium.org/2105943002/diff/20001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2105943002/diff/20001/src/api.cc#newcode7571
src/api.cc:7571: regs.pc = state.pc;
On 2016/06/29 19:39:09, alph wrote:
> Hmm, how these got here??? Can you please drop these three lines, as regs is
an
> out parameter of FillRegisters.

Done.

https://codereview.chromium.org/2105943002/diff/20001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler-inl.h (right):

https://codereview.chromium.org/2105943002/diff/20001/src/profiler/cpu-profil...
src/profiler/cpu-profiler-inl.h:75: evt->timestamp = evt->sample.pc == nullptr
On 2016/06/29 19:39:09, alph wrote:
> It's not its business. FinishTickSample should not update the record. Also I'd
> rename it to CommitTickSample

Since I remove the timestamp from TickSample, I think it makes sense that when
FinishTickSample gets called, it stamps the timestamp on the record since it
knows the current ticksample in record has been written.

Why CommitTickSample? Since we already have another function named
StarTickSample, should we keep it as a pair? Or rename StartTickSample to
RequestTickSample?

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/test-profil...
File test/cctest/test-profile-generator.cc (right):

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/test-profil...
test/cctest/test-profile-generator.cc:46: using v8::TickSample;
On 2016/06/29 19:39:09, alph wrote:
> sort it?

Done.

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/trace-exten...
File test/cctest/trace-extension.cc (right):

https://codereview.chromium.org/2105943002/diff/20001/test/cctest/trace-exten...
test/cctest/trace-extension.cc:93: static struct { v8::TickSample* sample; }
trace_env = {NULL};
On 2016/06/29 19:39:09, alph wrote:
> nullptr

Done.

[lpy, 2016-06-29 21:23:37]

Patchset #4 (id:60001) has been deleted

[alph, 2016-06-30 21:18:08]

https://codereview.chromium.org/2105943002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2105943002/diff/80001/src/api.cc#newcode7576
src/api.cc:7576: const_cast<RegisterState*>(&state))) {
please avoid using const_cast, as the caller is not expecting you to change the
state.
Bring back the regs variable.

https://codereview.chromium.org/2105943002/diff/80001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler.h (right):

https://codereview.chromium.org/2105943002/diff/80001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.h:113: base::TimeTicks timestamp;
I don't like the timestamp sitting here.
How about inheriting v8::internal::TickSample from v8::TickSample and add the
timestamp ?

[lpy, 2016-06-30 22:51:29]

Thanks, ptal.

https://codereview.chromium.org/2105943002/diff/80001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/2105943002/diff/80001/src/api.cc#newcode7576
src/api.cc:7576: const_cast<RegisterState*>(&state))) {
On 2016/06/30 21:18:08, alph wrote:
> please avoid using const_cast, as the caller is not expecting you to change
the
> state.
> Bring back the regs variable.

Done.

https://codereview.chromium.org/2105943002/diff/80001/src/profiler/cpu-profil...
File src/profiler/cpu-profiler.h (right):

https://codereview.chromium.org/2105943002/diff/80001/src/profiler/cpu-profil...
src/profiler/cpu-profiler.h:113: base::TimeTicks timestamp;
On 2016/06/30 21:18:08, alph wrote:
> I don't like the timestamp sitting here.
> How about inheriting v8::internal::TickSample from v8::TickSample and add the
> timestamp ?

Done.

[lpy, 2016-06-30 22:51:42]

Patchset #5 (id:100001) has been deleted

[lpy, 2016-06-30 22:51:55]

Patchset #5 (id:120001) has been deleted

[alph, 2016-07-01 00:38:13]

lgtm

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/cpu-profi...
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/cpu-profi...
src/profiler/cpu-profiler.cc:52: record.sample.timestamp = record.sample.pc ==
nullptr
nit: You can create internal::TickSample::Init and setup timestamp there after
calling v8::TickSample::Init.

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/tick-samp...
File src/profiler/tick-sample.h (right):

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/tick-samp...
src/profiler/tick-sample.h:15: struct TickSample;
These two are not needed. The declaration is visible here.

[lpy, 2016-07-01 01:00:10]

Thanks, I updated the CL.

Yang@, ptal.

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/cpu-profi...
File src/profiler/cpu-profiler.cc (right):

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/cpu-profi...
src/profiler/cpu-profiler.cc:52: record.sample.timestamp = record.sample.pc ==
nullptr
On 2016/07/01 00:38:13, alph wrote:
> nit: You can create internal::TickSample::Init and setup timestamp there after
> calling v8::TickSample::Init.

Done.

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/tick-samp...
File src/profiler/tick-sample.h (right):

https://codereview.chromium.org/2105943002/diff/160001/src/profiler/tick-samp...
src/profiler/tick-sample.h:15: struct TickSample;
On 2016/07/01 00:38:13, alph wrote:
> These two are not needed. The declaration is visible here.

Done.

[alph, 2016-07-01 01:08:58]

https://codereview.chromium.org/2105943002/diff/180001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/2105943002/diff/180001/src/log.cc#newcode660
src/log.cc:660: sample->timestamp = base::TimeTicks::HighResolutionNow();
not needed?

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/cpu-profi...
File src/profiler/cpu-profiler.h (right):

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/cpu-profi...
src/profiler/cpu-profiler.h:31: struct TickSample;
not needed

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/tick-samp...
File src/profiler/tick-sample.h (right):

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/tick-samp...
src/profiler/tick-sample.h:18: TickSample() : v8::TickSample(),
timestamp(base::TimeTicks()) {}
drop timestamp initialization. It has a default constructor.

[lpy, 2016-07-01 17:33:38]

https://codereview.chromium.org/2105943002/diff/180001/src/log.cc
File src/log.cc (right):

https://codereview.chromium.org/2105943002/diff/180001/src/log.cc#newcode660
src/log.cc:660: sample->timestamp = base::TimeTicks::HighResolutionNow();
On 2016/07/01 01:08:57, alph wrote:
> not needed?

Done.

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/cpu-profi...
File src/profiler/cpu-profiler.h (right):

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/cpu-profi...
src/profiler/cpu-profiler.h:31: struct TickSample;
On 2016/07/01 01:08:57, alph wrote:
> not needed

Done.

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/tick-samp...
File src/profiler/tick-sample.h (right):

https://codereview.chromium.org/2105943002/diff/180001/src/profiler/tick-samp...
src/profiler/tick-sample.h:18: TickSample() : v8::TickSample(),
timestamp(base::TimeTicks()) {}
On 2016/07/01 01:08:57, alph wrote:
> drop timestamp initialization. It has a default constructor.

Done.

[Yang, 2016-07-04 05:46:15]

On 2016/07/01 17:33:38, lpy wrote:
> https://codereview.chromium.org/2105943002/diff/180001/src/log.cc
> File src/log.cc (right):
> 
> https://codereview.chromium.org/2105943002/diff/180001/src/log.cc#newcode660
> src/log.cc:660: sample->timestamp = base::TimeTicks::HighResolutionNow();
> On 2016/07/01 01:08:57, alph wrote:
> > not needed?
> 
> Done.
> 
>
https://codereview.chromium.org/2105943002/diff/180001/src/profiler/cpu-profi...
> File src/profiler/cpu-profiler.h (right):
> 
>
https://codereview.chromium.org/2105943002/diff/180001/src/profiler/cpu-profi...
> src/profiler/cpu-profiler.h:31: struct TickSample;
> On 2016/07/01 01:08:57, alph wrote:
> > not needed
> 
> Done.
> 
>
https://codereview.chromium.org/2105943002/diff/180001/src/profiler/tick-samp...
> File src/profiler/tick-sample.h (right):
> 
>
https://codereview.chromium.org/2105943002/diff/180001/src/profiler/tick-samp...
> src/profiler/tick-sample.h:18: TickSample() : v8::TickSample(),
> timestamp(base::TimeTicks()) {}
> On 2016/07/01 01:08:57, alph wrote:
> > drop timestamp initialization. It has a default constructor.
> 
> Done.

lgtm.

[jochen (gone - plz use gerrit), 2016-07-06 11:23:15]

lgtm

[lpy, 2016-07-06 18:14:54]

The CQ bit was checked by lpy@chromium.org

[lpy, 2016-07-06 18:14:54]

The patchset sent to the CQ was uploaded after l-g-t-m from alph@chromium.org,
jochen@chromium.org, yangguo@chromium.org
Link to the patchset: https://codereview.chromium.org/2105943002/#ps220001
(title: "rebase")

[commit-bot: I haz the power, 2016-07-06 18:14:57]

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-06 18:37:17]

Committed patchset #9 (id:220001)

[commit-bot: I haz the power, 2016-07-06 18:40:39]

Description was changed from

==========
Expose TickSample and its APIs in v8-profiler.h

We want to eventually move the profiling functionality out of V8 as library,
this patch exposes TickSample and its APIs in v8-profiler.h so that when
embedders use library, they can have more details.

Minor change: Rename tick-sample.[h|cc] to simulator-helper.[h|cc].

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

to

==========
Expose TickSample and its APIs in v8-profiler.h

We want to eventually move the profiling functionality out of V8 as library,
this patch exposes TickSample and its APIs in v8-profiler.h so that when
embedders use library, they can have more details.

Minor change: Rename tick-sample.[h|cc] to simulator-helper.[h|cc].

BUG=v8:4789
LOG=N

Committed: https://crrev.com/3172f6a9ce72e7318dd55239677dd542a3fab9e6
Cr-Commit-Position: refs/heads/master@{#37564}
==========

[commit-bot: I haz the power, 2016-07-06 18:40:40]

Patchset 9 (id:??) landed as
https://crrev.com/3172f6a9ce72e7318dd55239677dd542a3fab9e6
Cr-Commit-Position: refs/heads/master@{#37564}