[webkit-reviews] review denied: [Bug 109884] Add CSS Property tracking to FeatureObserver. Creates new histogram for CSS Property usage data. : [Attachment 194985] Update Changelog to state removal of Tab's old method

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Kassy Coan <kassycoan at gmail.com>'s
request for review:
Bug 109884: Add CSS Property tracking to FeatureObserver. Creates new histogram
for CSS Property usage data.
https://bugs.webkit.org/show_bug.cgi?id=109884

Attachment 194985: Update Changelog to state removal of Tab's old method
https://bugs.webkit.org/attachment.cgi?id=194985&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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.