Bug 81431 – [chromium] Allow us to disable overdraw metrics when tracing is off

• Component: New Bugs
• Status: RESOLVED FIXED

[Dana Jansens]

[chromium] Throttle overdraw metrics

[Dana Jansens]

Created attachment 132421 [details]
Patch

[Dana Jansens]

Created attachment 132423 [details]
Patch

- Need to override the overdraw metrics for the TiledLayerChromiumTest's TestCCOcclusionTracker

[James Robinson]

What is the perf delta from this?

[James Robinson]

Comment on attachment 132423 [details]
Patch

This seems like crazy overkill.  There are only two callers to overdrawMetrics - TiledLayerChromium and CCQuadCuller.  Just keep a counter in CCQuadCuller as a static local and for TiledLayerChromium stash a frame # somewhere and key off of that.  This should not be a 30kb+ patch that touches tons of files and has a whole pile of new tests.

[Dana Jansens]

ok! (touching all the tests was avoiding holding pointers to things that were freed, there's just 1 new test)

[Dana Jansens]

- Make the decision in CCLTH(Impl) to record the frame or not (since it also uses the metrics class).
- I can test that metrics do/don't record when you tell occlusion tracker what you like. But not sure how to test that the LTH is recording metrics at the desired rate in a sane way.

[Dana Jansens]

Created attachment 132436 [details]
Patch

[Dana Jansens]

Perf delta seems to be somewhere between 0.05ms and 0.1ms per frame out of a 16-30ms frame. So about 0.3% of a frame.

[Dana Jansens]

Is 0.3% of the frame on each thread worth having this code? Or should we just record every frame?

[Nat Duca]

(In reply to comment #9)
> Is 0.3% of the frame on each thread worth having this code? Or should we just record every frame?

What about recording every frame, but conditional to tracing being on? @jbates can help you make some code conditional on tracing being on.

The idea of having histograms on this is useful but thats only useful when someone posts a patch to src/tools/histograms/histograms.xml and pings jar@ to get them activated. We should decide if we want to get from-the-field data about overdraw --- if we do, we should turn on the histogram briefly for a canary or two, then turn it off again and revert to the "only on when tracing is on."

[Dana Jansens]

Created attachment 132859 [details]
Patch

This stops throttling, and record every frame. But still puts code in place to easily disable overdraw metrics when about:tracing is off in the future.

[Adrienne Walker]

Comment on attachment 132859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132859&action=review

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:279
> +    bool recordMetricsForFrame = true; // FIXME: In the future, disable this when about:tracing is off.

Can the future be now? Is it hard to resolve this FIXME?

[Dana Jansens]

Comment on attachment 132859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132859&action=review

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:279
>> +    bool recordMetricsForFrame = true; // FIXME: In the future, disable this when about:tracing is off.
> 
> Can the future be now? Is it hard to resolve this FIXME?

We don't want it now. I think we would like to collect some histograms in the wild for a while before we flip this off.

There is also no interface AIUI to check if tracing is on, but I think that won't be too bad.

[Adrienne Walker]

(In reply to comment #13)
> (From update of attachment 132859 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132859&action=review
> 
> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:279
> >> +    bool recordMetricsForFrame = true; // FIXME: In the future, disable this when about:tracing is off.
> > 
> > Can the future be now? Is it hard to resolve this FIXME?
> 
> We don't want it now. I think we would like to collect some histograms in the wild for a while before we flip this off.
> 
> There is also no interface AIUI to check if tracing is on, but I think that won't be too bad.

Maybe I misunderstood.  It sounded like Nat was saying there was some work to get histograms enabled, so I was thinking you would only turn on overdraw metrics when tracing was on for now and would force the flag to always on once the histograms were enabled.

[Dana Jansens]

Comment on attachment 132859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132859&action=review

>>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:279
>>>> +    bool recordMetricsForFrame = true; // FIXME: In the future, disable this when about:tracing is off.
>>> 
>>> Can the future be now? Is it hard to resolve this FIXME?
>> 
>> We don't want it now. I think we would like to collect some histograms in the wild for a while before we flip this off.
>> 
>> There is also no interface AIUI to check if tracing is on, but I think that won't be too bad.
> 
> Maybe I misunderstood.  It sounded like Nat was saying there was some work to get histograms enabled, so I was thinking you would only turn on overdraw metrics when tracing was on for now and would force the flag to always on once the histograms were enabled.

There is some work to do server side to store the histograms that we can do at any point, and a flag to flip in chromium. But otherwise, they are good to go.

[Adrienne Walker]

Comment on attachment 132859 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=132859&action=review

Ok, thanks for the explanation.

> Source/WebCore/platform/graphics/chromium/cc/CCOverdrawMetrics.h:53
> +    CCOverdrawMetrics(bool recordMetricsForFrame);

nit: explicit.

[WebKit Review Bot]

Comment on attachment 132859 [details]
Patch

Attachment 132859 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12002434

[Dana Jansens]

Created attachment 132882 [details]
Patch

Woops, thanks. Also, rebased.

[WebKit Review Bot]

Comment on attachment 132882 [details]
Patch

Clearing flags on attachment: 132882

Committed r111441: <http://trac.webkit.org/changeset/111441>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.