Replacing circular queue by single buffer in CPU Profiler.

Replacing circular queue by single buffer in CPU Profiler.

BUG=None

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

[Jakob Kummerow, 2012-09-04 11:25:18]

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler-inl.h
File src/cpu-profiler-inl.h (right):

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler-inl.h#newc...
src/cpu-profiler-inl.h:60: return NULL;
Please either move this onto the same line as the "if", or put {} around it. (V8
differs from Chromium style here, we don't omit the {} unless everything is on
the same line.)

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc#newcode51
src/cpu-profiler.cc:51: ticks_buffer_is_empty_(true),
You should still initialize ticks_buffer_(NULL) too (C++ doesn't do that
automatically).

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc#newcode216
src/cpu-profiler.cc:216: if (ticks_buffer_.order == dequeue_order) {
Does it still make sense to have a dequeue_order specified and compared against
when there's no queue any more?

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc#newcode221
src/cpu-profiler.cc:221: ticks_buffer_.sample.frames_count = 0;
{} around the conditional block please

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.h
File src/cpu-profiler.h (right):

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.h#newcode160
src/cpu-profiler.h:160: // Finish should be called only after successful Start
(returning non-NULL pointer).
long line.
Also, can you put in an ASSERT that makes sure this rule is not violated?

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.h#newcode225
src/cpu-profiler.h:225: // Finish should be called only after successful Start
(returning non-NULL pointer).
long line

[rogulenko, 2012-09-04 13:14:55]

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc#newcode51
src/cpu-profiler.cc:51: ticks_buffer_is_empty_(true),
On 2012/09/04 11:25:18, Jakob wrote:
> You should still initialize ticks_buffer_(NULL) too (C++ doesn't do that
> automatically).
Did you mean that you would like ticks_buffer_ to be a pointer?

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc#newcode216
src/cpu-profiler.cc:216: if (ticks_buffer_.order == dequeue_order) {
On 2012/09/04 11:25:18, Jakob wrote:
> Does it still make sense to have a dequeue_order specified and compared
against
> when there's no queue any more?
ProfilerEventsProcessor has two queues. One to keep stack samples (circular
queue, that is removed by this patch), another to keep code events. Code events
are used to keep up-to-date a map from pointers in JavaScript stack to code
lines. Actually dequeue_order for a stack sample is the number of code events
recorded before taking this stack sample. Global dequeue_order is the number of
processed code events. So inequality here means that we processed less code
events than we recorded before taking a stack sample => we should process
another code event.

[Jakob Kummerow, 2012-09-04 13:36:22]

LGTM

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10871039/diff/8004/src/cpu-profiler.cc#newcode51
src/cpu-profiler.cc:51: ticks_buffer_is_empty_(true),
On 2012/09/04 13:14:55, rogulenko wrote:
> On 2012/09/04 11:25:18, Jakob wrote:
> > You should still initialize ticks_buffer_(NULL) too (C++ doesn't do that
> > automatically).
> Did you mean that you would like ticks_buffer_ to be a pointer?

Ah, sorry, somehow I thought it was one already. It's fine then.

[Jakob Kummerow, 2012-09-04 17:44:45]

http://codereview.chromium.org/10871039/diff/15005/src/cpu-profiler.cc
File src/cpu-profiler.cc (right):

http://codereview.chromium.org/10871039/diff/15005/src/cpu-profiler.cc#newcod...
src/cpu-profiler.cc:254: sampler_->DoSample();
{} please

http://codereview.chromium.org/10871039/diff/15005/test/cctest/test-cpu-profi...
File test/cctest/test-cpu-profiler.cc (right):

http://codereview.chromium.org/10871039/diff/15005/test/cctest/test-cpu-profi...
test/cctest/test-cpu-profiler.cc:43: i::Address frame1,
indentation

http://codereview.chromium.org/10871039/diff/15005/test/cctest/test-cpu-profi...
test/cctest/test-cpu-profiler.cc:163: ToAddress(0x1404),
indentation

[Jakob Kummerow, 2012-10-02 10:56:51]

Landed. Had to fix to issues to make it compile:

http://codereview.chromium.org/10871039/diff/24004/src/cpu-profiler-inl.h
File src/cpu-profiler-inl.h (right):

http://codereview.chromium.org/10871039/diff/24004/src/cpu-profiler-inl.h#new...
src/cpu-profiler-inl.h:68: ASSERT(ticks_buffer_is_initialized &&
ticks_buffer_is_empty_);
typo: ticks_buffer_is_initialized_ is missing its trailing underscore.

http://codereview.chromium.org/10871039/diff/24004/src/cpu-profiler.h
File src/cpu-profiler.h (left):

http://codereview.chromium.org/10871039/diff/24004/src/cpu-profiler.h#oldcode33
src/cpu-profiler.h:33: #include "circular-queue.h"
We still need this for the ASSERT in line 104.