Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
422593003:
Initial GetSample implementation. (Closed)
DescriptionInitial GetSample implementation.
BUG=v8:3490
LOG=N
Patch Set 1 #Patch Set 2 : Moved the thread pausing logic inside Sampler::GetSample #
Total comments: 1
Patch Set 3 : Killed max_frame_count argument. #Patch Set 4 : Moved initialization of sample from the signal emitter to sigal handler thread. #
Total comments: 12
Patch Set 5 : Added TODOs, removed trailing underscores from var names. #Patch Set 6 : Added TODOs, removed trailing underscores from var names. #Patch Set 7 : Oops. The previous commit was broken. #Patch Set 8 : Frames count was a bit-field had missed that previously. Now the API actually works. #
Total comments: 18
Patch Set 9 : Addressed the comments. #
Total comments: 17
Patch Set 10 : Addressed some comments. memory barrier and semaphore_signal still WIP. #
Total comments: 4
Patch Set 11 : Addressed the nits. #
Total comments: 2
Patch Set 12 : Fixed typo. Made sure SIGPROF handler is always installed. Added a test. #
Total comments: 2
Patch Set 13 : Increased loop length. Changed assignment to addition to subvert optimization. #Patch Set 14 : Time based loop. Added sample count. Removed the sleep(0). #Patch Set 15 : Added synchronization to sampler api test. #
Total comments: 28
Patch Set 16 : Addressed the comments. Fixed the nits. #
Total comments: 2
Patch Set 17 : Fixed variable naming nits. #
Total comments: 4
Patch Set 18 : Removed Address from public API. Removed the function to copy TickSample to Sample. #
Total comments: 14
Patch Set 19 : reinterpret_cast -> static_cast. Also addressed other comments. #Patch Set 20 : Simplified profile-generator for loop. #Patch Set 21 : Added test to verify the pc values in obtained sample. #
Total comments: 6
Patch Set 22 : unsigned -> size_t and kMaxFramesCount is in an enum now as opposed to a compile time constant. #Patch Set 23 : Made the Sample class into an iterable. #
Total comments: 16
Patch Set 24 : Addressed nits. Built and tested on windows. #Patch Set 25 : Rebased on bleeding_edge #
Total comments: 17
Patch Set 26 : Moved thread logic out of GetSample #
Total comments: 5
Messages
Total messages: 59 (4 generated)
I have done a quick first pass. https://codereview.chromium.org/422593003/diff/20001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/20001/include/v8-sampler.h#new... include/v8-sampler.h:62: unsigned max_frame_count, I would suggest removing that, currently the stack size is at 255 which is small enough. https://codereview.chromium.org/422593003/diff/60001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/422593003/diff/60001/src/api.cc#newcode7440 src/api.cc:7440: if (sampler_ == NULL) return NULL; Can you cache those between calls? i.e. check and store, start with sampler_ all the way back. Also I think _ indicates private, with local variables do not use _ (double check the coding guidelines). https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode298 src/sampler.cc:298: Delete the added line. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode325 src/sampler.cc:325: // and return just one sample instead, rightaway. Add a TODO that we will eventually have GetSample, once the CPU Profiler will be external as well. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode335 src/sampler.cc:335: static base::Semaphore* new_sample_semaphore_; I would call it, sampling_semaphore instead. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode341 src/sampler.cc:341: static bool new_sample_available_; just sample_available_ https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode372 src/sampler.cc:372: #define SIGNAL_AND_RETURN(GOT_NEW) \ Explain why this macro is needed.
ptal https://codereview.chromium.org/422593003/diff/60001/src/api.cc File src/api.cc (right): https://codereview.chromium.org/422593003/diff/60001/src/api.cc#newcode7440 src/api.cc:7440: if (sampler_ == NULL) return NULL; On 2014/07/29 18:30:50, fmeawad-cr wrote: > Can you cache those between calls? > i.e. check and store, start with sampler_ all the way back. > > Also I think _ indicates private, with local variables do not use _ (double > check the coding guidelines). Acknowledged. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode298 src/sampler.cc:298: On 2014/07/29 18:30:50, fmeawad-cr wrote: > Delete the added line. Done. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode325 src/sampler.cc:325: // and return just one sample instead, rightaway. On 2014/07/29 18:30:50, fmeawad-cr wrote: > Add a TODO that we will eventually have GetSample, once the CPU Profiler will be > external as well. Done. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode335 src/sampler.cc:335: static base::Semaphore* new_sample_semaphore_; On 2014/07/29 18:30:50, fmeawad-cr wrote: > I would call it, sampling_semaphore instead. Done. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode341 src/sampler.cc:341: static bool new_sample_available_; On 2014/07/29 18:30:50, fmeawad-cr wrote: > just sample_available_ Done. https://codereview.chromium.org/422593003/diff/60001/src/sampler.cc#newcode372 src/sampler.cc:372: #define SIGNAL_AND_RETURN(GOT_NEW) \ On 2014/07/29 18:30:50, fmeawad-cr wrote: > Explain why this macro is needed. Done.
This is the first CL towards moving the cpu-profiler out of v8 by creating an API for getting samples. Quite a bit of temporary stuff here, to keep the CL in a reasonable size. (As per the discussion in the design-doc): https://docs.google.com/a/google.com/document/d/11Fl6Wb83eqcN0G9kTY0UEGHmHSQM... please have a look and let me know if this seems a reasonable starting point?
I'm not sure I'm the right guy to do this review, since both Sven and Benedikt seem more familiar with signal handlers and profiling. I have some comments. https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:28: IDLE // The VM is idle. v8::internal::VMState scope still uses the StateTag in globals.h, which should mirror this. Can we only have this one copy, or at least STATIC_ASSERT that this is in sync? https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:50: unsigned frames_count : kMaxFramesCountLog2; // Number of captured frames. Casting between Sample and TickSample seems very fragile to me. Can we subclass TickSample from Sample? https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode383 src/sampler.cc:383: return; \ Using a scope, which signals in its destructor, seems a cleaner solution. https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode759 src/sampler.cc:759: SignalHandler::called_from_get_sample_ = true; Don't we need a memory barrier here? Is it guaranteed that this value change is observed by the signal handler? https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode763 src/sampler.cc:763: sample->state = SignalHandler::sample_.state; Why doesn't GetSample simply set a static pointer for the return value, and the signal handler calls Init on that pointer? We would get away without copying everything. If we really have to copy, why not copy constructor? https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode777 src/sampler.cc:777: TickSample* Sampler::GetSampleHelper_(TickSample * sample, Please omit that _ in the method name.
https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:17: typedef unsigned char* Address; I don't think we need this type at all. See below. https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:49: Address stack[kMaxFramesCount]; // Call stack. Use const void* instead of Address. https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode759 src/sampler.cc:759: SignalHandler::called_from_get_sample_ = true; At the very least, this has to be volatile.
I've addressed the comments, please let me know how it looks like... Also, I'll try to sync up with Munich time so that back-and-forth can be reduced... https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:17: typedef unsigned char* Address; On 2014/08/13 06:29:11, Benedikt Meurer wrote: > I don't think we need this type at all. See below. Yang suggested that we subclass the internal TickSample struct from the Sample struct defined in here, that would address the flimsy and fragile nature of the casting issue. On the other hand, that means we would need to keep the Sample::stack as Address... What would you suggest? https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:28: IDLE // The VM is idle. On 2014/08/12 14:21:35, Yang wrote: > v8::internal::VMState scope still uses the StateTag in > globals.h, which should mirror this. > Can we only have this one copy? Yes, I moved the internal StateTag from globals.h to v8.h and now that's the only place now this enum exists. Let me know how that sounds. https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:49: Address stack[kMaxFramesCount]; // Call stack. On 2014/08/13 06:29:11, Benedikt Meurer wrote: > Use const void* instead of Address. Acknowledged. https://codereview.chromium.org/422593003/diff/140001/include/v8-sampler.h#ne... include/v8-sampler.h:50: unsigned frames_count : kMaxFramesCountLog2; // Number of captured frames. On 2014/08/12 14:21:36, Yang wrote: > Casting between Sample and TickSample seems very fragile to me. Can we subclass > TickSample from Sample? Done. https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode383 src/sampler.cc:383: return; \ On 2014/08/12 14:21:36, Yang wrote: > Using a scope, which signals in its destructor, seems a cleaner solution. Done. https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode759 src/sampler.cc:759: SignalHandler::called_from_get_sample_ = true; On 2014/08/13 06:29:11, Benedikt Meurer wrote: > At the very least, this has to be volatile. Done. https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode759 src/sampler.cc:759: SignalHandler::called_from_get_sample_ = true; On 2014/08/12 14:21:36, Yang wrote: > Don't we need a memory barrier here? Is it guaranteed that this value change is > observed by the signal handler? Yes. Made it volatile. This would just be a transient issue, though. Once the API is done, the only thing that's going to send the signal is GetSample, eliminating the internal code-path, and the need for this variable. https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode763 src/sampler.cc:763: sample->state = SignalHandler::sample_.state; On 2014/08/12 14:21:36, Yang wrote: > Why doesn't GetSample simply set a static pointer for the return value, and the signal handler calls Init on that pointer? We would get away without copying everything.If we really have to copy, why not copy constructor? Let's call the code that uses the API as "consumer". GetSample takes a Sample* for which, the memory is allocated and owned by the consumer. We don't want to have an Init method in Sample class because it doesn't make sense for the consumer. We don't want to mirror the Sample class internally with an additional Init method because of the fragility issue. Also, as memories for both TickSample and Sample that we are dealing with here are pre-allocated, a copy constructor won't work either. Moreover, as we flesh out the API and make it more and more comprehensive, we'll eventually get rid of TickSample and the copying logic... https://codereview.chromium.org/422593003/diff/140001/src/sampler.cc#newcode777 src/sampler.cc:777: TickSample* Sampler::GetSampleHelper_(TickSample * sample, On 2014/08/12 14:21:36, Yang wrote: > Please omit that _ in the method name. Done.
https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h#ne... include/v8-sampler.h:16: typedef unsigned char* Address; We don't need this typedef at all. Just use const void* for pc values. https://codereview.chromium.org/422593003/diff/160001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8.h#newcode6700 include/v8.h:6700: enum StateTag { Move this to v8-sampler.h and fix the comment (VMState is an internal thing, no need to document that in a public header). https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode274 src/sampler.cc:274: if (!mutex_) mutex_ = new base::Mutex(); CHECK that both mutex_ and sampling_semaphore_ are NULL initially. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode280 src/sampler.cc:280: delete mutex_; Reset mutex_ and sampling_semaphore_ to NULL. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode329 src/sampler.cc:329: static TickSample sample_; I'm not 100%, but I guess sample_ will also have to be marked as volatile. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode375 src/sampler.cc:375: ScopedSemaphore scoped_semaphore(sampling_semaphore_); I'm not sure it's safe to call semaphore_signal() in a POSIX signal handler (on OS X). On Linux we use sem_post() which is async-signal safe.
Addressed most the comments and nits. Memory barrier and semaphore_signal still WIP. (I'm figuring out, but a bit clueless...) https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h#ne... include/v8-sampler.h:16: typedef unsigned char* Address; On 2014/08/14 04:20:23, Benedikt Meurer wrote: > We don't need this typedef at all. Just use const void* for pc values. As I mentioned in the reply to your earlier comment, as we are subclassing TickSample from Sample, we need the stack to be const char* for the code to even compile. Let's discuss this on a hangout? https://codereview.chromium.org/422593003/diff/160001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8.h#newcode6700 include/v8.h:6700: enum StateTag { On 2014/08/14 04:20:23, Benedikt Meurer wrote: > Move this to v8-sampler.h and fix the comment >(VMState is an internal thing, no > need to document that in a public header). Now that you've mentioned it, I've looked into both devtools and trace viewer code and they don't use the state. So, after a discussion with fmeawad, I removed this from public API altogether. Please let me know whether this is a bad idea... https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode274 src/sampler.cc:274: if (!mutex_) mutex_ = new base::Mutex(); On 2014/08/14 04:20:23, Benedikt Meurer wrote: > CHECK that both mutex_ and sampling_semaphore_ are NULL initially. Done. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode280 src/sampler.cc:280: delete mutex_; On 2014/08/14 04:20:23, Benedikt Meurer wrote: > Reset mutex_ and sampling_semaphore_ to NULL. Done.
just checked semaphore_signal on xnu man pages. It is safe to call it even from inside of an interrupt handler. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode375 src/sampler.cc:375: ScopedSemaphore scoped_semaphore(sampling_semaphore_); On 2014/08/14 04:20:23, Benedikt Meurer wrote: > I'm not sure it's safe to call semaphore_signal() in a POSIX signal handler (on > OS X). > On Linux we use sem_post() which is async-signal safe. Yes, according to the xnu man pages, (http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/semaphore_signal.html) it is safe.
LGTM with nits. https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h#ne... include/v8-sampler.h:16: typedef unsigned char* Address; On 2014/08/15 17:54:13, gholap wrote: > On 2014/08/14 04:20:23, Benedikt Meurer wrote: > > We don't need this typedef at all. Just use const void* for pc values. > > As I mentioned in the reply to your earlier comment, as we are subclassing > TickSample from Sample, we need the stack to be const char* for the code to even > compile. Let's discuss this on a hangout? Got it. But please add a TODO to the typedef, that this should become const void* at some point. (it's unsigned char* in the public API because of some implementation detail) https://codereview.chromium.org/422593003/diff/160001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8.h#newcode6700 include/v8.h:6700: enum StateTag { Great! We should not expose internal state via the public API. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode274 src/sampler.cc:274: if (!mutex_) mutex_ = new base::Mutex(); On 2014/08/15 17:54:13, gholap wrote: > On 2014/08/14 04:20:23, Benedikt Meurer wrote: > > CHECK that both mutex_ and sampling_semaphore_ are NULL initially. > > Done. Acknowledged. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode280 src/sampler.cc:280: delete mutex_; On 2014/08/15 17:54:13, gholap wrote: > On 2014/08/14 04:20:23, Benedikt Meurer wrote: > > Reset mutex_ and sampling_semaphore_ to NULL. > > Done. Acknowledged. https://codereview.chromium.org/422593003/diff/160001/src/sampler.cc#newcode375 src/sampler.cc:375: ScopedSemaphore scoped_semaphore(sampling_semaphore_); On 2014/08/15 22:05:19, gholap wrote: > On 2014/08/14 04:20:23, Benedikt Meurer wrote: > > I'm not sure it's safe to call semaphore_signal() in a POSIX signal handler > (on > > OS X). > > On Linux we use sem_post() which is async-signal safe. > > Yes, according to the xnu man pages, > (http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/semaphore_signal.html) it > is safe. Acknowledged. https://codereview.chromium.org/422593003/diff/180001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/180001/src/sampler.cc#newcode274 src/sampler.cc:274: CHECK(mutex_ == NULL); Nit: DCHECK_EQ(NULL, mutex_) https://codereview.chromium.org/422593003/diff/180001/src/sampler.cc#newcode275 src/sampler.cc:275: CHECK(sampling_semaphore_ == NULL); Nit: DCHECK_EQ(NULL, sampling_semaphore_)
Fixed the nits. I don't have commiter access, whom could I contact so that they could commit this? https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/160001/include/v8-sampler.h#ne... include/v8-sampler.h:16: typedef unsigned char* Address; On 2014/08/18 04:22:03, Benedikt Meurer wrote: > On 2014/08/15 17:54:13, gholap wrote: > > On 2014/08/14 04:20:23, Benedikt Meurer wrote: > > > We don't need this typedef at all. Just use const void* for pc values. > > > > As I mentioned in the reply to your earlier comment, as we are subclassing > > TickSample from Sample, we need the stack to be const char* for the code to > even > > compile. Let's discuss this on a hangout? > > Got it. But please add a TODO to the typedef, that this should become const > void* at some point. (it's unsigned char* in the public API because of some > implementation detail) Acknowledged. https://codereview.chromium.org/422593003/diff/180001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/180001/src/sampler.cc#newcode274 src/sampler.cc:274: CHECK(mutex_ == NULL); On 2014/08/18 04:22:04, Benedikt Meurer wrote: > Nit: DCHECK_EQ(NULL, mutex_) Done. https://codereview.chromium.org/422593003/diff/180001/src/sampler.cc#newcode275 src/sampler.cc:275: CHECK(sampling_semaphore_ == NULL); On 2014/08/18 04:22:04, Benedikt Meurer wrote: > Nit: DCHECK_EQ(NULL, sampling_semaphore_) Done.
On 2014/08/18 at 19:52:00, gholap wrote: > Fixed the nits. I don't have commiter access, whom could I contact so that they could commit this? You should talk to Fadi.
On 2014/08/19 03:45:12, Benedikt Meurer wrote: > On 2014/08/18 at 19:52:00, gholap wrote: > > Fixed the nits. I don't have commiter access, whom could I contact so that > they could commit this? > > You should talk to Fadi. I'll land this once the tree is open.
On 2014/08/19 08:06:24, Yang wrote: > On 2014/08/19 03:45:12, Benedikt Meurer wrote: > > On 2014/08/18 at 19:52:00, gholap wrote: > > > Fixed the nits. I don't have commiter access, whom could I contact so that > > they could commit this? > > > > You should talk to Fadi. > > I'll land this once the tree is open. Hm. I was about to land this, but saw that there is no test case covering this at all. Could you please provide one? Thanks.
https://codereview.chromium.org/422593003/diff/200001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/200001/include/v8-sampler.h#ne... include/v8-sampler.h:16: /* TODO(gholap): This should go away and struct Smaple sholud "Sample should"
Added a test. Please have a look :) https://codereview.chromium.org/422593003/diff/200001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/200001/include/v8-sampler.h#ne... include/v8-sampler.h:16: /* TODO(gholap): This should go away and struct Smaple sholud On 2014/08/21 06:04:52, Yang wrote: > "Sample should" Done.
https://codereview.chromium.org/422593003/diff/220001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/220001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:110: TEST(StackDepthIsConsistent) { Please don't add any more of these fragile tests. First of all, the loop above is basically dead-code, i.e. can be replaced with an assignment a = 100000, which neither Crankshaft nor Fullcodegen do right now, but we may do that in the future in Turbofan. Next, even if the loop runs for some time, there's absolutely no warranty, that the OS scheduler allows you to capture a sample with stack height 5. You'll need some kind of synchronization to ensure that the "script thread" is in an expected state, otherwise this test will end up being as useless as the current CPU profiler tests.
https://codereview.chromium.org/422593003/diff/220001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/220001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:110: TEST(StackDepthIsConsistent) { On 2014/08/25 04:24:22, Benedikt Meurer wrote: > Please don't add any more of these fragile tests. First of all, the loop above > is basically dead-code, i.e. can be replaced with an assignment a = 100000, > which neither Crankshaft nor Fullcodegen do right now, but we may do that in the > future in Turbofan. Next, even if the loop runs for some time, there's > absolutely no warranty, that the OS scheduler allows you to capture a sample > with stack height 5. You'll need some kind of synchronization to ensure that the > "script thread" is in an expected state, otherwise this test will end up being > as useless as the current CPU profiler tests. Ah! I see your point. Without synchronization, the test is a fragile one. Also, I didn't realize the loop optimization issue either. The idea was to have a loop big enough so that it runs long enough for the sampler to be able to capture at least a full-depth stack trace. I think, if we do a += i instead of a = i; the optimization issue will be taken care of? Also, about synchronization, I assumed the following: 1) Sampler starts before the JS thread. 2) Sampler ends only after the JS thread is finished executing. 3) If sampler sleeps every one millisecond, and if the JS thread runs for orders of magnitude longer, the sampler is bound to get _some_ sample. Again, because the JS thread spends almost all its time at the full depth, the chances of not hitting any of those samples becomes very very small. Now, after you pointed out, I realize how that logic was flawed. Small or not, any test which relies on probabilities is bad. I'm a bit stuck here at this and it would be really awesome if you could help me out and teach me how to handle such cases...
> Ah! I see your point. Without synchronization, the test is a fragile one. Also, I didn't realize the loop optimization issue either. The idea was to have a loop big enough so that it runs long enough for the sampler to be able to capture at least a full-depth stack trace. I think, if we do a += i instead of a = i; the optimization issue will be taken care of? We could still have an optimization pass that turns the loop into a single addition. > Also, about synchronization, I assumed the following: > 1) Sampler starts before the JS thread. > 2) Sampler ends only after the JS thread is finished executing. > 3) If sampler sleeps every one millisecond, and if the JS thread runs for orders of magnitude longer, the sampler is bound to get _some_ sample. Again, because the JS thread spends almost all its time at the full depth, the chances of not hitting any of those samples becomes very very small. > > Now, after you pointed out, I realize how that logic was flawed. Small or not, any test which relies on probabilities is bad. I'm a bit stuck here at this and it would be really awesome if you could help me out and teach me how to handle such cases... As discussed offline: You need synchronization, otherwise your test will always rely on the OS scheduler to schedule threads the way you expect it.
I changed the test so that it now uses semaphores for synchronization instead of relying on convoluted timer-based logic. Please take a look :)
https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:56: static Sample* GetSample(Isolate* isolate, Looking at the use in the test, I think we should have GetSample return a bool instead. https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:59: } // namespace v8 Nit: Add newline after class declaration. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:1: // Copyright 2014 the V8 project authors. All rights reserved. Nit: Use new short copyright header. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:44: namespace SamplerTester { Use an anonymous namespace instead. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:64: class SamplerThread : public v8::base::Thread { Nit: Mark as V8_FINAL. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:77: static const int kSamplerThreadStackSize = 64 * 1024; There should be no need to manually override specify the stack size for the sampler thread. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:79: SamplerThread::SamplerThread(v8::Isolate* isolate) Nit: Inline method definition into declaration above. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:87: void SamplerThread::Stop() { Nit: Remove this method, there's no advantage over using Join() directly. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:92: void SamplerThread::Run() { Nit: Inline method definition into declaration above. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:98: v8::Sample* sample_ptr = &sample; Replace with something like: v8::Sample sample; CHECK_NOT_NULL(v8::Sampler::GetSample(isolate_, &sample)); CHECK_EQ(8, sample.frames_count); https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:136: SamplerThread* sampler = new SamplerThread(isolate); You leak the SamplerThread in this method. Just allocate it on the stack, i.e. SamplerThread sampler(isolate); sampler.Start(); script->Run(); sampler.Join(); https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:142: CHECK_EQ(8, sampler->sampleStackSize); Remove this CHECK_EQ here.
https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:41: unsigned frames_count : kMaxFramesCountLog2; // Number of captured frames. Don't use a bit field here, a plain unsigned int would be better. Nuke kMaxFramesCountLog2. https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:56: static Sample* GetSample(Isolate* isolate, On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Looking at the use in the test, I think we should have GetSample return a bool > instead. I would even go a step further and return void: Setting frames_count to zero is consistent and easier, I think. Or do we explicitly want to distinguish between some (undocumented!) error conditions and empty samples? I don't think so... Furthermore, I don't really like static functions, they are a strong indicator that the function should really live somewhere else. In our case: Just make it a member function of Isolate, nuke the Sampler class. Then we can move Sample to v8.h, too and remove the whole v8-sampler.h.
> I would even go a step further and return void: Setting frames_count to zero is > consistent and easier, I think. Or do we explicitly want to distinguish between > some (undocumented!) error conditions and empty samples? I don't think so... Oh yeah! That's definitely a better way of doing it. Will make the necessary changes... > Furthermore, I don't really like static functions, they are a strong indicator > that the function should really live somewhere else. In our case: Just make it a > member function of Isolate, nuke the Sampler class. Then we can move Sample to > v8.h, too and remove the whole v8-sampler.h. I see your point. It makes complete sense to move getsample into the isolate. I was wondering whether we should move the code-event listeners that would be needed soon, also to v8.h or whether we should keep v8-sampler.h for the code-event listener? (If you could have a quick look at https://codereview.chromium.org/499483002/ to get a rough idea of the listeners I'm talking about) Basically, we would need to expose code-creation and related events in the API so that the PC values in the sample make sense... What is your take on it?
- Addressed the comments. - Got rid of v8-sampler.h by moving code into v8.h - Added a test for the case where stack is deeper than kMaxFramesCount - Fixed the nits. https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h File include/v8-sampler.h (right): https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:41: unsigned frames_count : kMaxFramesCountLog2; // Number of captured frames. On 2014/08/27 06:13:47, Sven Panne wrote: > Don't use a bit field here, a plain unsigned int would be better. Nuke > kMaxFramesCountLog2. Done. https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:56: static Sample* GetSample(Isolate* isolate, On 2014/08/27 06:13:47, Sven Panne wrote: > On 2014/08/27 04:17:23, Benedikt Meurer wrote: > > Looking at the use in the test, I think we should have GetSample return a bool > > instead. > > I would even go a step further and return void: Setting frames_count to zero is > consistent and easier, I think. Or do we explicitly want to distinguish between > some (undocumented!) error conditions and empty samples? I don't think so... > > Furthermore, I don't really like static functions, they are a strong indicator > that the function should really live somewhere else. In our case: Just make it a > member function of Isolate, nuke the Sampler class. Then we can move Sample to > v8.h, too and remove the whole v8-sampler.h. Done. https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:56: static Sample* GetSample(Isolate* isolate, On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Looking at the use in the test, I think we should have GetSample return a bool > instead. Yes, that would surely be a better way to do it compared to the redundant and ambiguous use of sample pointer. As per Sven's suggestion, setting the frame count to zero should also do. I'm not able to see a case rightaway where that would be a problem, but you are much more familiar with the code and you might, please let me know in that case... https://codereview.chromium.org/422593003/diff/280001/include/v8-sampler.h#ne... include/v8-sampler.h:59: } // namespace v8 On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Nit: Add newline after class declaration. Acknowledged. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:1: // Copyright 2014 the V8 project authors. All rights reserved. On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Nit: Use new short copyright header. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:44: namespace SamplerTester { On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Use an anonymous namespace instead. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:64: class SamplerThread : public v8::base::Thread { On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Nit: Mark as V8_FINAL. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:77: static const int kSamplerThreadStackSize = 64 * 1024; On 2014/08/27 04:17:24, Benedikt Meurer wrote: > There should be no need to manually override specify the stack size for the > sampler thread. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:79: SamplerThread::SamplerThread(v8::Isolate* isolate) On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Nit: Inline method definition into declaration above. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:87: void SamplerThread::Stop() { On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Nit: Remove this method, there's no advantage over using Join() directly. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:92: void SamplerThread::Run() { On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Nit: Inline method definition into declaration above. Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:98: v8::Sample* sample_ptr = &sample; On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Replace with something like: > > v8::Sample sample; > CHECK_NOT_NULL(v8::Sampler::GetSample(isolate_, &sample)); > CHECK_EQ(8, sample.frames_count); Changed the API so that sample.frames_count is zero in case of failure in sample collection. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:136: SamplerThread* sampler = new SamplerThread(isolate); On 2014/08/27 04:17:23, Benedikt Meurer wrote: > You leak the SamplerThread in this method. Just allocate it on the stack, i.e. > > SamplerThread sampler(isolate); > sampler.Start(); > script->Run(); > sampler.Join(); Oops. Damn :( Done. https://codereview.chromium.org/422593003/diff/280001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:142: CHECK_EQ(8, sampler->sampleStackSize); On 2014/08/27 04:17:23, Benedikt Meurer wrote: > Remove this CHECK_EQ here. Yes, removed all CHECK_* logic from the Run method and put it here instead.
On 2014/08/27 16:21:41, gholap wrote: > I see your point. It makes complete sense to move getsample into the isolate. > I was wondering whether we should move the code-event listeners that would be > needed soon, also to v8.h or whether we should keep v8-sampler.h for the > code-event > listener? (If you could have a quick look at > https://codereview.chromium.org/499483002/ > to get a rough idea of the listeners I'm talking about) > > Basically, we would need to expose code-creation and related events in the API > so that > the PC values in the sample make sense... > > What is your take on it? I've just commented on that CL, and I think that we don't need a new kind of listener at all.
https://codereview.chromium.org/422593003/diff/300001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/300001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:21: v8::base::Semaphore* SamplerSemaphore = new v8::base::Semaphore(0); Nit: Variable naming, SamplerSemaphore -> sampler_semaphore. https://codereview.chromium.org/422593003/diff/300001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:24: v8::base::Semaphore* V8Semaphore = new v8::base::Semaphore(0); Nit: Variable naming.
Taken into account all nits and changes pointed out by Benedikt and Sven. Please take a look :)
https://codereview.chromium.org/422593003/diff/320001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/320001/include/v8.h#newcode1303 include/v8.h:1303: typedef unsigned char* Address; Just use void* in sample and do the casting internally, otherwise some people out there start using this and we will be tied to this eternally. https://codereview.chromium.org/422593003/diff/320001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/320001/src/sampler.cc#newcode743 src/sampler.cc:743: void Sampler::CopyTickSampleToSample(TickSample* tick_sample, You don't need this, normal assignment will do the slicing.
ptal? :) https://codereview.chromium.org/422593003/diff/320001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/320001/include/v8.h#newcode1303 include/v8.h:1303: typedef unsigned char* Address; On 2014/09/01 08:35:58, Sven Panne wrote: > Just use void* in sample and do the casting internally, otherwise some people > out there start using this and we will be tied to this eternally. Done. https://codereview.chromium.org/422593003/diff/320001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/320001/src/sampler.cc#newcode743 src/sampler.cc:743: void Sampler::CopyTickSampleToSample(TickSample* tick_sample, On 2014/09/01 08:35:58, Sven Panne wrote: > You don't need this, normal assignment will do the slicing. Done.
Almost there... https://codereview.chromium.org/422593003/diff/340001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/log.cc#newcode1723 src/log.cc:1723: msg.AppendAddress(reinterpret_cast<Address>(sample->stack[i])); Always use the least powerful cast possible: A static_cast is enough here. https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.c... src/profile-generator.cc:631: for (const Address* stack_pos = reinterpret_cast<const Address*>(sample.stack), static_cast again, the const seems to be unnecessary here. https://codereview.chromium.org/422593003/diff/340001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/sampler.cc#newcode818 src/sampler.cc:818: *sample = reinterpret_cast<v8::Sample>(tick_sample); Remove the cast, it's not necessary. https://codereview.chromium.org/422593003/diff/340001/src/sampler.h File src/sampler.h (right): https://codereview.chromium.org/422593003/diff/340001/src/sampler.h#newcode51 src/sampler.h:51: const RegisterState& state); Is this caused by "git cl format"? I thought it keeps things on a single line if possible. https://codereview.chromium.org/422593003/diff/340001/src/sampler.h#newcode132 src/sampler.h:132: static void CopyTickSampleToSample(TickSample* tick_sample, Remove this. https://codereview.chromium.org/422593003/diff/340001/test/cctest/test-log-st... File test/cctest/test-log-stack-tracer.cc (right): https://codereview.chromium.org/422593003/diff/340001/test/cctest/test-log-st... test/cctest/test-log-stack-tracer.cc:59: static bool IsAddressWithinFuncCode(JSFunction* function, Address addr) { Use a void* for the 2nd arg and do the static_cast within this function. This way you can adapt the callers and avoid the casting there.
Addressed the comments. Sven, thanks a lot for the quick reviews!! :) https://codereview.chromium.org/422593003/diff/340001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/log.cc#newcode1723 src/log.cc:1723: msg.AppendAddress(reinterpret_cast<Address>(sample->stack[i])); On 2014/09/02 08:15:01, Sven Panne wrote: > Always use the least powerful cast possible: A static_cast is enough here. Done. https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.c... src/profile-generator.cc:631: for (const Address* stack_pos = reinterpret_cast<const Address*>(sample.stack), On 2014/09/02 08:15:02, Sven Panne wrote: > static_cast again, the const seems to be unnecessary here. Hmm... that should work for individual pointers, right? and not arrays of pointers? (won't compile unless reinterpret_cast and const both are present...) https://codereview.chromium.org/422593003/diff/340001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/sampler.cc#newcode818 src/sampler.cc:818: *sample = reinterpret_cast<v8::Sample>(tick_sample); On 2014/09/02 08:15:02, Sven Panne wrote: > Remove the cast, it's not necessary. Oops. Done. https://codereview.chromium.org/422593003/diff/340001/src/sampler.h File src/sampler.h (right): https://codereview.chromium.org/422593003/diff/340001/src/sampler.h#newcode51 src/sampler.h:51: const RegisterState& state); On 2014/09/02 08:15:02, Sven Panne wrote: > Is this caused by "git cl format"? I thought it keeps things on a single line if > possible. Hmm... I must've goofed it up... https://codereview.chromium.org/422593003/diff/340001/src/sampler.h#newcode132 src/sampler.h:132: static void CopyTickSampleToSample(TickSample* tick_sample, On 2014/09/02 08:15:02, Sven Panne wrote: > Remove this. Done. https://codereview.chromium.org/422593003/diff/340001/test/cctest/test-log-st... File test/cctest/test-log-stack-tracer.cc (right): https://codereview.chromium.org/422593003/diff/340001/test/cctest/test-log-st... test/cctest/test-log-stack-tracer.cc:59: static bool IsAddressWithinFuncCode(JSFunction* function, Address addr) { On 2014/09/02 08:15:02, Sven Panne wrote: > Use a void* for the 2nd arg and do the static_cast within this function. This > way you can adapt the callers and avoid the casting there. Done.
https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.c... src/profile-generator.cc:631: for (const Address* stack_pos = reinterpret_cast<const Address*>(sample.stack), On 2014/09/02 08:56:08, gholap wrote: > On 2014/09/02 08:15:02, Sven Panne wrote: > > static_cast again, the const seems to be unnecessary here. > > Hmm... that should work for individual pointers, right? and not arrays of > pointers? > (won't compile unless reinterpret_cast and const both are present...) Done correctly, it *would* work, but actually there is a far easier way than this unreadable pointer fiddling: for (unsigned i = 0; i < sample.frames_count; ++i) { *entry++ = code_map_.FindEntry(static_cast<Address>(sample.stack[i])); } Looking at the use sites, you might even consider replacing the old-skool array+size interface of Sample by an iterator interface with begin() and end(), so one could write (in C++11, available soon): for (auto slot: sample) { *entry++ = code_map_.FindEntry(static_cast<Address>(slot)); }
As Sven pointed out, simplified the for loop. what say now? https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.cc File src/profile-generator.cc (right): https://codereview.chromium.org/422593003/diff/340001/src/profile-generator.c... src/profile-generator.cc:631: for (const Address* stack_pos = reinterpret_cast<const Address*>(sample.stack), On 2014/09/02 11:15:36, Sven Panne wrote: > On 2014/09/02 08:56:08, gholap wrote: > > On 2014/09/02 08:15:02, Sven Panne wrote: > > > static_cast again, the const seems to be unnecessary here. > > > > Hmm... that should work for individual pointers, right? and not arrays of > > pointers? > > (won't compile unless reinterpret_cast and const both are present...) > > Done correctly, it *would* work, but actually there is a far easier way than > this unreadable pointer fiddling: > > for (unsigned i = 0; i < sample.frames_count; ++i) { > *entry++ = code_map_.FindEntry(static_cast<Address>(sample.stack[i])); > } > > Looking at the use sites, you might even consider replacing the old-skool > array+size interface of Sample by an iterator interface with begin() and end(), > so one could write (in C++11, available soon): > > for (auto slot: sample) { > *entry++ = code_map_.FindEntry(static_cast<Address>(slot)); > } Done. I know where I went wrong! I took my working thumb-rule to be edit/touch as few lines of code as possible :P That way, I didn't even consider what is going on inside the for loop! I'll make efforts to avoid such thing henceforth...
As Sven pointed out, simplified the for loop. what say now?
As Sven pointed out, simplified the for loop. what say now?
As Sven pointed out, simplified the for loop. what say now?
Patchset #21 (id:400001) has been deleted
Now the tests also cover the verification of pc values against the compiled code to make sure that the stack returned by GetSample is complete and consistent. Please have a look... I know this is really getting long-drawn, I appreciate the patience and feedback :)
I thought about the public API again. https://codereview.chromium.org/422593003/diff/420001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/420001/include/v8.h#newcode1313 include/v8.h:1313: struct V8_EXPORT Sample { I would really love to get rid of the public fields below, exposing the internals is never a good idea, although I admit that it's unlikely that we will ever change this. But how about this instead? class V8_EXPORT Sample { public: enum { kMaxSize = 255u }; Sample() : size_(0) {} template <class InputIterator> Sample(InputIterator first, InputIterator last) : size_(0) { for (; first != last; ++first) data_[size_++] = *first; } typedef void const** const_iterator; const_iterator begin() const { return &data_[0]; } const_iterator end() const { return &data_[size_]; } size_t size() const { return size_; } private: void const* data_[kMaxSize]; size_t size_; }; Not super critical for the first version, but on the other hand this is public API and we should not refactor it too often once landed, so maybe better have it in from the beginning. WDYT? https://codereview.chromium.org/422593003/diff/420001/include/v8.h#newcode1316 include/v8.h:1316: static const unsigned kMaxFramesCount = 255; Should probably be enum { kMaxFramesCount = 255u }; or otherwise requires a corresponding definition in an implementation file (we had strange linker bugs with some compilers and code like this recently). https://codereview.chromium.org/422593003/diff/420001/include/v8.h#newcode1319 include/v8.h:1319: unsigned frames_count; // Number of captured frames. I think this should be size_t instead of unsigned.
made the changes: 1) unsigned -> size_t 2) kMaxFramesCount is in an enum now as opposed to a compile time constant. Denedikt, let's discuss the iterator in person... Also, how does the test look? Some input on that would be good too :) https://codereview.chromium.org/422593003/diff/420001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/420001/include/v8.h#newcode1313 include/v8.h:1313: struct V8_EXPORT Sample { On 2014/09/03 04:24:30, Benedikt Meurer wrote: > I would really love to get rid of the public fields below, exposing the > internals is never a good idea, although I admit that it's unlikely that we will > ever change this. But how about this instead? > > class V8_EXPORT Sample { > public: > enum { kMaxSize = 255u }; > > Sample() : size_(0) {} > template <class InputIterator> > Sample(InputIterator first, InputIterator last) : size_(0) { > for (; first != last; ++first) data_[size_++] = *first; > } > > typedef void const** const_iterator; > const_iterator begin() const { return &data_[0]; } > const_iterator end() const { return &data_[size_]; } > size_t size() const { return size_; } > > private: > void const* data_[kMaxSize]; > size_t size_; > }; > > Not super critical for the first version, but on the other hand this is public > API and we should not refactor it too often once landed, so maybe better have it > in from the beginning. WDYT? Let us discuss this in person? https://codereview.chromium.org/422593003/diff/420001/include/v8.h#newcode1316 include/v8.h:1316: static const unsigned kMaxFramesCount = 255; On 2014/09/03 04:24:30, Benedikt Meurer wrote: > Should probably be > > enum { kMaxFramesCount = 255u }; > > or otherwise requires a corresponding definition in an implementation file (we > had strange linker bugs with some compilers and code like this recently). Done. https://codereview.chromium.org/422593003/diff/420001/include/v8.h#newcode1319 include/v8.h:1319: unsigned frames_count; // Number of captured frames. On 2014/09/03 04:24:30, Benedikt Meurer wrote: > I think this should be size_t instead of unsigned. Done.
Made the Sample class into an iterable thing. Have a look :)
One last nit from me. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode757 src/sampler.cc:757: *sample = v8::Sample(SignalHandler::sample_.stack, Nit: Use v8::Sample(&SignalHandler::sample_.stack[0], &SignalHandler::sample_.stack[SignalHandler::sample_.frames_count]) syntax instead to make it easier to spot the array arithmetic here. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode820 src/sampler.cc:820: *sample = v8::Sample(tick_sample.stack, Nit: Use v8::Sample(&tick_sample.stack[0], &tick_sample.stack[tick_sample.frames_count]) syntax instead to make it easier to spot the array arithmetic here.
LGTM if Benedikt's comments are addressed
And of course, rebase your patch on bleeding_edge.
alph@chromium.org changed reviewers: + alph@chromium.org, yurys@chromium.org
https://codereview.chromium.org/422593003/diff/460001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/460001/include/v8.h#newcode1301 include/v8.h:1301: * Isolate::Getsample collects the current JS execution state as a sample. nit: GetSample https://codereview.chromium.org/422593003/diff/460001/include/v8.h#newcode1331 include/v8.h:1331: void const* data_[kMaxSize]; nit: const void https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode757 src/sampler.cc:757: *sample = v8::Sample(SignalHandler::sample_.stack, Note it will always copy all the 255 frames. The profiler code is quite performance critical. Yury was squeezing it to make it possible to do 10kHz sampling. I wonder if it's possible to avoid such a huge memory copying, e.g. by using a placement new? https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode766 src/sampler.cc:766: void Sampler::GetSampleHelper(TickSample * sample, nit: it doesn't match the declaration in .h https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode799 src/sampler.cc:799: sample->Init(sampler->isolate(), state); hmm... what is sampler? Does it compile on Win? https://codereview.chromium.org/422593003/diff/460001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/460001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:11: #include "src/v8.h" style: no need for extra lines between includes.
Addressed the issues - nits - placement new - windows build and testing TODO: rebase on bleeding_edge Please take a look :) https://codereview.chromium.org/422593003/diff/460001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/460001/include/v8.h#newcode1301 include/v8.h:1301: * Isolate::Getsample collects the current JS execution state as a sample. On 2014/09/08 12:47:40, alph wrote: > nit: GetSample Done. https://codereview.chromium.org/422593003/diff/460001/include/v8.h#newcode1331 include/v8.h:1331: void const* data_[kMaxSize]; On 2014/09/08 12:47:40, alph wrote: > nit: const void Done. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode757 src/sampler.cc:757: *sample = v8::Sample(SignalHandler::sample_.stack, On 2014/09/05 03:50:00, Benedikt Meurer wrote: > Nit: Use > > v8::Sample(&SignalHandler::sample_.stack[0], > &SignalHandler::sample_.stack[SignalHandler::sample_.frames_count]) > > syntax instead to make it easier to spot the array arithmetic here. Done. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode757 src/sampler.cc:757: *sample = v8::Sample(SignalHandler::sample_.stack, On 2014/09/08 12:47:40, alph wrote: > Note it will always copy all the 255 frames. > The profiler code is quite performance critical. Yury was squeezing it to make > it possible to do 10kHz sampling. > > I wonder if it's possible to avoid such a huge memory copying, e.g. by using a > placement new? Done. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode766 src/sampler.cc:766: void Sampler::GetSampleHelper(TickSample * sample, On 2014/09/08 12:47:40, alph wrote: > nit: it doesn't match the declaration in .h Done. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode799 src/sampler.cc:799: sample->Init(sampler->isolate(), state); On 2014/09/08 12:47:40, alph wrote: > hmm... what is sampler? > Does it compile on Win? Corrected. https://codereview.chromium.org/422593003/diff/460001/src/sampler.cc#newcode820 src/sampler.cc:820: *sample = v8::Sample(tick_sample.stack, On 2014/09/05 03:50:00, Benedikt Meurer wrote: > Nit: Use > > v8::Sample(&tick_sample.stack[0], &tick_sample.stack[tick_sample.frames_count]) > > syntax instead to make it easier to spot the array arithmetic here. Done. https://codereview.chromium.org/422593003/diff/460001/test/cctest/test-sample... File test/cctest/test-sampler-api.cc (right): https://codereview.chromium.org/422593003/diff/460001/test/cctest/test-sample... test/cctest/test-sampler-api.cc:11: #include "src/v8.h" On 2014/09/08 12:47:40, alph wrote: > style: no need for extra lines between includes. Done.
lgtm. I'd suggest you to have yurys@ to take a look at it as he's responsible for major part of sampler. https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc File src/sampler.cc (left): https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc#oldcode4 src/sampler.cc:4: Please revert https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc#newcode779 src/sampler.cc:779: void Sampler::GetSampleHelper(TickSample* sample, bool called_from_get_sample) { nit: you can get rid of the called_from_get_sample argument. Instead fill the sample with Init if it's not NULL, and do the SampleStack otherwise. https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc#newcode826 src/sampler.cc:826: // a TickSample and never on Sample. nit: I don't think this comment adds something here. https://codereview.chromium.org/422593003/diff/500001/src/sampler.h File src/sampler.h (right): https://codereview.chromium.org/422593003/diff/500001/src/sampler.h#newcode19 src/sampler.h:19: class ScopedSemaphore { Move it into the cc file?
https://codereview.chromium.org/422593003/diff/500001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/500001/include/v8.h#newcode4359 include/v8.h:4359: void GetSample(Sample* sample); This is not what the document proposed. The idea was to expose an API for collecting current v8 stack trace on a paused thread. It is the embedder who should be responsible for pausing the thread (using SIGPROF or SuspendThread) and calling GetSample. Introducing such API would allow us to move profiler implementation out of V8 into e.g. Blink. With GetSample introduced in this CL we will have to keep all the platform-specific profiler code in v8. https://codereview.chromium.org/422593003/diff/500001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/422593003/diff/500001/src/log.cc#newcode1872 src/log.cc:1872: // TODO(gholap): This one's for the new sampler API. (include/v8-sampler.h) There is no include/v8-sampler.h https://codereview.chromium.org/422593003/diff/500001/src/log.cc#newcode1877 src/log.cc:1877: ticker_->IncreaseProfilingDepth(); This breaks the invariant as now Sampler::IsProfiling will always return true. Doing this unconditionally is weird. We shouldn't need SIGPROF handler unless profiler is on. Having thread state passed into v8::Isolate::GetSample as described in the document would help to avoid such issues. https://codereview.chromium.org/422593003/diff/500001/src/sampler.h File src/sampler.h (right): https://codereview.chromium.org/422593003/diff/500001/src/sampler.h#newcode25 src/sampler.h:25: base::Semaphore* semaphore_; semaphore.h would be a more appropriate place for this. https://codereview.chromium.org/422593003/diff/500001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/422593003/diff/500001/test/cctest/cctest.gyp#... test/cctest/cctest.gyp:145: 'test-sampler-api.cc', This file is not in the CL anymore. Is that intentional?
gholap@chromium.org changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/422593003/diff/500001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/500001/include/v8.h#newcode4359 include/v8.h:4359: void GetSample(Sample* sample); On 2014/09/09 14:35:20, yurys wrote: > This is not what the document proposed. The idea was to expose an API for > collecting current v8 stack trace on a paused thread. It is the embedder who > should be responsible for pausing the thread (using SIGPROF or SuspendThread) > and calling GetSample. Introducing such API would allow us to move profiler > implementation out of V8 into e.g. Blink. With GetSample introduced in this CL > we will have to keep all the platform-specific profiler code in v8. Agreed. We'll soon delete threads from v8 and instead solely rely on embedder threads, then this won't work anymore anyways
I've addressed the main concern regarding moving the threading logic out of GetSample and I've updated the test accordingly to demonstrate the use case. This is the document which goes into details about why I could not come up with an uniform API for both windows and unix: https://docs.google.com/a/chromium.org/document/d/1TiY-DFwkRTT0EiOijg2aTPrt2A... My internship's coming to an end this Friday, and I really appreciate feedback on this CL and the document both. Also, currently the test fails on windows because SuspendThread fails and I'm trying to figure that out, but meanwhile, it'll be awesome to get inputs on the current design and implementation of the API. https://codereview.chromium.org/422593003/diff/500001/include/v8.h File include/v8.h (right): https://codereview.chromium.org/422593003/diff/500001/include/v8.h#newcode4359 include/v8.h:4359: void GetSample(Sample* sample); On 2014/09/10 07:39:20, jochen wrote: > On 2014/09/09 14:35:20, yurys wrote: > > This is not what the document proposed. The idea was to expose an API for > > collecting current v8 stack trace on a paused thread. It is the embedder who > > should be responsible for pausing the thread (using SIGPROF or SuspendThread) > > and calling GetSample. Introducing such API would allow us to move profiler > > implementation out of V8 into e.g. Blink. With GetSample introduced in this CL > > we will have to keep all the platform-specific profiler code in v8. > > Agreed. > > We'll soon delete threads from v8 and instead solely rely on embedder threads, > then this won't work anymore anyways Acknowledged. https://codereview.chromium.org/422593003/diff/500001/src/log.cc File src/log.cc (right): https://codereview.chromium.org/422593003/diff/500001/src/log.cc#newcode1872 src/log.cc:1872: // TODO(gholap): This one's for the new sampler API. (include/v8-sampler.h) On 2014/09/09 14:35:20, yurys wrote: > There is no include/v8-sampler.h Done. https://codereview.chromium.org/422593003/diff/500001/src/log.cc#newcode1877 src/log.cc:1877: ticker_->IncreaseProfilingDepth(); On 2014/09/09 14:35:20, yurys wrote: > This breaks the invariant as now Sampler::IsProfiling will always return true. > Doing this unconditionally is weird. We shouldn't need SIGPROF handler unless > profiler is on. Having thread state passed into v8::Isolate::GetSample as > described in the document would help to avoid such issues. Done. https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc#newcode779 src/sampler.cc:779: void Sampler::GetSampleHelper(TickSample* sample, bool called_from_get_sample) { On 2014/09/09 13:54:37, alph wrote: > nit: you can get rid of the called_from_get_sample argument. > Instead fill the sample with Init if it's not NULL, and do the SampleStack > otherwise. Done. https://codereview.chromium.org/422593003/diff/500001/src/sampler.cc#newcode826 src/sampler.cc:826: // a TickSample and never on Sample. On 2014/09/09 13:54:37, alph wrote: > nit: I don't think this comment adds something here. Acknowledged. https://codereview.chromium.org/422593003/diff/500001/src/sampler.h File src/sampler.h (right): https://codereview.chromium.org/422593003/diff/500001/src/sampler.h#newcode25 src/sampler.h:25: base::Semaphore* semaphore_; On 2014/09/09 14:35:20, yurys wrote: > semaphore.h would be a more appropriate place for this. Yes agreed. Another alternative could be, as alph suggested, to move it to the cc file. I'm more inclined toward moving it to .cc file because this doesn't look like a terribly useful thing to make it general. But yes, I might be completely wrong about that, I would love your input on this. https://codereview.chromium.org/422593003/diff/500001/test/cctest/cctest.gyp File test/cctest/cctest.gyp (right): https://codereview.chromium.org/422593003/diff/500001/test/cctest/cctest.gyp#... test/cctest/cctest.gyp:145: 'test-sampler-api.cc', On 2014/09/09 14:35:20, yurys wrote: > This file is not in the CL anymore. Is that intentional? Done.
As far as I understand the idea was to remove all the signal handling / thread stopping logic out of v8. But this patch seems to bring more of these instead. Shouldn't it be just a plain GetSample function that collects a stack trace for the specified context assuming that the thread either: - has already been stopped (Win) - is the current thread (Posix). Right? https://codereview.chromium.org/422593003/diff/520001/src/sampler.cc File src/sampler.cc (left): https://codereview.chromium.org/422593003/diff/520001/src/sampler.cc#oldcode4 src/sampler.cc:4: plz revert. https://codereview.chromium.org/422593003/diff/520001/src/sampler.cc File src/sampler.cc (right): https://codereview.chromium.org/422593003/diff/520001/src/sampler.cc#newcode274 src/sampler.cc:274: return; // Already setup no need for the comment. https://codereview.chromium.org/422593003/diff/520001/src/sampler.h File src/sampler.h (right): https://codereview.chromium.org/422593003/diff/520001/src/sampler.h#newcode22 src/sampler.h:22: class SignalHandler; seems to be unused in the header. https://codereview.chromium.org/422593003/diff/520001/src/sampler.h#newcode26 src/sampler.h:26: explicit ScopedSemaphore(base::Semaphore* sem) {semaphore_ = sem;} nit: should be an initializer. https://codereview.chromium.org/422593003/diff/520001/src/sampler.h#newcode78 src/sampler.h:78: static void SetUpForNewSamplingAPI(); Avoid using words like 'old' and 'new' in the function names. Something that was new becomes old at some point.
> [...] Shouldn't it be just a plain GetSample function that collects a stack trace for > the specified context assuming that the thread either: > - has already been stopped (Win) > - is the current thread (Posix). Just a small remark regarding the Win/Linux distinction (Mac can actually stop threads from the outside AFAIK): Wouldn't it be easier and more consistent to stop the thread to be sampled on *all* platforms? <HandWavingMode>I think you can emulate the Win/Mac behavior with a signal/signal handler/semaphores on Linux.</HandWavingMode> ;-)
How about we move the design discussion here? https://docs.google.com/a/chromium.org/document/d/1TiY-DFwkRTT0EiOijg2aTPrt2A... The document has extensive listing of various alternatives and pros cons of each one.
On 2014/09/17 16:33:39, gholap wrote: > How about we move the design discussion here? > https://docs.google.com/a/chromium.org/document/d/1TiY-DFwkRTT0EiOijg2aTPrt2A... > > The document has extensive listing of various alternatives and pros cons of each > one. Could you please enable editing and/or commenting. thanks
svenpanne@chromium.org changed reviewers: - svenpanne@chromium.org
Closing this issue as this functionality was implemented in https://codereview.chromium.org/596533002 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
