Bug 111725 - Adding a hook to collect data for a Google UMA histogram to track when m_needsCompositedScrolling is turned on and off
Summary: Adding a hook to collect data for a Google UMA histogram to track when m_need...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-03-07 06:34 PST by Glenn Hartmann
Modified: 2013-03-08 08:30 PST (History)
8 users (show)

See Also:

Attachments
Patch (1.67 KB, patch)
2013-03-07 06:35 PST, Glenn Hartmann 		no flags Details | Formatted Diff | Diff
Patch (3.25 KB, patch)
2013-03-08 06:52 PST, Glenn Hartmann 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Glenn Hartmann 2013-03-07 06:34:39 PST 
Adding a histogram to track when m_needsCompositedScrolling is turned on and off
Comment 1 Glenn Hartmann 2013-03-07 06:35:14 PST 
Created attachment 191986 [details]
Patch
Comment 2 Julien Chaffraix 2013-03-07 14:06:18 PST 
Comment on attachment 191986 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2018
> +        if (acceleratedCompositingForOverflowScrollEnabled())
> +            HistogramSupport::histogramEnumeration("Renderer.NeedsCompositedScrolling", m_needsCompositedScrolling, 2);

It would have been nice to explain *what* you are trying to measure as well as *why*. Without the context, it's difficult to know if this is correct or why we can't other some other mechanism like FeatureObserver.

After talking with Glenn over IM, they want to measure the frequency of the feature at the layer level as a rough estimate of what could be done. They understand that this number will not be actionable but that giving the reasons for not having composited scrolling are expensive to compute and we want the overhead minimal.

All in all, the code change make sense but please add the relevant bits to the ChangeLog or as a comment in the code (remember that WebKit prefers 'why' comments over 'what' comments though).
Comment 3 Simon Fraser (smfr) 2013-03-07 14:16:20 PST 
Out of context, I've no idea what it means to "add a Histogram". How do I get to see the histogram?

I think what you really mean is "add some instrumentation so that Google can collect data, which might be displayed in a histogram at some point".
Comment 4 Glenn Hartmann 2013-03-08 06:52:50 PST 
Created attachment 192219 [details]
Patch
Comment 5 WebKit Review Bot 2013-03-08 08:30:41 PST 
Comment on attachment 192219 [details]
Patch

Clearing flags on attachment: 192219

Committed r145229: <http://trac.webkit.org/changeset/145229>
Comment 6 WebKit Review Bot 2013-03-08 08:30:44 PST 
All reviewed patches have been landed.  Closing bug.