[Webkit-unassigned] [Bug 109884] Add CSS Property tracking to FeatureObserver. Creates new histogram for CSS Property usage data.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109884


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #194985|review?, commit-queue?      |review-, commit-queue-
               Flag|                            |




--- Comment #36 from Ojan Vafai <ojan at chromium.org>  2013-03-27 17:03:57 PST ---
(From update of attachment 194985)
View in context: https://bugs.webkit.org/attachment.cgi?id=194985&action=review

I guess I can see the value in tracking common properties. As an example, Eric and I were considering the other day what percentage of pages use floats in order to judge whether it was worth doing a performance optimization in RenderBlock for pages that don't contain any floats.

>>> Source/WebCore/css/CSSParser.cpp:1827
>>> +        FeatureObserver::observe(m_context.m_document.get(), propId);
>> 
>> parseValue functions are hot. Have you measured what this does to our performance?
> 
> Yes I have measured. FeatureObserver has 1 main loop that is most time consuming, and it is very minor.
> 
> Here are elapsed time over a few runs. In the worse case, it took 80 microseconds.
> 
> Elapsed Time: 0.000080 count: 105
> 
> Elapsed Time: 0.000031 count: 103
> 
> Elapsed Time: 0.000029 count: 95
> 
> Elapsed Time: 0.000031 count: 95
> 
> Elapsed Time: 0.000028 count: 94
> 
> (count meaning the number of CSS Properties used on that page)
> 
> I believe the if check here is unnecessary (see my previous comment for explanation of how the RefPtr works for my reasoning), but I added it is as per request of Ojan, and I have not heard back yet of if it's good to remove.

I didn't realize that observe null-checks document. I think you're right that you don't need it here.

> Source/WebCore/page/FeatureObserver.cpp:495
> +    m_CSSFeatureBits.ensureSize(lastCSSProperty + 1);

Is this line needed? m_featureBits doesn't seem to call this?

> Source/WebCore/page/FeatureObserver.cpp:529
> +    if (flushCSSResults)
> +        HistogramSupport::histogramEnumeration("WebCore.FeatureObserver.CSSProperties", cssFlushPropertyId(), maximumCSSPropertyId());

I now understand what the flushing is, but I don't think it's necessary. If we're tracking every CSS property, then literally every page will hit this code. As such, the flushing is redundant with the PageVisits enumeration.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.