Track history of events in GCTracer.

Track history of events in GCTracer.

- track incremental marking stats directly on GCTracer.
- add simple ring buffer class.
- track last 10 scavenges and mark-compacts in ring buffers on GCTracer.
- various clean-ups.

R=hpayer@chromium.org
BUG=

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

[ernstm, 2014-07-21 16:01:44]

PTAL

[Hannes Payer (out of office), 2014-07-22 09:10:17]

I like it! First round of comments.

https://codereview.chromium.org/391413006/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/391413006/diff/20001/src/globals.h#newcode361
src/globals.h:361: enum GarbageCollector { SCAVENGER = 0, MARK_COMPACTOR = 1};
Why do you have to initialize it with =0, and =1?

https://codereview.chromium.org/391413006/diff/20001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/20001/src/heap.cc#newcode6022
src/heap.cc:6022: Event mark_compactor_dummy_event(MARK_COMPACTOR, NULL, NULL);
Hmm, I am not sure if the dummy elements may be problematic. What about
calculating the average gc time of the events in a buffer. The dummy element
would have an impact on that.

https://codereview.chromium.org/391413006/diff/20001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/20001/src/heap.h#newcode653
src/heap.h:653: struct Event {
C++ style guide: Use a struct only for passive objects that carry data;
everything else is a class.

https://codereview.chromium.org/391413006/diff/20001/src/heap.h#newcode749
src/heap.h:749: EventBuffer event_buffers_[2];
Ah, now I see why you have to initialize the enum. Why don't we use two
EventBuffer intances?
EventBuffer scavenber_buffer_;
EventBuffer mark_compact_buffer_;
Should actually result in more compact code. WDYT? However, I can live with both
options.

[ernstm, 2014-07-22 14:09:29]

- fixed RingBuffer.
- added unit test for RingBuffer
- separate RingBuffer members for scavenge and mark_compactor
- Event::Type to distinguish ANCHOR events from real GC events.
- Clean-ups.

https://codereview.chromium.org/391413006/diff/20001/src/globals.h
File src/globals.h (right):

https://codereview.chromium.org/391413006/diff/20001/src/globals.h#newcode361
src/globals.h:361: enum GarbageCollector { SCAVENGER = 0, MARK_COMPACTOR = 1};
On 2014/07/22 09:10:17, Hannes Payer wrote:
> Why do you have to initialize it with =0, and =1?

Not needed any more, with the introduction of Event::Type.

https://codereview.chromium.org/391413006/diff/20001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/20001/src/heap.cc#newcode6022
src/heap.cc:6022: Event mark_compactor_dummy_event(MARK_COMPACTOR, NULL, NULL);
On 2014/07/22 09:10:17, Hannes Payer wrote:
> Hmm, I am not sure if the dummy elements may be problematic. What about
> calculating the average gc time of the events in a buffer. The dummy element
> would have an impact on that.

Good point. I've added a type to the Event that will allow us to filter out the
dummy events (now called ANCHOR). Not having them will complicate a lot of the
code, because we need stats at GCTracer construction time to compute deltas for
the first real GC event.

https://codereview.chromium.org/391413006/diff/20001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/20001/src/heap.h#newcode653
src/heap.h:653: struct Event {
On 2014/07/22 09:10:17, Hannes Payer wrote:
> C++ style guide: Use a struct only for passive objects that carry data;
> everything else is a class.

Done.

https://codereview.chromium.org/391413006/diff/20001/src/heap.h#newcode749
src/heap.h:749: EventBuffer event_buffers_[2];
On 2014/07/22 09:10:17, Hannes Payer wrote:
> Ah, now I see why you have to initialize the enum. Why don't we use two
> EventBuffer intances?
> EventBuffer scavenber_buffer_;
> EventBuffer mark_compact_buffer_;
> Should actually result in more compact code. WDYT? However, I can live with
both
> options.

I've chosen the array, because the Previous() method can be implemented more
compactly if the 'other' ring buffer can be selected by index.

[ernstm, 2014-07-22 14:10:26]

https://codereview.chromium.org/391413006/diff/20001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/20001/src/heap.h#newcode749
src/heap.h:749: EventBuffer event_buffers_[2];
On 2014/07/22 14:09:28, ernstm wrote:
> On 2014/07/22 09:10:17, Hannes Payer wrote:
> > Ah, now I see why you have to initialize the enum. Why don't we use two
> > EventBuffer intances?
> > EventBuffer scavenber_buffer_;
> > EventBuffer mark_compact_buffer_;
> > Should actually result in more compact code. WDYT? However, I can live with
> both
> > options.
> 
> I've chosen the array, because the Previous() method can be implemented more
> compactly if the 'other' ring buffer can be selected by index.

Update: The latest version uses two separate members.

[Hannes Payer (out of office), 2014-07-22 15:21:31]

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode5974
src/heap.cc:5974: GCTracer::Event::Event()
Why do we need the default constructor?

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6059
src/heap.cc:6059: if (collector == SCAVENGER)
coding style: if/else likes curly braces

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6088
src/heap.cc:6088: scavenger_events_.push_back(pending_);
coding style: if/else likes curly braces

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6130
src/heap.cc:6130: mark_compactor_events_.back()->end_time)
coding style: if/else likes curly braces

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6159
src/heap.cc:6159: return last_in_other_buffer;
coding style: if/else likes curly braces

https://codereview.chromium.org/391413006/diff/40001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode607
src/heap.h:607: end_ = (end_ + 1) % (MAX_SIZE + 1);
Hmm, MAX_SIZE+1 makes the code really complicated. And, the anchor elements is
just there until it gets overwritten (logically it is still there after that). I
am wondering if it wouldn't be cleaner if we would handle the case where there
is no previous element externally... WDYT?

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode615
src/heap.h:615: size_t end_;
DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode676
src/heap.h:676: INVALID = 3
what is the purpose of invalid?

https://codereview.chromium.org/391413006/diff/40001/test/cctest/cctest.gyp
File test/cctest/cctest.gyp (right):

https://codereview.chromium.org/391413006/diff/40001/test/cctest/cctest.gyp#n...
test/cctest/cctest.gyp:81: 'test-gc-tracer.cc',
awesome!

[ernstm, 2014-07-22 15:57:25]

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode5974
src/heap.cc:5974: GCTracer::Event::Event()
On 2014/07/22 15:21:30, Hannes Payer wrote:
> Why do we need the default constructor?

RingBuffer allocates an array of Events. The default constructor is required for
that. We can leave the Events uninitialized, though.

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6059
src/heap.cc:6059: if (collector == SCAVENGER)
On 2014/07/22 15:21:30, Hannes Payer wrote:
> coding style: if/else likes curly braces

Done.

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6088
src/heap.cc:6088: scavenger_events_.push_back(pending_);
On 2014/07/22 15:21:30, Hannes Payer wrote:
> coding style: if/else likes curly braces

Done.

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6130
src/heap.cc:6130: mark_compactor_events_.back()->end_time)
On 2014/07/22 15:21:30, Hannes Payer wrote:
> coding style: if/else likes curly braces

Done.

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode6159
src/heap.cc:6159: return last_in_other_buffer;
On 2014/07/22 15:21:30, Hannes Payer wrote:
> coding style: if/else likes curly braces

Done.

https://codereview.chromium.org/391413006/diff/40001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode607
src/heap.h:607: end_ = (end_ + 1) % (MAX_SIZE + 1);
On 2014/07/22 15:21:31, Hannes Payer wrote:
> Hmm, MAX_SIZE+1 makes the code really complicated. And, the anchor elements is
> just there until it gets overwritten (logically it is still there after that).
I
> am wondering if it wouldn't be cleaner if we would handle the case where there
> is no previous element externally... WDYT?

That is unrelated to anchor elements. The extra element is required to correctly
handle wrap around cases in the ring buffer. Without the extra element, we
couldn't distinguish an empty ring buffer from one that is completely filled
(begin_ == end_ in both cases) and the iterator pointing to the first element
would be identical to the end() iterator.

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode615
src/heap.h:615: size_t end_;
On 2014/07/22 15:21:31, Hannes Payer wrote:
> DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode676
src/heap.h:676: INVALID = 3
On 2014/07/22 15:21:30, Hannes Payer wrote:
> what is the purpose of invalid?

Not really required. See commend on default constructor of Event.

[Hannes Payer (out of office), 2014-07-22 16:29:12]

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.cc#newcode5974
src/heap.cc:5974: GCTracer::Event::Event()
On 2014/07/22 15:57:24, ernstm wrote:
> On 2014/07/22 15:21:30, Hannes Payer wrote:
> > Why do we need the default constructor?
> 
> RingBuffer allocates an array of Events. The default constructor is required
for
> that. We can leave the Events uninitialized, though.

Yes, I overlooked this one. Fine.

https://codereview.chromium.org/391413006/diff/40001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode607
src/heap.h:607: end_ = (end_ + 1) % (MAX_SIZE + 1);
On 2014/07/22 15:57:25, ernstm wrote:
> On 2014/07/22 15:21:31, Hannes Payer wrote:
> > Hmm, MAX_SIZE+1 makes the code really complicated. And, the anchor elements
is
> > just there until it gets overwritten (logically it is still there after
that).
> I
> > am wondering if it wouldn't be cleaner if we would handle the case where
there
> > is no previous element externally... WDYT?
> 
> That is unrelated to anchor elements. The extra element is required to
correctly
> handle wrap around cases in the ring buffer. Without the extra element, we
> couldn't distinguish an empty ring buffer from one that is completely filled
> (begin_ == end_ in both cases) and the iterator pointing to the first element
> would be identical to the end() iterator.

Ahm, yes. I guess we could also use a counter. But I am fine with what you have.

But, I am not really convinced that we should have an anchor element. It counts
as a real element, i.e., you would have to add special handling left and right o
filter out the element. Wouldn't it be just simpler to handle the empty case.

[ernstm, 2014-07-23 10:32:26]

https://codereview.chromium.org/391413006/diff/40001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/40001/src/heap.h#newcode607
src/heap.h:607: end_ = (end_ + 1) % (MAX_SIZE + 1);
> Ahm, yes. I guess we could also use a counter. But I am fine with what you
have.

The counter could be used to distinguish empty from full cases. The tricky part
is the iterators. With one extra slot we can always point the end iterator to an
empty slot.

 
> But, I am not really convinced that we should have an anchor element. It
counts
> as a real element, i.e., you would have to add special handling left and right
o
> filter out the element. Wouldn't it be just simpler to handle the empty case.

The current version explicitly tracks the current, previous and
previous_mark_compactor events on GCTracer (outside of the ring buffers). The
tracked events are initialized with a special START event that records the
statistics at construction time. The rest of the code handles the case of
insufficient event counts in a completely transparent way.

[Hannes Payer (out of office), 2014-07-23 14:35:16]

LGTM, just nits.

https://codereview.chromium.org/391413006/diff/80001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/80001/src/heap.cc#newcode6082
src/heap.cc:6082: Max(current_.start_time - previous_.end_time, 0.0);
Can this line be negative? That's strange, it should always be a positive value.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode550
src/heap.h:550: // TODO(ernstm): Move into GCTracer
"."

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode634
src/heap.h:634: // TODO(ernstm): unit tests. move to separate file.
Unit tests. Move to separate file.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode748
src/heap.h:748: typedef RingBuffer<Event, 10> EventBuffer;
Can we make 10 a proper constant.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode765
src/heap.h:765: // TODO(ernstm): move to Heap.
"Move"

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode769
src/heap.h:769: // TODO(ernstm): move to Heap.
"Move"

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode783
src/heap.h:783: Event previous_mark_compactor_;
previous_mark_compactor_event_

[ernstm, 2014-07-23 14:59:27]

https://codereview.chromium.org/391413006/diff/80001/src/heap.cc
File src/heap.cc (right):

https://codereview.chromium.org/391413006/diff/80001/src/heap.cc#newcode6082
src/heap.cc:6082: Max(current_.start_time - previous_.end_time, 0.0);
On 2014/07/23 14:35:15, Hannes Payer wrote:
> Can this line be negative? That's strange, it should always be a positive
value.

This was migrated from the old implementation of GCTracer. Not all clocks are
monotonic. Maybe this showed up as an issue when two events were back to back?
I'll leave it for now. We can still remove it in a subsequent CL.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h
File src/heap.h (right):

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode550
src/heap.h:550: // TODO(ernstm): Move into GCTracer
On 2014/07/23 14:35:15, Hannes Payer wrote:
> "."

Done.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode634
src/heap.h:634: // TODO(ernstm): unit tests. move to separate file.
On 2014/07/23 14:35:15, Hannes Payer wrote:
> Unit tests. Move to separate file.

Done.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode748
src/heap.h:748: typedef RingBuffer<Event, 10> EventBuffer;
On 2014/07/23 14:35:15, Hannes Payer wrote:
> Can we make 10 a proper constant.

Done.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode765
src/heap.h:765: // TODO(ernstm): move to Heap.
On 2014/07/23 14:35:15, Hannes Payer wrote:
> "Move"

Done.

https://codereview.chromium.org/391413006/diff/80001/src/heap.h#newcode769
src/heap.h:769: // TODO(ernstm): move to Heap.
On 2014/07/23 14:35:15, Hannes Payer wrote:
> "Move"

Done.

[ernstm, 2014-07-23 15:17:03]

Committed patchset #6 manually as r22567 (presubmit successful).