Bug 109884 – Add CSS Property tracking to FeatureObserver. Creates new histogram for CSS Property usage data.

• Component: New Bugs
• Status: UNCONFIRMED

[Kassy Coan]

Add CSS Property tracking to FeatureObserver. Creates new histogram for CSS Property usage data.

[Kassy Coan]

Created attachment 188471 [details]
Patch

[Kassy Coan]

@abarth This patch just aims to add CSS tracking ability to FeatureObserver, not yet addressing the improved flushing of the histogram. CSS Features are tracked in a new histogram.

[Build Bot]

Comment on attachment 188471 [details]
Patch

Attachment 188471 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16542955

New failing tests:
inspector/extensions/extensions-resources.html

[Adam Barth]

Comment on attachment 188471 [details]
Patch

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

> Source/WebCore/css/CSSParserMode.h:76
> +    Document* m_document;

Is there a risk that m_document could be deallocated while the CSSParserContext is alive?

[Kassy Coan]

After researching a lot, I am still not sure. Adding anttik, kling for their opinions.

[Tony Chang]

Comment on attachment 188471 [details]
Patch

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

This looks much better than what we currently have in CSSParser.cpp (search for histogram).  We should delete the code in CSSParser.cpp to save memory and CPU.

This looks like it has the same problem as the existing code in that it counts the user agent style sheet values, making it hard to tell the actual usage of properties in the user agent style sheet.

> Source/WebCore/page/FeatureObserver.h:126
> +            m_CSSFeatureBits = adoptPtr(new BitVector(numCSSProperties));

Why do we dynamically allocate?  It seems like all pages are going to allocate this unless there's no CSS on the page.

[Kassy Coan]

Created attachment 190371 [details]
Patch

[Kassy Coan]

This patch still has the Document* as a member variable.
The previous CSS tracking by tabatkins@ is removed.
It only adds one 'observe' method, as the css tracking does not utilize the other method involving the domwindow, as the dom feautres do.

[Kassy Coan]

> This looks like it has the same problem as the existing code in that it counts the user agent style sheet values, making it hard to tell the actual usage of properties in the user agent style sheet.
> 

I was under the impression that this method is not counting the user agent style sheet. Here is why:

I put a print in the updateMeasurements() method, and another in the if (m_CSSFeatureBits) portion of updateMeasurements().

Now, when visiting different pages, I can see when we attempt to update, and when there are CSS Properties to update.

I created 2 html files with basic text in the body, and each html includes a blank css file.

When navigating between these two html files, I can see in the prints that we trigger updateMeasurements() but we never enter the if (m_CSSFeatureBits), because no bits are set.

My thought process was, if we were counting the user agent style sheet, then there would be m_CSSFeatureBits set, even though the page I visited had no css.

[Build Bot]

Comment on attachment 190371 [details]
Patch

Attachment 190371 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16798205

New failing tests:
inspector/extensions/extensions-resources.html
inspector/styles/get-set-stylesheet-text.html

[Kassy Coan]

Created attachment 190882 [details]
Patch

[Kassy Coan]

m_CSSFeatureBits is now statically allocated.

[Build Bot]

Comment on attachment 190882 [details]
Patch

Attachment 190882 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16663746

New failing tests:
inspector/extensions/extensions-resources.html
inspector/styles/get-set-stylesheet-text.html

[Build Bot]

Comment on attachment 190882 [details]
Patch

Attachment 190882 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16846098

New failing tests:
inspector/extensions/extensions-resources.html
inspector/styles/get-set-stylesheet-text.html

[Tony Chang]

Comment on attachment 190882 [details]
Patch

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

I would like to understand why the default style sheet isn't being counted by this change.  What makes the parseValue functions you hooked different from cssPropertyID?

> Source/WebCore/ChangeLog:9
> +        No new tests (OOPS!).

You need to remove the OOPS! and put the reason why there are no new tests.  Alternately, would it be possible to write a unit test for this (webkit_unit_tests)?

[Tony Chang]

Also, the test failures are oddly consistent.  Maybe it's a real regression?

[Kassy Coan]

Created attachment 194778 [details]
Document as RefPtr, and CSS properties with stable tracking

[Kassy Coan]

How does this look, Adam?

This patch addresses the storage of document on CSSParserContext making it now a RefPtr, and makes the tracking of CSSProperties stable, since the property ids can change over time. The only thing not addressed is that we are still tracking the user agent style sheet. I believe addressing the user agent style sheet should happen in a separate patch.

[Kassy Coan]

On bug 111949 I did a lot of tests to figure out what was causing the bot failure on mac-wk2 inspector tests. The problem was the way document was being stored on CSSParserContext. It was causing a document destruction race condition as Adam proposed. Fixed by making document a refptr.

[Kassy Coan]

Need to fix the changelog. Patch coming.

[Kassy Coan]

Created attachment 194779 [details]
Corrected ChangeLog

[Ojan Vafai]

Comment on attachment 194779 [details]
Corrected ChangeLog

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

> Source/WebCore/css/CSSParser.cpp:1327
> +    FeatureObserver::observe(m_context.m_document.get(), propertyID);

Don't we crash if you use a parser context created with the two argument constructor?

> Source/WebCore/page/FeatureObserver.cpp:41
> +static int mapCSSPropertyId(int id)
> +{
> +    switch (id) {

I'm not a huge fan of adding another switch statement that we need to update every time we add/remove a CSS property. The switch statement is fine, but lets only put the properties in this switch statement that we actually care to measure. Specifically, lets just put the WebKit prefixed properties for now since those are the properties we'd like to kill if they have sufficiently low usage.

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

I don't really follow what the flushing is all about. Can you add a description of this to the ChangeLog?

> Source/WebCore/page/FeatureObserver.h:137
> +    BitVector m_CSSFeatureBits;

Should this also be OwnPtr like m_featureBits? Then you also can initialize it lazily.

[Kassy Coan]

Created attachment 194951 [details]
Changelog describing fluhses

[Kassy Coan]

(In reply to comment #22)

> > Source/WebCore/page/FeatureObserver.cpp:41
> > +static int mapCSSPropertyId(int id)
> > +{
> > +    switch (id) {
> 
> I'm not a huge fan of adding another switch statement that we need to update every time we add/remove a CSS property. The switch statement is fine, but lets only put the properties in this switch statement that we actually care to measure. Specifically, lets just put the WebKit prefixed properties for now since those are the properties we'd like to kill if they have sufficiently low usage.

I believe we do care to measure all the properties. We would like to compare the usage of the prefixed versions to the non prefixed versions.

> 
> > Source/WebCore/page/FeatureObserver.cpp:529
> > +    if (flushCSSResults)
> > +        HistogramSupport::histogramEnumeration("WebCore.FeatureObserver.CSSProperties", cssFlushPropertyId(), maximumCSSPropertyId());
> 
> I don't really follow what the flushing is all about. Can you add a description of this to the ChangeLog?

Done.

> 
> > Source/WebCore/page/FeatureObserver.h:137
> > +    BitVector m_CSSFeatureBits;
> 
> Should this also be OwnPtr like m_featureBits? Then you also can initialize it lazily.

This was previously an OwnPtr and dynamically allocated, but changed as per request of Tony Chang.

[Kassy Coan]

Created attachment 194957 [details]
Protects against document null before calling observe

[Kassy Coan]

(In reply to comment #22)
> (From update of attachment 194779 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194779&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1327
> > +    FeatureObserver::observe(m_context.m_document.get(), propertyID);
> 
> Don't we crash if you use a parser context created with the two argument constructor?

New patch addresses this.

[Kassy Coan]

(In reply to comment #26)
> (In reply to comment #22)
> > (From update of attachment 194779 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=194779&action=review
> > 
> > > Source/WebCore/css/CSSParser.cpp:1327
> > > +    FeatureObserver::observe(m_context.m_document.get(), propertyID);
> > 
> > Don't we crash if you use a parser context created with the two argument constructor?
> 
> New patch addresses this.

My understanding is RefPtr.get() returns 0 if the parser context m_document is 0, and there is a check in FeatureObserver::observe() to early return if the given document is 0.  So, I don't believe we can crash.

If this is correct, perhaps we should code for the common case (the document exists) instead of doing a if(m_context.m_document) check every time before we call FeatureObserver::observe()?

[Mike Lawther]

(In reply to comment #22)
> I'm not a huge fan of adding another switch statement that we need to update every time we add/remove a CSS property. The switch statement is fine, but lets only put the properties in this switch statement that we actually care to measure. Specifically, lets just put the WebKit prefixed properties for now since those are the properties we'd like to kill if they have sufficiently low usage.

Yeah, we went back and forth on this one.  We do want to track all properties, not just prefixed ones.

The alternative we considered was to map everything post-facto on the dashboard side, involving using the existing CSSProperty enum values, knowing what version of WebKit the features were from, checking that out from source control and reconciling the enum values with property names etc. It sounded like a good idea at the time, but when we looked at implementing it For Reals, it was becoming pretty brittle.

We discussed this with abarth, and he said that doing dashboard side might be easier (it turned out not to be :( ), and that he was OK with the WebKit-side mapping approach if we thought it best. The UMA team were also pretty set on having a stable ID come through the pipeline (ie map WebKit side).

I can't think of a better way to automatically get tracking of all CSS properties. It's a huge boring 1:1 mapping at the moment, but is pretty future proof. What do you reckon?

[Kassy Coan]

Created attachment 194985 [details]
Update Changelog to state removal of Tab's old method

[Ojan Vafai]

I'm fine with doing the manual mapping.

Why do we want to track all CSS properties? For example, what do we gain by tracking "margin"? We already know that margin is basically used on every page on the internet because it's used in the UA stylesheet.

As I see it, the point of tracking these things is to figure out what APIs we can remove. Is there another benefit to tracking CSS property usage?

[Kassy Coan]

(In reply to comment #30)
> I'm fine with doing the manual mapping.
> 
> Why do we want to track all CSS properties? For example, what do we gain by tracking "margin"? We already know that margin is basically used on every page on the internet because it's used in the UA stylesheet.

Eventually, we are going to stop tracking the UA style sheet. That will be addressed in a separate patch.

> 
> As I see it, the point of tracking these things is to figure out what APIs we can remove. Is there another benefit to tracking CSS property usage?

1. It can help developers decide what they should and shouldn't be using in their own sites and
2. help developers discover new features

Our goal is to be able to see all the trends overtime. We should track them all so that we know when there is a decrease is usage. There is no way we can know now if property <anything> will experience a decrease in usage in the future, and the only way we will know with certainty that there was a decrease is to have the present usage data.

[Antti Koivisto]

Comment on attachment 194985 [details]
Update Changelog to state removal of Tab's old method

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

The feature seems very low value for all the bloat it introduces.

> Source/WebCore/css/CSSParser.cpp:1827
>  bool CSSParser::parseValue(CSSPropertyID propId, bool important)
>  {
> +    if (m_context.m_document)
> +        FeatureObserver::observe(m_context.m_document.get(), propId);

parseValue functions are hot. Have you measured what this does to our performance?

[Kassy Coan]

(In reply to comment #32)
> (From update of attachment 194985 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194985&action=review
> 
> The feature seems very low value for all the bloat it introduces.
> 
> > Source/WebCore/css/CSSParser.cpp:1827
> >  bool CSSParser::parseValue(CSSPropertyID propId, bool important)
> >  {
> > +    if (m_context.m_document)
> > +        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.

[Kassy Coan]

(In reply to comment #32)
> parseValue functions are hot. Have you measured what this does to our performance?

I did additional performance testing over my function call paths and they show at worst case my function took 1 ms longer than Tab's implementation loading the HTML5 spec.

We should watch the perf bots to verify the performance on landing.

[Mike Lawther]

(In reply to comment #32)
> The feature seems very low value for all the bloat it introduces.

Hi Antti! I think it's very valuable to be tracking *at minimum* properties considered experimental (ie prefixed ones). Is the low value / bloat tradeoff you see due to currently tracking all properties?

Can we agree that tracking prefixed features (and their current/future unprefixed counterparts) as a minimum is worth having a mechanism in place? 

I would argue that in the future, any new CSS properties added should also be tracked as they are untested in The Real World and tracking their usage will be interesting. So setting things up so that an author is required to add them to the tracking makes sense. The switch statement added in this patch makes this a compile time check. 

Kassy's performance numbers show that the execution time for tracking *all* properties is in the noise.

> As I see it, the point of tracking these things is to figure out what APIs we can remove. Is there another benefit to tracking CSS property usage?

Removing is a black/white type decision. There are levels of gray that are useful as well :) It can also help inform any optimisation/tradeoffs we might make. eg if we want to try and do some gnarly optimisation to border-radius drawing, it'd be helpful to have an idea of how many pages/users would benefit. We've got data structures for 'rare' styles, but these aren't based on hard data from the field as far as I know. Tracking changes of properties over time can tell if we've got the makeup of these data structures right. Yeah, there are some obvious ones that are used everywhere - do you reckon we should make an exception for them?

[Ojan Vafai]

Comment on attachment 194985 [details]
Update Changelog to state removal of Tab's old method

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.

[Kassy Coan]

(In reply to comment #36)

> > 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.

Correct, PageVisits and CSSFlush would be redundant if both are tracked correctly.
PageVisits is not being tracked correctly and I filed a bug about it: https://bugs.webkit.org/show_bug.cgi?id=113461

[Eric Seidel (no email)]

Comment on attachment 194985 [details]
Update Changelog to state removal of Tab's old method

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

I think this is an *awesome* idea.  I think we just have to be careful of perf when doing this.

> Source/WebCore/page/FeatureObserver.cpp:40
> +static int mapCSSPropertyId(int id)
> +{

I recommend autogenerating this and checking it in.  That will avoid having to type all this out, and yet still make it possible to guarantee that new ones are always added to the end.

> Source/WebCore/page/FeatureObserver.cpp:487
> +    default:
> +        ASSERT_NOT_REACHED();
> +        return 0;

Better to remove the deafult: and have it be a compiler error.

[Mike Lawther]

(In reply to comment #38)
> > Source/WebCore/page/FeatureObserver.cpp:40
> > +static int mapCSSPropertyId(int id)
> > +{
> 
> I recommend autogenerating this and checking it in.  That will avoid having to type all this out, and yet still make it possible to guarantee that new ones are always added to the end.

Just to clear - you mean check the generator script in and do the generation during the build process?

[Ojan Vafai]

Talked to Eric about this and here's the idea:
Write a script that reads in CSSPropertyNames.in and a new CSSPropertyNamesFeatures.h (or something like that). The script  overwrites the CSSPropertyNamesFeatures.h file, which is a file that we actually check in to the repo. The script only modifies CSSPropertyNamesFeatures.h to append new properties added to CSSPropertyNames.in. That way the numbers are stable.

[Mike Lawther]

(In reply to comment #40)
> Talked to Eric about this and here's the idea [...]

Awesome. Would it be OK to land this now as a starting point and file that as a followup bug?