Implemented GetSample.

Implemented GetSample.

DESIGN_DOCS:
- https://docs.google.com/a/google.com/document/d/11Fl6Wb83eqcN0G9kTY0UEGHmHSQMDEYR0aYqLvqrudw/edit
- https://docs.google.com/a/chromium.org/document/d/1TiY-DFwkRTT0EiOijg2aTPrt2AuZwGCCP8UfKLB-ogI/edit

BUG=v8:3490
LOG=N

[gholap, 2014-09-18 03:25:48]

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

[gholap, 2014-09-18 03:27:59]

Implemented GetSample.
- It doesn't have any thread-specific stuff
- A test to go along, demonstrating a very simple use case.

Please take a look :)

DESIGN_DOCS:
-
https://docs.google.com/a/google.com/document/d/11Fl6Wb83eqcN0G9kTY0UEGHmHSQM...
-
https://docs.google.com/a/chromium.org/document/d/1TiY-DFwkRTT0EiOijg2aTPrt2A...

[alph, 2014-09-18 11:34:29]

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

https://codereview.chromium.org/578163002/diff/1/src/api.cc#newcode6779
src/api.cc:6779: void Isolate::GetSample(void* sp, void* fp, Sample* sample) {
What about pc register? It is essential to determine the top level function.
TickSample::Init makes use of it.

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

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:101: // Note: The argumnets.callee stuff is
there so that the
typo

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:150: CHECK_EQ(true, stack_top_is_inner);
CHECK

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:250: void GetStackAndFramePointers(void** sp,
void** fp) {
you can move the function name & args out of #ifdef since it does not depend on
the platform.

[loislo, 2014-09-18 13:07:27]

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

[loislo, 2014-09-18 13:07:28]

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

https://codereview.chromium.org/578163002/diff/1/include/v8.h#newcode1428
include/v8.h:1428: Sample(InputIterator first, InputIterator last)
Looks like it should be a v8 internal constructor or method.
Also it is not clear why do you use template here.

[loislo, 2014-09-18 14:42:44]

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

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:155: // Platform specific implementation of
GetStackAndFramePointers
this is doesn't look as a good idea.

I mean we have platform depended code in TickSample and
put another portion of the platform depended code into the embedder.

[gholap, 2014-09-18 23:24:15]

Patch 2 Address all concerns except:
Getting the RegisterState from CONTEXT.
Added patch 3 for that.

I would love to hear whether it is an appropriate solution or we should just go
with patch 2.

Please take a look :)

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

https://codereview.chromium.org/578163002/diff/1/include/v8.h#newcode1428
include/v8.h:1428: Sample(InputIterator first, InputIterator last)
On 2014/09/18 13:07:28, loislo wrote:
> Looks like it should be a v8 internal constructor or method.
> Also it is not clear why do you use template here.

We start with: We don't want to expose the private variables.

If we don't have this constructor, we'd need to add v8's internal classes (which
make / fill up a sample) as friend classes. But we shouldn't add internal stuff
in include/v8.h, right?

Also, we don't really mind if the embedder knows how to construct a sample,
actually, it provides a way of testing the sample without exposing the
implementation details (whether it has an array inside it, or whether it is a
vector)

Referring to: https://codereview.chromium.org/422593003/diff/420001/include/v8.h
(Benedikt's first comment on the diff)

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

https://codereview.chromium.org/578163002/diff/1/src/api.cc#newcode6779
src/api.cc:6779: void Isolate::GetSample(void* sp, void* fp, Sample* sample) {
On 2014/09/18 11:34:29, alph wrote:
> What about pc register? It is essential to determine the top level function.
> TickSample::Init makes use of it.

Done.

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

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:101: // Note: The argumnets.callee stuff is
there so that the
On 2014/09/18 11:34:29, alph wrote:
> typo

Done.

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:150: CHECK_EQ(true, stack_top_is_inner);
On 2014/09/18 11:34:29, alph wrote:
> CHECK

Done.

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:155: // Platform specific implementation of
GetStackAndFramePointers
On 2014/09/18 14:42:44, loislo wrote:
> this is doesn't look as a good idea.
> 
> I mean we have platform depended code in TickSample and
> put another portion of the platform depended code into the embedder.

Oh! I did not know that...
I checked the TickSample code (especially, the TickSample::Init) and I
could not find platform dependent code...
I'm missing something? :(

Also, with the GetSample API that we want (give it the register
values and a paused isolate, and it gives back the stack), I'm not
sure how we could avoid making the embedder platform agnostic.
Because, the embedder needs to pass the registers and it has no
way of getting them in a platform independent manner. (unless we
provide an API for doing that)

https://codereview.chromium.org/578163002/diff/1/test/cctest/test-sampler-api...
test/cctest/test-sampler-api.cc:250: void GetStackAndFramePointers(void** sp,
void** fp) {
On 2014/09/18 11:34:29, alph wrote:
> you can move the function name & args out of #ifdef since it does not depend
on
> the platform.

Done.

[yurys, 2014-09-19 08:08:12]

https://codereview.chromium.org/578163002/diff/60001/include/v8.h
File include/v8.h (right):

https://codereview.chromium.org/578163002/diff/60001/include/v8.h#newcode64
include/v8.h:64: /* #if !defined(__MINGW32__) ||
defined(__MINGW64_VERSION_MAJOR) */
We shouldn't expose all this platform-specific stuff in v8.h. All of the can be
handled on the embedder side. Look at  src/sampler.h - it doesn't include any of
this
(https://code.google.com/p/chromium/codesearch#chromium/src/v8/src/sampler.h&q...).

https://codereview.chromium.org/578163002/diff/60001/include/v8.h#newcode1435
include/v8.h:1435: explicit RegisterState(const context_t& context);
This is a platform-specific constructor, please don't expose it in the API (see
above).

https://codereview.chromium.org/578163002/diff/60001/include/v8.h#newcode4603
include/v8.h:4603: * \param sp Stack pointer.
There are not such params, they are passed as |state|

https://codereview.chromium.org/578163002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/578163002/diff/60001/src/api.cc#newcode6630
src/api.cc:6630: #if V8_OS_POSIX && !V8_OS_CYGWIN
One of the goals of having sampler API is to eventually move all this code out
of v8. I'm not sure yet whether it should be moved into Blink, Chromium or some
third-party project as other clients (e.g. Node.js) may want it as well. In any
case with this approach we end up with one more copy of this platform-dependent
block inside v8 which moves us in the opposite direction.

https://codereview.chromium.org/578163002/diff/60001/src/sampler.h
File src/sampler.h (left):

https://codereview.chromium.org/578163002/diff/60001/src/sampler.h#oldcode24
src/sampler.h:24: struct RegisterState {
I would expect this struct to be moved into v8.h almost as is without adding new
(platform-specific) constructors.

[loislo, 2014-09-19 08:58:16]

looks like I was a bit wrong in my previous comment about platform depended
code. I'm sorry. The only thing I'd like to see is that we keep all such code in
one place.

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

https://codereview.chromium.org/578163002/diff/1/include/v8.h#newcode1428
include/v8.h:1428: Sample(InputIterator first, InputIterator last)
On 2014/09/18 23:24:14, gholap wrote:
> On 2014/09/18 13:07:28, loislo wrote:
> > Looks like it should be a v8 internal constructor or method.
> > Also it is not clear why do you use template here.
> 
> We start with: We don't want to expose the private variables.
> 
> If we don't have this constructor, we'd need to add v8's internal classes
(which
> make / fill up a sample) as friend classes. But we shouldn't add internal
stuff
> in include/v8.h, right?
> 
> Also, we don't really mind if the embedder knows how to construct a sample,
> actually, it provides a way of testing the sample without exposing the
> implementation details (whether it has an array inside it, or whether it is a
> vector)
> 
> Referring to:
https://codereview.chromium.org/422593003/diff/420001/include/v8.h
> (Benedikt's first comment on the diff)

Got it. The only thing that still bother me is the fact that when we fill the
sample from linux signal handler we actually can't use allocator. I.e. Sample
and its buffer need to be preallocated. Otherwise we will get a deadlock. And in
this case I'd prefer a template method fill just because it is semantically
clearer.

https://codereview.chromium.org/578163002/diff/60001/src/api.cc
File src/api.cc (right):

https://codereview.chromium.org/578163002/diff/60001/src/api.cc#newcode6910
src/api.cc:6910: tick_sample.Init(isolate, state);
can we pass sample into Init method and fill it there directly instead of
copying the addresses from TickSample to Sample?

[alph, 2014-09-19 11:43:05]

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

https://codereview.chromium.org/578163002/diff/60001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:47: }
nit:
}  // namespace

https://codereview.chromium.org/578163002/diff/60001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:109: }
ditto

https://codereview.chromium.org/578163002/diff/60001/test/cctest/test-sampler...
test/cctest/test-sampler-api.cc:169: }
ditto

[yurys, 2014-09-19 13:48:21]

We discussed this over VC today and agreed that the API should not use
plarform-specific #ifdeds and should accept a pointer to a raw output buffer and
return the number of actually captured frames. So it would look like this:

int GetSample(const RegisterState& state, void** frames, int length);

There are 2 advantages of such approach:
1) It doesn't impose additional constraints on the embedder's structure (as
opposed to having Sample class).
2) There is a simple way for the embedder to control maximal sample size using
length param.

[Sven Panne, 2014-11-25 11:43:40]

svenpanne@chromium.org changed reviewers:
- svenpanne@chromium.org

[yurys, 2014-11-25 12:18:59]

Closing as obsolete. See https://codereview.chromium.org/596533002