Precisely measure duration of mark and sweep phases. Changed print_cumulative_gc_stat flag to only …

Precisely measure duration of mark and sweep phases. Changed print_cumulative_gc_stat flag to only print GC statistics at end of program.

BUG=

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

[Michael Starzinger, 2013-01-28 13:55:38]

One round of comments.

As a high level comment: We might want to consider extending the lifetime of the
GCTracer to when incremental marking is started (to cover the whole GC cycle so
to speak). That way the incremental marking subsystem would be able to use the
GCTracer directly. But that is probably outside the scope of this CL. So I am
fine with this CL if comments are addressed.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.cc#newcode...
src/heap.cc:6909: heap_->incremental_marking()->steps_took_since_last_gc();
As discussed offline, the GCTracer should ask the incremental marking subsystem
about it's measurement instead of having the callbacks into the heap.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.h#newcode564
src/heap.h:564: void AddMarkingTime(double marking_time) {
Drop these callbacks.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.h#newcode2041
src/heap.h:2041: double marking_time_;
Move these counters into IncrementalMarking and the GCTracer respectively.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/incremental-mar...
File src/incremental-marking.cc (right):

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/incremental-mar...
src/incremental-marking.cc:770: heap_->AddMarkingTime(delta);
If you want to count the time we spend in Hurry() separately, I would suggest a
steps_took_in_hurry_ counter.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/incremental-mar...
src/incremental-marking.cc:986: heap_->AddMarkingTime(delta);
It seems this measures the same as steps_took_, I don't think we need a new
counter.

[Hannes Payer (out of office), 2013-01-29 12:01:54]

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.cc
File src/heap.cc (right):

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.cc#newcode...
src/heap.cc:6909: heap_->incremental_marking()->steps_took_since_last_gc();
I think the current solution is the "cleanest" hack, since the destructor of the
GCTracer and IncrementalMarking::ResetStepCounters() are not called
synchronously.

On 2013/01/28 13:55:39, Michael Starzinger wrote:
> As discussed offline, the GCTracer should ask the incremental marking
subsystem
> about it's measurement instead of having the callbacks into the heap.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.h#newcode564
src/heap.h:564: void AddMarkingTime(double marking_time) {
As discussed offline, I will mark them TODO remove.

On 2013/01/28 13:55:39, Michael Starzinger wrote:
> Drop these callbacks.

https://chromiumcodereview.appspot.com/11595006/diff/2001/src/heap.h#newcode2041
src/heap.h:2041: double marking_time_;
As discussed offline, they stay in heap.h

On 2013/01/28 13:55:39, Michael Starzinger wrote:
> Move these counters into IncrementalMarking and the GCTracer respectively.

[Michael Starzinger, 2013-01-29 12:13:47]

LGTM (if final nits are addressed).

As discussed offline: You convinced me that this is the least intrusive
temporary hack. And since we agree on how the design of the GCTracer should look
like in the end, I can live with this solution for now.

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode564
src/heap.h:564: // TODO(hpayer): remove, should be handled by GCTracer
Move all of these accessors down to where the others are declared.

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode1598
src/heap.h:1598: 
... to here.

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode2180
src/heap.h:2180: // GC time spent in marking
s/GC time/Cumulative GC time/

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode2183
src/heap.h:2183: // GC time spent in sweeping
Likewise.

[Hannes Payer (out of office), 2013-01-30 10:42:20]

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h
File src/heap.h (right):

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode564
src/heap.h:564: // TODO(hpayer): remove, should be handled by GCTracer
On 2013/01/29 12:13:47, Michael Starzinger wrote:
> Move all of these accessors down to where the others are declared.

Done.

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode1598
src/heap.h:1598: 
On 2013/01/29 12:13:47, Michael Starzinger wrote:
> ... to here.

Done.

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode2180
src/heap.h:2180: // GC time spent in marking
On 2013/01/29 12:13:47, Michael Starzinger wrote:
> s/GC time/Cumulative GC time/

Done.

https://chromiumcodereview.appspot.com/11595006/diff/5002/src/heap.h#newcode2183
src/heap.h:2183: // GC time spent in sweeping
On 2013/01/29 12:13:47, Michael Starzinger wrote:
> Likewise.

Done.