Oilpan: GC profiling.

Oilpan: GC profiling.

This CL adds GC memory profiling using the about:tracing infrastructure. The
profiling can be time consuming, so the GC_PROFILE_* macros are used to
selectively enable various profiling features for the time being.

The GC_TRACING macro has been renamed to GC_PROFILE_MARKING to avoid confusion with about:tracing. The tracing event group has been renamed from blink to blink_gc to allow collection of GC related events independent of other blink events.

BUG=

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

[zerny-chromium, 2014-07-11 09:14:44]

This CL depends on https://codereview.chromium.org/381653002/

I've split up the profiling macros and renamed GC_TRACING to GC_PROFILE_MARKING
to cause less confusion between GC mark/trace and profiling with tracing events.

Snapshotting full headers of all objects on our pages results in too much data,
so it is limited by GC_PROFILE_HEAP_PAGE_DUMP_THRESHOLD (default 0) for the time
being.

I'll send out patch-set for a work-in-progress extension of the trace-viewer
display to oilpan-reviews that can be manually applied to
third_party/trace-viewer.

The export changes are not needed for this CL but are missing if anyone wants to
use the facilities in a component build so I've kept them in the CL.

[Mads Ager (chromium), 2014-07-11 11:26:54]

LGTM

https://codereview.chromium.org/383743002/diff/40001/Source/build/features.gypi
File Source/build/features.gypi (right):

https://codereview.chromium.org/383743002/diff/40001/Source/build/features.gy...
Source/build/features.gypi:46: 'gc_profiling%': 0
We might want to split this into two separate things. One for just tracing
marking and one for gathering profiling information?

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

https://codereview.chromium.org/383743002/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:308: NO_SANITIZE_ADDRESS size_t age() const { return
m_size >> 27; }
Let's consistently add NO_SANITIZE_ADDRESS on a line on its own?

https://codereview.chromium.org/383743002/diff/40001/Source/platform/heap/Pro...
File Source/platform/heap/Profiling.h (right):

https://codereview.chromium.org/383743002/diff/40001/Source/platform/heap/Pro...
Source/platform/heap/Profiling.h:2: * Copyright (C) 2013 Google Inc. All rights
reserved.
2014, and we should probably use the short copyright notice?

[zerny-chromium, 2014-07-11 11:56:38]

Thanks for the review. Fixes uploaded.

https://codereview.chromium.org/383743002/diff/40001/Source/build/features.gypi
File Source/build/features.gypi (right):

https://codereview.chromium.org/383743002/diff/40001/Source/build/features.gy...
Source/build/features.gypi:46: 'gc_profiling%': 0
On 2014/07/11 11:26:54, Mads Ager (chromium) wrote:
> We might want to split this into two separate things. One for just tracing
> marking and one for gathering profiling information?

Acknowledged.

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

https://codereview.chromium.org/383743002/diff/40001/Source/platform/heap/Hea...
Source/platform/heap/Heap.h:308: NO_SANITIZE_ADDRESS size_t age() const { return
m_size >> 27; }
On 2014/07/11 11:26:54, Mads Ager (chromium) wrote:
> Let's consistently add NO_SANITIZE_ADDRESS on a line on its own?

Acknowledged.

https://codereview.chromium.org/383743002/diff/40001/Source/platform/heap/Pro...
File Source/platform/heap/Profiling.h (right):

https://codereview.chromium.org/383743002/diff/40001/Source/platform/heap/Pro...
Source/platform/heap/Profiling.h:2: * Copyright (C) 2013 Google Inc. All rights
reserved.
On 2014/07/11 11:26:54, Mads Ager (chromium) wrote:
> 2014, and we should probably use the short copyright notice?

Acknowledged.

[haraken, 2014-07-14 02:26:14]

Mostly looks good.

https://codereview.chromium.org/383743002/diff/100001/Source/build/features.gypi
File Source/build/features.gypi (right):

https://codereview.chromium.org/383743002/diff/100001/Source/build/features.g...
Source/build/features.gypi:93: 'ENABLE_GC_PROFILING_HEAP=1',

Given that we use ENABLE_OILPAN, shall we use ENABLE_OILPAN_PROFILING_HEAP and
ENABLE_OILPAN_PROFILING_MARKING ?

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1295: header->incAge();

Do we want to increase the age when age == maxHeapObjectAge-1?

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1381: header->incAge();

Do we want to increase the age when age == maxHeapObjectAge-1?

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1388: header->incAge();

Factor out the line 1378 - 1381 and the line 1385 - 1388 to the outside of the
if branch.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.h:309: size_t age() const { return m_size >> 27; }

Can we avoid hard-coding 27?

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
File Source/platform/heap/Profiling.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
Source/platform/heap/Profiling.h:9: #define GC_PROFILE_GROUP "BlinkGC"

"BlinkGC" => "blinkGC" (due to a convention of trace event names)

Also we normally hard-code the name when using trace event macros.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
Source/platform/heap/Profiling.h:12: #define GC_PROFILE_HEAP
ENABLE(GC_PROFILING_HEAP)

Blink normally doesn't use this kind of re-define. Shall we use
ENABLE(GC_PROFILING_HEAP) as is?

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
Source/platform/heap/Profiling.h:16: #define GC_PROFILE_MARKING
ENABLE(GC_PROFILING_MARKING)

Ditto.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:529: SnapshotInfo(ThreadState* state) :
state(state), liveSize(0), deadSize(0), freeSize(0), pageCount(0) { }

Add explicit.

[zerny-chromium, 2014-07-28 11:54:42]

Thanks for the review! A follow-up patch is pushed to address some of your
comments and to add reasoning to those not fixed. PTAL.

https://codereview.chromium.org/383743002/diff/100001/Source/build/features.gypi
File Source/build/features.gypi (right):

https://codereview.chromium.org/383743002/diff/100001/Source/build/features.g...
Source/build/features.gypi:93: 'ENABLE_GC_PROFILING_HEAP=1',
On 2014/07/14 02:26:14, haraken wrote:
> 
> Given that we use ENABLE_OILPAN, shall we use ENABLE_OILPAN_PROFILING_HEAP and
> ENABLE_OILPAN_PROFILING_MARKING ?

I don't think so. Oilpan is the project code name and should eventually not
appear in the code at all. It is also badly named since it really only affects
transition types. I think using 'GC' is better here since these will stay (just
as for the plugin).

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1295: header->incAge();
On 2014/07/14 02:26:14, haraken wrote:
> 
> Do we want to increase the age when age == maxHeapObjectAge-1?

No, the increment check against maxHeapObjectAge is in incAge().

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1381: header->incAge();
On 2014/07/14 02:26:14, haraken wrote:
> 
> Do we want to increase the age when age == maxHeapObjectAge-1?

Ditto.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1388: header->incAge();
On 2014/07/14 02:26:14, haraken wrote:
> 
> Factor out the line 1378 - 1381 and the line 1385 - 1388 to the outside of the
> if branch.

This is a copy-paste error. The dead-branch should not increment the age. Fixed.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
File Source/platform/heap/Heap.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.h:309: size_t age() const { return m_size >> 27; }
On 2014/07/14 02:26:14, haraken wrote:
> 
> Can we avoid hard-coding 27?

Done.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
File Source/platform/heap/Profiling.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
Source/platform/heap/Profiling.h:9: #define GC_PROFILE_GROUP "BlinkGC"
On 2014/07/14 02:26:14, haraken wrote:
> 
> "BlinkGC" => "blinkGC" (due to a convention of trace event names)
> 
> Also we normally hard-code the name when using trace event macros.

Done, but kept the macro so we avoid search-replace on a string key for a const
name that will likely change again.

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
Source/platform/heap/Profiling.h:12: #define GC_PROFILE_HEAP
ENABLE(GC_PROFILING_HEAP)
On 2014/07/14 02:26:14, haraken wrote:
> 
> Blink normally doesn't use this kind of re-define. Shall we use
> ENABLE(GC_PROFILING_HEAP) as is?
> 

I can do that. The reasoning for having the two distinct types was that we can
more easily split the GC_PROFILE_HEAP into several sub #defs (such as _FAST and
_SLOW) independent of the GYP flag/naming. Should I change it or keep it?

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.h:529: SnapshotInfo(ThreadState* state) :
state(state), liveSize(0), deadSize(0), freeSize(0), pageCount(0) { }
On 2014/07/14 02:26:14, haraken wrote:
> 
> Add explicit.

Done.

[zerny-chromium, 2014-07-28 11:57:59]

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:1295: header->incAge();
On 2014/07/28 11:54:41, zerny-chromium wrote:
> On 2014/07/14 02:26:14, haraken wrote:
> > 
> > Do we want to increase the age when age == maxHeapObjectAge-1?
> 
> No, the increment check against maxHeapObjectAge is in incAge().

(Too quick there) We do want to increment when it is max-1 which will push it
over to the then stable "max age". We count "live objects" exactly once exactly
when the age goes from maxHeapObjectAge-1 to maxHeapObjectAge.

[haraken, 2014-07-28 17:45:48]

LGTM

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
File Source/platform/heap/Profiling.h (right):

https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
Source/platform/heap/Profiling.h:12: #define GC_PROFILE_HEAP
ENABLE(GC_PROFILING_HEAP)
On 2014/07/28 11:54:41, zerny-chromium wrote:
> On 2014/07/14 02:26:14, haraken wrote:
> > 
> > Blink normally doesn't use this kind of re-define. Shall we use
> > ENABLE(GC_PROFILING_HEAP) as is?
> > 
> 
> I can do that. The reasoning for having the two distinct types was that we can
> more easily split the GC_PROFILE_HEAP into several sub #defs (such as _FAST
and
> _SLOW) independent of the GYP flag/naming. Should I change it or keep it?

If the split does really matter, I'm OK with introducing GC_PROFILE_HEAP.
However, otherwise I'd prefer just using ENABLE(GC_PROFILING_HEAP), following
other Blink code base. I guess it's important to follow existing conventions.

Actually I hope this file is gone since this file is just redefining macros.

[zerny-chromium, 2014-07-29 12:12:34]

On 2014/07/28 17:45:48, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
> File Source/platform/heap/Profiling.h (right):
> 
>
https://codereview.chromium.org/383743002/diff/100001/Source/platform/heap/Pr...
> Source/platform/heap/Profiling.h:12: #define GC_PROFILE_HEAP
> ENABLE(GC_PROFILING_HEAP)
> On 2014/07/28 11:54:41, zerny-chromium wrote:
> > On 2014/07/14 02:26:14, haraken wrote:
> > > 
> > > Blink normally doesn't use this kind of re-define. Shall we use
> > > ENABLE(GC_PROFILING_HEAP) as is?
> > > 
> > 
> > I can do that. The reasoning for having the two distinct types was that we
can
> > more easily split the GC_PROFILE_HEAP into several sub #defs (such as _FAST
> and
> > _SLOW) independent of the GYP flag/naming. Should I change it or keep it?
> 
> If the split does really matter, I'm OK with introducing GC_PROFILE_HEAP.
> However, otherwise I'd prefer just using ENABLE(GC_PROFILING_HEAP), following
> other Blink code base. I guess it's important to follow existing conventions.
> 
> Actually I hope this file is gone since this file is just redefining macros.

Thanks for the feedback. I've replaced the macros and removed Profiling.h

[zerny-chromium, 2014-08-04 07:40:01]

tkent: could I ping you for an owners review? (also, let me know if you'd rather
see the wtf exports in a separate CL?)

[tkent, 2014-08-05 00:36:57]

> I've split up the profiling macros and renamed GC_TRACING to
GC_PROFILE_MARKING
> to cause less confusion between GC mark/trace and profiling with tracing
events.

The CL description should mention the rename.
Also, it should mention the trace event category name change.

> tkent: could I ping you for an owners review? (also, let me know if you'd
rather
> see the wtf exports in a separate CL?)

Smaller patches are better, and a CL should not contain unrelated changes.

[tkent, 2014-08-05 00:38:10]

https://codereview.chromium.org/383743002/diff/160001/Source/platform/heap/He...
File Source/platform/heap/Heap.cpp (right):

https://codereview.chromium.org/383743002/diff/160001/Source/platform/heap/He...
Source/platform/heap/Heap.cpp:375: TRACE_EVENT0("blinkGC", "Heap::GCScope");
should be "blink_gc" for consistency with "blink_rendering".

https://codereview.chromium.org/383743002/diff/160001/Source/platform/heap/Th...
File Source/platform/heap/ThreadState.cpp (right):

https://codereview.chromium.org/383743002/diff/160001/Source/platform/heap/Th...
Source/platform/heap/ThreadState.cpp:954:
TRACE_EVENT_CATEGORY_GROUP_ENABLED("blinkGC", &gcTracingEnabled);
Ditto.

[zerny-chromium, 2014-08-05 07:28:05]

Thanks for the review. Split out the wtf exports into another CL, updated the
description and renamed the trace-group name. PTAL

[tkent, 2014-08-05 07:30:44]

lgtm

[zerny-chromium, 2014-08-05 07:43:34]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-05 07:45:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/383743002/180001

[zerny-chromium, 2014-08-05 07:57:36]

The CQ bit was unchecked by zerny@chromium.org

[zerny-chromium, 2014-08-05 07:59:39]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-05 08:01:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/383743002/200001

[commit-bot: I haz the power, 2014-08-05 13:13:51]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/1...)
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20859)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42496)

[commit-bot: I haz the power, 2014-08-05 13:31:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-05 13:31:39]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/20930)

[tkent, 2014-08-05 22:55:51]

The CQ bit was checked by tkent@chromium.org

[commit-bot: I haz the power, 2014-08-05 22:57:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/383743002/200001

[commit-bot: I haz the power, 2014-08-05 23:52:18]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21151)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/42911)

[commit-bot: I haz the power, 2014-08-06 00:32:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 00:32:28]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)

[zerny-chromium, 2014-08-06 08:19:41]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-06 08:20:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/383743002/200001

[commit-bot: I haz the power, 2014-08-06 08:37:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21302)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/43099)

[commit-bot: I haz the power, 2014-08-06 08:45:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-06 08:45:26]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_compile_dbg/bu...)
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/21305)

[zerny-chromium, 2014-08-14 11:44:13]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-14 11:45:21]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/383743002/220001

[zerny-chromium, 2014-08-14 12:22:23]

The CQ bit was unchecked by zerny@chromium.org

[zerny-chromium, 2014-08-14 12:22:57]

It appears that the TracedValue structures have been pulled out again, so this
needs to be refactored (again).

[zerny-chromium, 2014-08-14 17:58:41]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-08-14 17:58:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/zerny@chromium.org/383743002/240001

[commit-bot: I haz the power, 2014-08-14 19:09:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/22803)

[commit-bot: I haz the power, 2014-08-14 20:16:29]

Committed patchset #13 (240001) as 180297