Oilpan: Add free list profiler.

Oilpan: Add free list profiler.

This patch is an attempt to land a part of keishi's free list profiler patch
(https://codereview.chromium.org/717923005/). Dumping mark/sweep stats is not
included in this patch; that will be implemented in the next CL.

This patch adds a functionality to dump statistics of free lists. The profiler
is guarded by ENABLE(GC_PROFILING), so you need to set a GYP variable to be
able to record the data. Since it is time-consuming to collect these data, the
profiler is disabled by default; you need to explicitly include a Telemetry
tracing category "disabled-by-default-blink_gc".

Some modifications are made to keishi's original patch, in addition to other
minor cleanups. Notable ones are:

* "FreeList" data is now in TRACE_DISABLED_BY_DEFAULT("blink_gc") category.
* FreeList snapshot timer is stored as a member of ThreadState instead of
  a static variable.

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

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

[kouhei (in TOK), 2015-02-04 06:46:10]

kouhei@chromium.org changed reviewers:
+ kouhei@chromium.org

[kouhei (in TOK), 2015-02-04 06:46:10]

lgtm

[keishi, 2015-02-04 06:58:26]

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1514: json.setDouble("totalAllocationSize",
m_totalAllocationSize);
nit: I think this could be renamed to cummulativeAllocationSize to make the
distinction from s_allocatedObjectSize clearer.

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1519: json.setDouble("averageAllocationPointSize",
static_cast<double>(m_allocationPointSizeSum) / m_setAllocationPointCount);
averageAllocationPointSize hasn't been a useful metric so far so I think we
should remove it.

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1531: size_t bucketTotalSizes[blinkPageSizeLog2];
I'm sorry. bucketSizes and bucketTotalSizes were horrible names. Hard to
understand the difference between them. bucketSizes should be something like
entryCountPerBucket. And bucketTotalSizes should be something like
totalFreeSizePerBucket.

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1350: {
Shouldn't we be updating m_totalAllocationSize here?

[peria, 2015-02-04 07:07:27]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2015-02-04 07:07:27]

Could you add a profiler for (disabled-by-default-)blink_gc category in
Telemetry? (in another CL, of course.)

[haraken, 2015-02-04 07:30:01]

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:552: , m_totalAllocationSize(0.0)

Where is this used?

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:553: , m_allocationCount(0)

Ditto.

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:554: , m_inlineAllocationCount(0)

Ditto.

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1519: json.setDouble("averageAllocationPointSize",
static_cast<double>(m_allocationPointSizeSum) / m_setAllocationPointCount);
On 2015/02/04 06:58:26, keishi wrote:
> averageAllocationPointSize hasn't been a useful metric so far so I think we
> should remove it.

Agreed. Also I'm not sure how helpful m_setAllocationPointCount is.

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:1210: SNAPSHOT_FREE_LIST(VectorBacking);

Add SNAPSHOT_FREE_LIST(InlineVectorBacking).

[Yuta Kitamura, 2015-02-04 09:45:04]

New patchsets have been uploaded after l-g-t-m from kouhei@chromium.org

[Yuta Kitamura, 2015-02-04 09:45:42]

PTAL

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1514: json.setDouble("totalAllocationSize",
m_totalAllocationSize);
On 2015/02/04 06:58:26, keishi wrote:
> nit: I think this could be renamed to cummulativeAllocationSize to make the
> distinction from s_allocatedObjectSize clearer.

Fixed. Can you fix your dashboard code, too?

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1519: json.setDouble("averageAllocationPointSize",
static_cast<double>(m_allocationPointSizeSum) / m_setAllocationPointCount);
On 2015/02/04 07:30:00, haraken wrote:
> On 2015/02/04 06:58:26, keishi wrote:
> > averageAllocationPointSize hasn't been a useful metric so far so I think we
> > should remove it.
> 
> Agreed. Also I'm not sure how helpful m_setAllocationPointCount is.

Done, m_allocationPointSizeSum and m_setAllocationPointCount are
removed too.

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1531: size_t bucketTotalSizes[blinkPageSizeLog2];
On 2015/02/04 06:58:26, keishi wrote:
> I'm sorry. bucketSizes and bucketTotalSizes were horrible names. Hard to
> understand the difference between them. bucketSizes should be something like
> entryCountPerBucket. And bucketTotalSizes should be something like
> totalFreeSizePerBucket.

Fixed, can you take another look at this version?
I've introduced struct PerBucketFreeListStats so people
can understand what each value means...

Metrics names renamed:
"bucketSizes" -> "perBucketEntryCount"
"bucketTotalSizes" -> "perBucketFreeSize"

If this looks okay to you, can you apply the change
to your dashboard?

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.h#...
Source/platform/heap/Heap.h:1350: {
On 2015/02/04 06:58:26, keishi wrote:
> Shouldn't we be updating m_totalAllocationSize here?

Do'h, you are right. Fixed.

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/ThreadS...
Source/platform/heap/ThreadState.cpp:1210: SNAPSHOT_FREE_LIST(VectorBacking);
On 2015/02/04 07:30:01, haraken wrote:
> 
> Add SNAPSHOT_FREE_LIST(InlineVectorBacking).

Done.

[Yuta Kitamura, 2015-02-04 09:49:02]

On 2015/02/04 07:07:27, peria wrote:
> Could you add a profiler for (disabled-by-default-)blink_gc category in
> Telemetry? (in another CL, of course.)

In theory, --profiler=trace-all should be fine, but,
for the moment, "run_benchmark --profiler=trace-all"
raises a Python exception at the end of the test :/

--profiler=trace-all is too heavy, though, so adding
a new profiler shortcut seems reasonable. Will do.
(Actually, keishi's original patch included that, too.)

[keishi, 2015-02-04 11:05:40]

LGTM

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

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1514: json.setDouble("totalAllocationSize",
m_totalAllocationSize);
On 2015/02/04 09:45:42, Yuta Kitamura wrote:
> On 2015/02/04 06:58:26, keishi wrote:
> > nit: I think this could be renamed to cummulativeAllocationSize to make the
> > distinction from s_allocatedObjectSize clearer.
> 
> Fixed. Can you fix your dashboard code, too?

Done.

https://codereview.chromium.org/883233003/diff/1/Source/platform/heap/Heap.cp...
Source/platform/heap/Heap.cpp:1531: size_t bucketTotalSizes[blinkPageSizeLog2];
On 2015/02/04 09:45:42, Yuta Kitamura wrote:
> On 2015/02/04 06:58:26, keishi wrote:
> > I'm sorry. bucketSizes and bucketTotalSizes were horrible names. Hard to
> > understand the difference between them. bucketSizes should be something like
> > entryCountPerBucket. And bucketTotalSizes should be something like
> > totalFreeSizePerBucket.
> 
> Fixed, can you take another look at this version?
> I've introduced struct PerBucketFreeListStats so people
> can understand what each value means...
> 
> Metrics names renamed:
> "bucketSizes" -> "perBucketEntryCount"
> "bucketTotalSizes" -> "perBucketFreeSize"
> 
> If this looks okay to you, can you apply the change
> to your dashboard?

Thanks, this is much better. Done.

[haraken, 2015-02-04 14:14:57]

LGTM

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:832: double m_cumulativeAllocationSize;

double => size_t ?

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:1188: static const double
recordIntervalSeconds = 0.010;

I'm not sure if 10 ms is a good interval for snapshotting free lists since the
result would become non-deterministic. Probably would it be better to snapshot
free lists every time we allocate a new page?

[Yuta Kitamura, 2015-02-05 04:13:11]

New patchsets have been uploaded after l-g-t-m from
keishi@chromium.org,haraken@chromium.org

[Yuta Kitamura, 2015-02-05 04:13:41]

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Heap.h
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:832: double m_cumulativeAllocationSize;
On 2015/02/04 14:14:57, haraken wrote:
> 
> double => size_t ?

Done.

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:1188: static const double
recordIntervalSeconds = 0.010;
On 2015/02/04 14:14:57, haraken wrote:
> 
> I'm not sure if 10 ms is a good interval for snapshotting free lists since the
> result would become non-deterministic. Probably would it be better to snapshot
> free lists every time we allocate a new page?

snapshotFreeListIfNecessary() is called at the end of
postGCProcessing() and at the beginnng of ThreadHeap::
outOfLineAllocate(). 10ms check is done after that, i.e.
free lists are recorded only when these events have
happened *and* 10ms has passed after the last recording.

I'm not sure which events (postGCProcessing+outOfLineAlloc
or page allocation) are appropriate for a trigger, though.

Is it okay to commit the patch as-is and fix the trigger
condition later if necessary?

[haraken, 2015-02-05 04:15:46]

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Thr...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/883233003/diff/20001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:1188: static const double
recordIntervalSeconds = 0.010;
On 2015/02/05 04:13:41, Yuta Kitamura wrote:
> On 2015/02/04 14:14:57, haraken wrote:
> > 
> > I'm not sure if 10 ms is a good interval for snapshotting free lists since
the
> > result would become non-deterministic. Probably would it be better to
snapshot
> > free lists every time we allocate a new page?
> 
> snapshotFreeListIfNecessary() is called at the end of
> postGCProcessing() and at the beginnng of ThreadHeap::
> outOfLineAllocate(). 10ms check is done after that, i.e.
> free lists are recorded only when these events have
> happened *and* 10ms has passed after the last recording.
> 
> I'm not sure which events (postGCProcessing+outOfLineAlloc
> or page allocation) are appropriate for a trigger, though.
> 
> Is it okay to commit the patch as-is and fix the trigger
> condition later if necessary?

Sure. Let's fix it in a follow-up if necessary.

I'm not concerned about the overhead but am just wondering if an interval-based
profiling might lead to flakiness of the profiled results.

[Yuta Kitamura, 2015-02-05 06:18:58]

The CQ bit was checked by yutak@chromium.org

[commit-bot: I haz the power, 2015-02-05 06:19:44]

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

[commit-bot: I haz the power, 2015-02-05 15:30:11]

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