Add UMAs about Blink GC metrics

Add UMAs about Blink GC metrics

This CL adds the following metrics to UMAs:

- Duration of time consumed in Heap::collectGarbage().
- Duration of time consumed in ThreadState::performPendingSweep().
- The total size of object space in all threads when Heap::collectGarbage() is triggered.

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

[haraken, 2014-05-28 09:02:44]

Chromium side CL is here:
https://codereview.chromium.org/302433014

PTAL.

[Mads Ager (chromium), 2014-05-28 09:35:44]

Looks great. Can we add totalAllocatedSpaceInAllThreads as a separate stat as
well?

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1702: uint64_t
Heap::totalObjectSpaceInAllThreads()
Let do totalAllocatedSpaceInAllThreads and report that to UMA as well? That will
be very helpful to figure out how much fragmentation we have. If we have a very
small amount of totalObjectSpace and a much, much larger value for
totalAllocatedSpace we have a lot of fragmentation.

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

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:826: timeStamp = WTF::currentTimeMS();
Is histogram collection also not thread safe? It would have been nice if we
could have histograms for all sweeps (also the ones not on the main thread).

[haraken, 2014-05-28 09:40:15]

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

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:826: timeStamp = WTF::currentTimeMS();
On 2014/05/28 09:35:45, Mads Ager (chromium) wrote:
> Is histogram collection also not thread safe? It would have been nice if we
> could have histograms for all sweeps (also the ones not on the main thread).

It's thread-safe.

At first, I thought that if we collect sweeping time from other threads
(workers, database threads etc), the histogram distribution might be biased by
those non-main threads which will allocate very little objects. So I thought it
might be better to limit the UMA to the main thread.

Either is fine with me.

[Mads Ager (chromium), 2014-05-28 10:01:40]

On 2014/05/28 09:40:15, haraken wrote:
>
https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Thr...
> File Source/platform/heap/ThreadState.cpp (right):
> 
>
https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Thr...
> Source/platform/heap/ThreadState.cpp:826: timeStamp = WTF::currentTimeMS();
> On 2014/05/28 09:35:45, Mads Ager (chromium) wrote:
> > Is histogram collection also not thread safe? It would have been nice if we
> > could have histograms for all sweeps (also the ones not on the main thread).
> 
> It's thread-safe.
> 
> At first, I thought that if we collect sweeping time from other threads
> (workers, database threads etc), the histogram distribution might be biased by
> those non-main threads which will allocate very little objects. So I thought
it
> might be better to limit the UMA to the main thread.
> 
> Either is fine with me.

SGTM, either is fine with me as well. In order to remove the bias we can add a
separate category for non-main thread sweeps. We can do that later as well.

[haraken, 2014-05-28 10:40:26]

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1702: uint64_t
Heap::totalObjectSpaceInAllThreads()
On 2014/05/28 09:35:45, Mads Ager (chromium) wrote:
> Let do totalAllocatedSpaceInAllThreads and report that to UMA as well? That
will
> be very helpful to figure out how much fragmentation we have. If we have a
very
> small amount of totalObjectSpace and a much, much larger value for
> totalAllocatedSpace we have a lot of fragmentation.

Done.

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

https://codereview.chromium.org/304673003/diff/40001/Source/platform/heap/Thr...
Source/platform/heap/ThreadState.cpp:826: timeStamp = WTF::currentTimeMS();
On 2014/05/28 09:35:45, Mads Ager (chromium) wrote:
> Is histogram collection also not thread safe? It would have been nice if we
> could have histograms for all sweeps (also the ones not on the main thread).

Done.

[haraken, 2014-05-28 10:40:32]

PTAL

[Mads Ager (chromium), 2014-05-28 12:39:56]

LGTM

[haraken, 2014-05-28 12:43:37]

Alexei, Ilya: Would you mind taking a look?

The corresponding Chromium-side CL is here:
https://codereview.chromium.org/302433014/

[Alexei Svitkine (slow), 2014-05-28 12:53:28]

https://codereview.chromium.org/304673003/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/304673003/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1684:
blink::Platform::current()->histogramEnumeration("BlinkGC.CollectGarbage",
WTF::currentTimeMS() - timeStamp, 30000);
You shouldn't use a histogramEnumeration() macro for these.

Since you're measuring latency, you want an exponential bucket sizes histogram,
not a linear one as would be provided by the enumeration faction.

Probably, you want the equivalent of UMA_HISTOGRAM_TIMES macro, which you can
probably craft up using histogramCustomCounts(). Maybe it's even worth adding a
new function wrapping histogramCustomCounts() so you don't have to manually
specify the params for that.

https://codereview.chromium.org/304673003/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1685:
blink::Platform::current()->histogramEnumeration("BlinkGC.TotalObjectSpace",
objectSpaceSize / 1024, 8 * 1024 * 1024);
Likewise, this should also use histogramCustomCounts().

[haraken, 2014-05-28 13:31:41]

Thanks for quick reivew! Updated the CL.

https://codereview.chromium.org/304673003/diff/60001/Source/platform/heap/Hea...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/304673003/diff/60001/Source/platform/heap/Hea...
Source/platform/heap/Heap.cpp:1684:
blink::Platform::current()->histogramEnumeration("BlinkGC.CollectGarbage",
WTF::currentTimeMS() - timeStamp, 30000);
On 2014/05/28 12:53:29, Alexei Svitkine wrote:
> You shouldn't use a histogramEnumeration() macro for these.
> 
> Since you're measuring latency, you want an exponential bucket sizes
histogram,
> not a linear one as would be provided by the enumeration faction.
> 
> Probably, you want the equivalent of UMA_HISTOGRAM_TIMES macro, which you can
> probably craft up using histogramCustomCounts(). Maybe it's even worth adding
a
> new function wrapping histogramCustomCounts() so you don't have to manually
> specify the params for that.

Done.

Creating a macro sounds nice, but as far as I see the Blink code base, various
places are using histogramCustomCounts with different parameters. I guess it
would be problematic to change the existing parameters, and if so it looks a bit
hard to unify those histogramCustomCounts into one macro.

[Alexei Svitkine (slow), 2014-05-28 19:27:34]

lgtm

[haraken, 2014-05-28 23:14:52]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-05-28 23:15:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/haraken@chromium.org/304673003/70001

[commit-bot: I haz the power, 2014-05-29 00:32:57]

Change committed as 175016