Oilpan: Add trace events for the GC marking and sweeping phases.

Oilpan: Add trace events for the GC marking and sweeping phases.

R=ager@chromium.org,haraken@chromium.org
BUG=

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

[Mads Ager (chromium), 2014-09-11 09:06:29]

LGTM

[zerny-chromium, 2014-09-11 09:07:04]

FYI, using the patch below allows running telemetry runs that will dump trace
files (also for android). For example:

  ./tools/perf/run_benchmark blink_perf.all \
    --profiler=trace-blinkgc \
    --browser=exact \
    --browser-executable=~/ssd/chromium/src/out_$out/Release/content_shell \
    --page-filter=SomeTest

Will generate a json dump file that you can load in chrome about:tracing.


diff --git a/tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py
b/tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py
index 089b038..7707675 100644
--- a/tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py
+++ b/tools/telemetry/telemetry/core/platform/profiler/trace_profiler.py
@@ -70,3 +70,13 @@ class TraceAllProfiler(TraceProfiler):
   @classmethod
   def name(cls):
     return 'trace-all'
+
+
+class TraceBlinkGCProfiler(TraceProfiler):
+  def __init__(self, browser_backend, platform_backend, output_path, state):
+    super(TraceBlinkGCProfiler, self).__init__(
+        browser_backend, platform_backend, output_path, state,
+        categories='blink_gc,webkit.console,benchmark,blink.console')
+  @classmethod
+  def name(cls):
+    return 'trace-blinkgc'

[wibling-chromium, 2014-09-11 09:07:52]

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

[wibling-chromium, 2014-09-11 09:07:52]

lgtm

[haraken, 2014-09-11 09:16:35]

LGTM

Do you really need to distinguish NormalGC from ForcedGC in your profiling?

- Regarding Heap::collectGarbage triggered in V8's GC, it would be clear that
it's a forced Heap::collectGarbage because the trace result appears in the trace
result of V8's GC.

- It wouldn't be that helpful to add ForcedGC to Heap::collectAllGarbage, since
Heap::collectAllGarbage is only used in testing. If you want to use the ForcedGC
flag in testing as well, you'll need to add ForcedGC to all other
Heap::collectGarbages used in unittests.

If the distinction of NormalGC or ForcedGC is not that important, I'd drop the
enum parameter.

[zerny-chromium, 2014-09-11 17:52:47]

Thanks for the review.

> Do you really need to distinguish NormalGC from ForcedGC in your profiling?

No, it is somewhat clear from the GC pattern, but this data can be used to
output other summarized info which would need to know which GCs are forced (and
therefore not of interest) and which are not. For tooling the explicit tag is
much easier to work with.

> - Regarding Heap::collectGarbage triggered in V8's GC, it would be clear that
> it's a forced Heap::collectGarbage because the trace result appears in the
trace
> result of V8's GC.

Assuming we have collected V8 traces. I'd rather not.

> - It wouldn't be that helpful to add ForcedGC to Heap::collectAllGarbage,
since
> Heap::collectAllGarbage is only used in testing. If you want to use the
ForcedGC
> flag in testing as well, you'll need to add ForcedGC to all other
> Heap::collectGarbages used in unittests.

It is used in performance tests where we want to collect info about our GCs. For
other tests we don't care so I did not bother adding it. If we end up caring, we
can make the "default" ForcedGC and add NormalGC to the once place we actually
do a normal one.

[zerny-chromium, 2014-09-11 17:53:36]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-09-11 17:54:40]

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

[commit-bot: I haz the power, 2014-09-11 19:48:06]

Committed patchset #1 (id:1) as 181843

[haraken, 2014-09-12 00:53:09]

On 2014/09/11 17:52:47, zerny-chromium wrote:
> Thanks for the review.
> 
> > Do you really need to distinguish NormalGC from ForcedGC in your profiling?
> 
> No, it is somewhat clear from the GC pattern, but this data can be used to
> output other summarized info which would need to know which GCs are forced
(and
> therefore not of interest) and which are not. For tooling the explicit tag is
> much easier to work with.
> 
> > - Regarding Heap::collectGarbage triggered in V8's GC, it would be clear
that
> > it's a forced Heap::collectGarbage because the trace result appears in the
> trace
> > result of V8's GC.
> 
> Assuming we have collected V8 traces. I'd rather not.
> 
> > - It wouldn't be that helpful to add ForcedGC to Heap::collectAllGarbage,
> since
> > Heap::collectAllGarbage is only used in testing. If you want to use the
> ForcedGC
> > flag in testing as well, you'll need to add ForcedGC to all other
> > Heap::collectGarbages used in unittests.
> 
> It is used in performance tests where we want to collect info about our GCs.
For
> other tests we don't care so I did not bother adding it. If we end up caring,
we
> can make the "default" ForcedGC and add NormalGC to the once place we actually
> do a normal one.

I guess what you want to do would be to mark "a Heap::collectGarbage() that is
called while ThreadState::m_forcePreciseGCForTesting is true" as a ForcedGC. If
that is the case, I guess we don't need a new enum to distinguish a ForcedGC
from a NormalGC.

My point is just that I want to keep a commonly-used API (i.e.,
Heap::collectGarbage) as clean as possible.

[zerny-chromium, 2014-09-12 05:12:02]

On 2014/09/12 00:53:09, haraken wrote:
> On 2014/09/11 17:52:47, zerny-chromium wrote:
> > Thanks for the review.
> > 
> > > Do you really need to distinguish NormalGC from ForcedGC in your
profiling?
> > 
> > No, it is somewhat clear from the GC pattern, but this data can be used to
> > output other summarized info which would need to know which GCs are forced
> (and
> > therefore not of interest) and which are not. For tooling the explicit tag
is
> > much easier to work with.
> > 
> > > - Regarding Heap::collectGarbage triggered in V8's GC, it would be clear
> that
> > > it's a forced Heap::collectGarbage because the trace result appears in the
> > trace
> > > result of V8's GC.
> > 
> > Assuming we have collected V8 traces. I'd rather not.
> > 
> > > - It wouldn't be that helpful to add ForcedGC to Heap::collectAllGarbage,
> > since
> > > Heap::collectAllGarbage is only used in testing. If you want to use the
> > ForcedGC
> > > flag in testing as well, you'll need to add ForcedGC to all other
> > > Heap::collectGarbages used in unittests.
> > 
> > It is used in performance tests where we want to collect info about our GCs.
> For
> > other tests we don't care so I did not bother adding it. If we end up
caring,
> we
> > can make the "default" ForcedGC and add NormalGC to the once place we
actually
> > do a normal one.
> 
> I guess what you want to do would be to mark "a Heap::collectGarbage() that is
> called while ThreadState::m_forcePreciseGCForTesting is true" as a ForcedGC.
If
> that is the case, I guess we don't need a new enum to distinguish a ForcedGC
> from a NormalGC.

m_forcePreciseGCForTesting is not true when collectGarbage is called from the
collectAllGarbage routine that is forced by that flag.

> My point is just that I want to keep a commonly-used API (i.e.,
> Heap::collectGarbage) as clean as possible.

I agree.