Oilpan: Implement mark/sweep stats collection for free list profiler.

Oilpan: Implement mark/sweep stats collection for free list profiler.

This patch is a follow-up of https://codereview.chromium.org/883233003.
It implements a data collector of marking and sweeping stats, completing all
the features that keishi's original WIP patch has implemented.

The code structure has radically changed from keishi's original patch in order
to organize the profiling code consistently. Especially, keishi's original
patch hooked the marking process to count marked objects, but in my patch
these objects are counted after all objects are marked (in postGC()), at which
point sweeping stats are collected, too. To align with these, existing code
for heap snapshotting is also moved to postGC().

BUG=454273
R=haraken@chromium.org, keishi@chromium.org, oilpan-reviews@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189638

[haraken, 2015-02-06 06:39:20]

Mostly looks good.

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1841: HeapObjectHeader* header = nullptr;

You can declare HeapObjectHeader* inside the loop. The same comment for other
methods in this file.

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:1321: if (!isMainThread())

Why do we need to have the isMainThread check? ThreadState::postGC() is called
only by one GCing thread (which may not be the main thread), meaning that
collectAndReportMarkSweepStats() is called only by the thread. Thus we don't
need to worry about thread races.

[Yuta Kitamura, 2015-02-06 07:45:45]

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/Heap.cpp
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1841: HeapObjectHeader* header = nullptr;
On 2015/02/06 06:39:20, haraken wrote:
> 
> You can declare HeapObjectHeader* inside the loop. The same comment for other
> methods in this file.
> 

It is used in the third part of the "for" expression
below.

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:1321: if (!isMainThread())
On 2015/02/06 06:39:20, haraken wrote:
> 
> Why do we need to have the isMainThread check? ThreadState::postGC() is called
> only by one GCing thread (which may not be the main thread), meaning that
> collectAndReportMarkSweepStats() is called only by the thread. Thus we don't
> need to worry about thread races.
> 

The reason is that data from non-main thread makes
the graph look strange (many objects are marked/swept in
main threads, while almost zero objects are marked/swept
in non-main threads, making the graph look zig-zag).

In the long term, we could extend the data format and
keishi's dashboard to allow thread-specific counts, but
that's something that we don't have right now AFAIK.

[haraken, 2015-02-06 07:51:50]

LGTM

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:1321: if (!isMainThread())
On 2015/02/06 07:45:45, Yuta Kitamura wrote:
> On 2015/02/06 06:39:20, haraken wrote:
> > 
> > Why do we need to have the isMainThread check? ThreadState::postGC() is
called
> > only by one GCing thread (which may not be the main thread), meaning that
> > collectAndReportMarkSweepStats() is called only by the thread. Thus we don't
> > need to worry about thread races.
> > 
> 
> The reason is that data from non-main thread makes
> the graph look strange (many objects are marked/swept in
> main threads, while almost zero objects are marked/swept
> in non-main threads, making the graph look zig-zag).
> 
> In the long term, we could extend the data format and
> keishi's dashboard to allow thread-specific counts, but
> that's something that we don't have right now AFAIK.

Thanks, makes sense.

[keishi, 2015-02-06 08:21:08]

LGTM

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/903033003/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:1321: if (!isMainThread())
On 2015/02/06 07:51:50, haraken wrote:
> On 2015/02/06 07:45:45, Yuta Kitamura wrote:
> > On 2015/02/06 06:39:20, haraken wrote:
> > > 
> > > Why do we need to have the isMainThread check? ThreadState::postGC() is
> called
> > > only by one GCing thread (which may not be the main thread), meaning that
> > > collectAndReportMarkSweepStats() is called only by the thread. Thus we
don't
> > > need to worry about thread races.
> > > 
> > 
> > The reason is that data from non-main thread makes
> > the graph look strange (many objects are marked/swept in
> > main threads, while almost zero objects are marked/swept
> > in non-main threads, making the graph look zig-zag).
> > 
> > In the long term, we could extend the data format and
> > keishi's dashboard to allow thread-specific counts, but
> > that's something that we don't have right now AFAIK.
> 
> Thanks, makes sense.

All records in the trace json file have tid fields and I did write code to
filter out non main thread records. If this makes a difference I think that
would be a bug in my viewer.

[Yuta Kitamura, 2015-02-06 10:59:46]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2015-02-06 11:00:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/903033003/1

[commit-bot: I haz the power, 2015-02-06 12:56:52]

Committed patchset #1 (id:1) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189638