Initial implementation of GetStackSample sampling profiler API.

Initial implementation of GetStackSample sampling profiler API.

The patch is based on https://codereview.chromium.org/578163002/#ps20001
made by gholap@chromium.org

LOG=N
BUG=v8:3490
R=bmeurer@chromium.org, yurys@chromium.org

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

[alph, 2014-09-22 19:18:48]

alph@chromium.org changed reviewers:
+ bmeurer@chromium.org, loislo@chromium.org, svenpanne@chromium.org,
yurys@chromium.org

[gholap, 2014-09-23 05:39:18]

gholap@chromium.org changed reviewers:
+ gholap@chromium.org

[gholap, 2014-09-23 05:39:18]

https://codereview.chromium.org/596533002/diff/1/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/596533002/diff/1/test/cctest/cctest.status#ne...
test/cctest/cctest.status:355: 'test-sampler-api/*': [SKIP],
https://codereview.chromium.org/588623002/ uses POSIX signals for the tests.
Still arm builds were failing. Passes MacOS (MacOS means osx, right?) though.

[gholap, 2014-09-23 05:40:16]

On 2014/09/23 05:39:18, gholap wrote:
> https://codereview.chromium.org/596533002/diff/1/test/cctest/cctest.status
> File test/cctest/cctest.status (right):
> 
>
https://codereview.chromium.org/596533002/diff/1/test/cctest/cctest.status#ne...
> test/cctest/cctest.status:355: 'test-sampler-api/*': [SKIP],
> https://codereview.chromium.org/588623002/ uses POSIX signals for the tests.
> Still arm builds were failing. Passes MacOS (MacOS means osx, right?) though.

(patch set 2)

[noordhuis, 2014-09-23 05:44:35]

https://codereview.chromium.org/596533002/diff/1/test/cctest/test-sampler-api.cc
File test/cctest/test-sampler-api.cc (right):

https://codereview.chromium.org/596533002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:249: return;
There is a getcontext() on OS X but you have to build with _XOPEN_SOURCE
defined, something V8 doesn't do, I think.

[Benedikt Meurer, 2014-09-23 05:47:48]

https://codereview.chromium.org/596533002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/596533002/diff/1/include/v8.h#newcode4568
include/v8.h:4568: int GetStackSample(const RegisterState& state, void** frames,
Nit: int -> size_t

https://codereview.chromium.org/596533002/diff/1/include/v8.h#newcode4569
include/v8.h:4569: int frames_limit);
Nit: int -> size_t

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

https://codereview.chromium.org/596533002/diff/1/src/sampler.cc#newcode619
src/sampler.cc:619: int TickSample::GetStackSample(Isolate* isolate, const
v8::RegisterState& regs,
Nit: int -> size_t

https://codereview.chromium.org/596533002/diff/1/src/sampler.cc#newcode620
src/sampler.cc:620: void** frames, int frames_limit) {
Nit: int -> size_t

https://codereview.chromium.org/596533002/diff/1/src/sampler.cc#newcode630
src/sampler.cc:630: int i = 0;
Nit: int -> size_t

[alph, 2014-09-23 07:30:46]

ptal

https://codereview.chromium.org/596533002/diff/1/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/596533002/diff/1/include/v8.h#newcode4568
include/v8.h:4568: int GetStackSample(const RegisterState& state, void** frames,
On 2014/09/23 05:47:48, Benedikt Meurer wrote:
> Nit: int -> size_t

Done.
I just saw v8 tends to use int whenever possible.

https://codereview.chromium.org/596533002/diff/1/include/v8.h#newcode4569
include/v8.h:4569: int frames_limit);
On 2014/09/23 05:47:48, Benedikt Meurer wrote:
> Nit: int -> size_t

Done.

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

https://codereview.chromium.org/596533002/diff/1/src/sampler.cc#newcode619
src/sampler.cc:619: int TickSample::GetStackSample(Isolate* isolate, const
v8::RegisterState& regs,
On 2014/09/23 05:47:48, Benedikt Meurer wrote:
> Nit: int -> size_t

Done.

https://codereview.chromium.org/596533002/diff/1/src/sampler.cc#newcode620
src/sampler.cc:620: void** frames, int frames_limit) {
On 2014/09/23 05:47:48, Benedikt Meurer wrote:
> Nit: int -> size_t

Done.

https://codereview.chromium.org/596533002/diff/1/src/sampler.cc#newcode630
src/sampler.cc:630: int i = 0;
On 2014/09/23 05:47:48, Benedikt Meurer wrote:
> Nit: int -> size_t

Done.

https://codereview.chromium.org/596533002/diff/1/test/cctest/cctest.status
File test/cctest/cctest.status (right):

https://codereview.chromium.org/596533002/diff/1/test/cctest/cctest.status#ne...
test/cctest/cctest.status:355: 'test-sampler-api/*': [SKIP],
On 2014/09/23 05:39:18, gholap wrote:
> https://codereview.chromium.org/588623002/ uses POSIX signals for the tests.
> Still arm builds were failing. Passes MacOS (MacOS means osx, right?) though.

Let's address arm tests in a separate patch.

https://codereview.chromium.org/596533002/diff/1/test/cctest/test-sampler-api.cc
File test/cctest/test-sampler-api.cc (right):

https://codereview.chromium.org/596533002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:249: return;
On 2014/09/23 05:44:35, info1 wrote:
> There is a getcontext() on OS X but you have to build with _XOPEN_SOURCE
> defined, something V8 doesn't do, I think.

Yep, I'm not sure v8 wants to use _XOPEN_SOURCE just for this test. So I think
we should emulate getcontext with signals.

[yurys, 2014-09-23 09:32:03]

https://codereview.chromium.org/596533002/diff/60001/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/596533002/diff/60001/src/sampler.cc#newcode623
src/sampler.cc:623: Address js_entry_sp = isolate->js_entry_sp();
I expected that we wouldn't collect call stack if current vm state is GC. I
think we should match TickSample::Init in this case and return current vm state
to indicate that GC is in progress without crawling the stack. Or is it always
safe to collect current stack frames and be prepare that some of the PCs may be
invalid?

[alph, 2014-09-23 11:13:02]

https://codereview.chromium.org/596533002/diff/60001/src/sampler.cc
File src/sampler.cc (right):

https://codereview.chromium.org/596533002/diff/60001/src/sampler.cc#newcode623
src/sampler.cc:623: Address js_entry_sp = isolate->js_entry_sp();
On 2014/09/23 09:32:03, yurys wrote:
> I expected that we wouldn't collect call stack if current vm state is GC. I
> think we should match TickSample::Init in this case and return current vm
state
> to indicate that GC is in progress without crawling the stack. Or is it always
> safe to collect current stack frames and be prepare that some of the PCs may
be
> invalid?

Done.

[yurys, 2014-09-23 14:57:53]

https://codereview.chromium.org/596533002/diff/80001/src/vm-state.h
File src/vm-state.h (right):

https://codereview.chromium.org/596533002/diff/80001/src/vm-state.h#newcode19
src/vm-state.h:19: 
style: remove empty line.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
File test/cctest/test-sampler-api.cc (right):

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:22: using v8::Local;
style nit: we don't usually declare such using in other places in v8, so I'd
rather be consistent with the rest of the codebase.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:83: #define SAMPLER_API_TESTS_BOOTSTRAP()       
                               \
style: I'd use a class rather than a macro here.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:112: int MAX_SIZE = Sample::kFramesLimit;
inline this

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:118: std::vector<v8::JitCodeEvent> inner_funcs;
JitCodeEvent contains Handle<Script>, is it a global or local handle? In the
latter case it may be unsafe to put it into a global vector like this as
corresponding HandleScope may be destroyed before the handle.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:122: if (event->type !=
v8::JitCodeEvent::CODE_ADDED) return;
What about v8::JitCodeEvent::CODE_MOVE? How can we be sure that GC won't move
the code during the test?

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:146: // They should fall in the range where the
compiled code
typo: ... where the compiled code resides.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:166: (int64_t)inner_funcs[i].code_start +
inner_funcs[i].code_len);
static_cast<intptr_t>(inner_funcs[i].code_start) as we don't use c-style cast.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:188: class SimulatorHelper {
I don't think we need to pull all these platform-specific code here. Since we
always call GetSample after exiting into the runtime we can pass address of some
C-function, fp and sp values as the register state, they should be ignored by
the stack iterator as it should take top frame from Isolate::c_entry_fp().

[alph, 2014-09-24 13:12:19]

ptal

https://codereview.chromium.org/596533002/diff/80001/src/vm-state.h
File src/vm-state.h (right):

https://codereview.chromium.org/596533002/diff/80001/src/vm-state.h#newcode19
src/vm-state.h:19: 
On 2014/09/23 14:57:53, yurys wrote:
> style: remove empty line.

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
File test/cctest/test-sampler-api.cc (right):

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:22: using v8::Local;
On 2014/09/23 14:57:53, yurys wrote:
> style nit: we don't usually declare such using in other places in v8, so I'd
> rather be consistent with the rest of the codebase.

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:83: #define SAMPLER_API_TESTS_BOOTSTRAP()       
                               \
On 2014/09/23 14:57:53, yurys wrote:
> style: I'd use a class rather than a macro here.

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:112: int MAX_SIZE = Sample::kFramesLimit;
On 2014/09/23 14:57:53, yurys wrote:
> inline this

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:118: std::vector<v8::JitCodeEvent> inner_funcs;
On 2014/09/23 14:57:53, yurys wrote:
> JitCodeEvent contains Handle<Script>, is it a global or local handle? In the
> latter case it may be unsafe to put it into a global vector like this as
> corresponding HandleScope may be destroyed before the handle.

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:122: if (event->type !=
v8::JitCodeEvent::CODE_ADDED) return;
On 2014/09/23 14:57:53, yurys wrote:
> What about v8::JitCodeEvent::CODE_MOVE? How can we be sure that GC won't move
> the code during the test?

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:146: // They should fall in the range where the
compiled code
On 2014/09/23 14:57:53, yurys wrote:
> typo: ... where the compiled code resides.

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:166: (int64_t)inner_funcs[i].code_start +
inner_funcs[i].code_len);
On 2014/09/23 14:57:53, yurys wrote:
> static_cast<intptr_t>(inner_funcs[i].code_start) as we don't use c-style cast.

Done.

https://codereview.chromium.org/596533002/diff/80001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:188: class SimulatorHelper {
On 2014/09/23 14:57:53, yurys wrote:
> I don't think we need to pull all these platform-specific code here. Since we
> always call GetSample after exiting into the runtime we can pass address of
some
> C-function, fp and sp values as the register state, they should be ignored by
> the stack iterator as it should take top frame from Isolate::c_entry_fp().

Done.

[yurys, 2014-09-24 13:44:07]

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
File test/cctest/test-sampler-api.cc (right):

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:10: #include "src/simulator.h"
Do we still need this include?

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:42: DCHECK_EQ(NULL, instance_);
I'd rather inline this check and singlton implementation into SamplingTestHelper

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:80: Sample& get_sample() { return sample_; }
style: no  get_ prefix on simple getters.

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:87: void DoCollectSample() {
This should be private

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:105: switch (event->type) {
We should ignore code events after the sample is taken.

[alph, 2014-09-24 14:00:52]

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
File test/cctest/test-sampler-api.cc (right):

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:10: #include "src/simulator.h"
On 2014/09/24 13:44:07, yurys wrote:
> Do we still need this include?

Done.

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:42: DCHECK_EQ(NULL, instance_);
On 2014/09/24 13:44:07, yurys wrote:
> I'd rather inline this check and singlton implementation into
SamplingTestHelper

Done.

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:80: Sample& get_sample() { return sample_; }
On 2014/09/24 13:44:07, yurys wrote:
> style: no  get_ prefix on simple getters.

Done.

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:87: void DoCollectSample() {
On 2014/09/24 13:44:07, yurys wrote:
> This should be private

Done.

https://codereview.chromium.org/596533002/diff/100001/test/cctest/test-sample...
test/cctest/test-sampler-api.cc:105: switch (event->type) {
On 2014/09/24 13:44:07, yurys wrote:
> We should ignore code events after the sample is taken.

Nice catch! Done.

[yurys, 2014-09-24 14:04:15]

lgtm

[loislo, 2014-09-24 14:41:29]

https://codereview.chromium.org/596533002/diff/140001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/596533002/diff/140001/include/v8.h#newcode1434
include/v8.h:1434: size_t frames_count;
Can we join together both output structures i.e. the array of frames and
SampleInfo?

The motivation: the embedder wouldn't be able to do a synchronous post
processing for these data because the restrictions for signal handler.
So with this API the embedder has to have two preallocated buffers. one for
frames and the other for SampleInfo with frames count and needs to keep them in
sync.

It could be done the old school way:

struct SampleInfo {
  StateTag vm_state;
  size_t frames_count;
  void* frames[1]; // the actual used size depends on frames_count.
}

[alph, 2014-09-24 14:57:39]

https://codereview.chromium.org/596533002/diff/140001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/596533002/diff/140001/include/v8.h#newcode1434
include/v8.h:1434: size_t frames_count;
On 2014/09/24 14:41:29, loislo wrote:
> Can we join together both output structures i.e. the array of frames and
> SampleInfo?
> 
> The motivation: the embedder wouldn't be able to do a synchronous post
> processing for these data because the restrictions for signal handler.
> So with this API the embedder has to have two preallocated buffers. one for
> frames and the other for SampleInfo with frames count and needs to keep them
in
> sync.
> 
> It could be done the old school way:
> 
> struct SampleInfo {
>   StateTag vm_state;
>   size_t frames_count;
>   void* frames[1]; // the actual used size depends on frames_count.
> }

It would be quite difficult for the embedder to convert (without copying) from
SampleInfo if it chooses to store the data in a different format.
E.g. if frames are stored in one buffer and the rest is in another.

The current format seems to be more flexible, as it allows the embedder to store
frames wherever it wants.

[Benedikt Meurer, 2014-09-25 06:08:39]

Tests are failing on ARM?

[Benedikt Meurer, 2014-09-25 06:09:06]

bmeurer@chromium.org changed reviewers:
+ danno@chromium.org

[alph, 2014-09-25 14:05:12]

On 2014/09/25 06:08:39, Benedikt Meurer wrote:
> Tests are failing on ARM?

Fixed.
Simulator is probably managing stack at some distinct location, so passing a raw
sp just didn't work.

ptal

[Benedikt Meurer, 2014-09-26 04:48:16]

lgtm

[alph, 2014-09-29 13:00:23]

Committed patchset #11 (id:200001) manually as 24285 (presubmit successful).