Bug 93420 – Track -webkit property usage.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Tab Atkins]

Track -webkit property usage.

[Tab Atkins]

Created attachment 157070 [details]
Patch

First draft of an attempt to track all -webkit property usage across the web.

[WebKit Review Bot]

Attachment 157070 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/css/CSSParser.cpp']" exit_code: 1
Source/WebCore/css/CSSParser.cpp:63:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/css/CSSParser.cpp:10041:  Missing space before ( in if(  [whitespace/parens] [5]
Source/WebCore/css/CSSParser.cpp:10043:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 3 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tab Atkins]

This patch is almost certainly going to need to be rewritten.  I don't actually know what I'm doing.

The goal here is to track usage of -webkit properties across the web, so we can see what can be removed from the public side of the platform.

Right now I'm doing this in the simplest possible way, by just histogramming every single use of all prefixed properties.  This should likely be improved to instead log only one entry per property per page, but that's a lot more complicated.  This first try will help us out with some of the obvious properties.

[Tab Atkins]

Created attachment 157077 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 157077 [details]
Patch

You'll need a ChangeLog, and to fix the style issues.

[Eric Seidel (no email)]

Maybe Luke or one of the other style-interested folks at Google could help you get this landed.  WebKit can be a bit quirky to get started in...

[Tab Atkins]

(In reply to comment #5)
> (From update of attachment 157077 [details])
> You'll need a ChangeLog, and to fix the style issues.

Style issues are fixed - I accidentally did the initial commit with some unsaved changes.  The second patch fixes them.

Forgot about ChangeLog, ugh.  I'll get one up on Friday, when I return to my desk.

Thanks, eseidel!

[Tab Atkins]

Created attachment 159206 [details]
Patch

First draft of an attempt to track all -webkit property usage across the web.

[WebKit Review Bot]

Attachment 159206 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/css/CSSParser.cpp:64:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Tab Atkins]

Created attachment 159208 [details]
Patch

Ugh, move the include again so that it sorts correctly.

[James Robinson]

Comment on attachment 159208 [details]
Patch

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

I think this looks pretty reasonable, but I'd like someone familiar with CSSParser to sanity check it.

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

There's a check that will barf on this.  This sort of change isn't directly testable by our unit tests (it should have no observable side-effects other than histogramming. You could describe how to manually test this in chromium, or just delete this line and explain why this doesn't need tests.

> Source/WebCore/css/CSSParser.cpp:10123
> +    if (buffer[0] == '-')

should we check for -webkit? looks like hasPrefix() could help with that

> Source/WebCore/css/CSSParser.cpp:10124
> +        HistogramSupport::histogramEnumeration("CSS.PrefixUsage", max(1, propertyID - firstCSSProperty), 600);

where does the number 600 come from?  It looks like we have 378 property IDs (although I imagine we want some room for growth).  Maybe pick a named constant with a comment indicating that it has to be >= numCSSProperties?

[Elliott Sprehn]

Comment on attachment 159208 [details]
Patch

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

>> Source/WebCore/css/CSSParser.cpp:10123
>> +    if (buffer[0] == '-')
> 
> should we check for -webkit? looks like hasPrefix() could help with that

This is going to match -moz, -ms etc. You need hasPrefix(buffer, length, "-webkit-") && propertyID != CSSPropertyInvalid

[Tab Atkins]

Created attachment 159215 [details]
Patch

Address jamesr's comments.

[Tab Atkins]

(In reply to comment #11)
> (From update of attachment 159208 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159208&action=review
> 
> I think this looks pretty reasonable, but I'd like someone familiar with CSSParser to sanity check it.
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests (OOPS!).
> 
> There's a check that will barf on this.  This sort of change isn't directly testable by our unit tests (it should have no observable side-effects other than histogramming. You could describe how to manually test this in chromium, or just delete this line and explain why this doesn't need tests.

Done.

> > Source/WebCore/css/CSSParser.cpp:10123
> > +    if (buffer[0] == '-')
> 
> should we check for -webkit? looks like hasPrefix() could help with that

I was under the mistaken impression that other prefixes would have been filtered out by now.  Switched to hasPrefix().

> > Source/WebCore/css/CSSParser.cpp:10124
> > +        HistogramSupport::histogramEnumeration("CSS.PrefixUsage", max(1, propertyID - firstCSSProperty), 600);
> 
> where does the number 600 come from?  It looks like we have 378 property IDs (although I imagine we want some room for growth).  Maybe pick a named constant with a comment indicating that it has to be >= numCSSProperties?

Done.  Also added an assert in case this sits around long enough for numCSSProperties to ever exceed it.

[Tab Atkins]

Created attachment 159216 [details]
Patch

Avoid histogramming invalid properties.

[Ojan Vafai]

Comment on attachment 159216 [details]
Patch

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

Seems fine to me. This of course will only catch uses in stylesheets. It won't catch prefixed properties being modified from JS (i.e. via node.style.WebKit*). That's fine for an initial pass, but probably isn't enough to confidently start killing prefixed property names.

Also, all the prefixed properties in the useragent stylesheet (html.css) will hit on every page.

I'm actually not sure how you'd go about restricting to one entry per document. You'd need to keep a hashtable of which entries we've seen so far, and I don't think we'd want to do that. Hm...I suppose you could just keep bitset of bool since there's ~400 ids. Not sure this even warrants 50 bytes per document though. Either way, that can be a follow-up decision once we have an initial dataset.

> Source/WebCore/css/CSSParser.cpp:10127
> +    if (hasPrefix(buffer, length, "-webkit-"))

We should watch the perf bots after this lands to be sure this doesn't affect performance.

[Elliott Sprehn]

I'm not sure how histograms work in Chrome, but if someone adds a new CSS property all the enum values are going to shift. That would skew your results because -webkit-border-radius could be two different values in different releases.

Are these aggregated by version and then converted back into strings?

[Peter Beverloo (cr-android ews)]

Comment on attachment 159216 [details]
Patch

Attachment 159216 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13531170

[Tab Atkins]

(In reply to comment #16)
> (From update of attachment 159216 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159216&action=review
> 
> Seems fine to me. This of course will only catch uses in stylesheets. It won't catch prefixed properties being modified from JS (i.e. via node.style.WebKit*). That's fine for an initial pass, but probably isn't enough to confidently start killing prefixed property names.

Hm, Luke thought that this function would intercept those calls as well.  Are you sure that this misses el.style changes?  If so, where should I instrument to catch those?

> Also, all the prefixed properties in the useragent stylesheet (html.css) will hit on every page.

Good point.  We'll have to keep that in mind when reviewing the results.

> I'm actually not sure how you'd go about restricting to one entry per document. You'd need to keep a hashtable of which entries we've seen so far, and I don't think we'd want to do that. Hm...I suppose you could just keep bitset of bool since there's ~400 ids. Not sure this even warrants 50 bytes per document though. Either way, that can be a follow-up decision once we have an initial dataset.

Yes, a bool bitset is the way to go, when we decide to improve this.  As you say, we can defer worrying about this until later.

> > Source/WebCore/css/CSSParser.cpp:10127
> > +    if (hasPrefix(buffer, length, "-webkit-"))
> 
> We should watch the perf bots after this lands to be sure this doesn't affect performance.

[Tab Atkins]

(In reply to comment #18)
> (From update of attachment 159216 [details])
> Attachment 159216 [details] did not pass cr-android-ews (chromium-android):
> Output: http://queues.webkit.org/results/13531170

I assume this failure is due to the "warning treated as error" where the assert compares a signed and unsigned int?

[James Robinson]

(In reply to comment #20)
> (In reply to comment #18)
> > (From update of attachment 159216 [details] [details])
> > Attachment 159216 [details] [details] did not pass cr-android-ews (chromium-android):
> > Output: http://queues.webkit.org/results/13531170
> 
> I assume this failure is due to the "warning treated as error" where the assert compares a signed and unsigned int?

Yes.  You probably want this to be a COMPILE_ASSERT() anyway

[Tab Atkins]

(In reply to comment #17)
> I'm not sure how histograms work in Chrome, but if someone adds a new CSS property all the enum values are going to shift. That would skew your results because -webkit-border-radius could be two different values in different releases.
> 
> Are these aggregated by version and then converted back into strings?

Ugh, I didn't think of that.  I see two ways around it:

1. Add version info to the histogram name, so we can aggregate and convert back to strings.
2. Use a big switch statement (and keep it up-to-date) to convert prefixed names to stable integers.

Unfortunately, histograms can't be used to capture strings.

[Ojan Vafai]

(In reply to comment #19)
> (In reply to comment #16)
> > (From update of attachment 159216 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=159216&action=review
> > 
> > Seems fine to me. This of course will only catch uses in stylesheets. It won't catch prefixed properties being modified from JS (i.e. via node.style.WebKit*). That's fine for an initial pass, but probably isn't enough to confidently start killing prefixed property names.
> 
> Hm, Luke thought that this function would intercept those calls as well.  Are you sure that this misses el.style changes?  If so, where should I instrument to catch those?

Oh, actually, yes, it turns out it will. I misread the code.

(In reply to comment #22)
> 1. Add version info to the histogram name, so we can aggregate and convert back to strings.
> 2. Use a big switch statement (and keep it up-to-date) to convert prefixed names to stable integers.

I'd rather not do 2. Not sure what you'd do for 1. You'd need something that would update every time someone modified the list of css property names, which is unrealistic.

Is it important to be able to compare one release to another for this? You just need to know which webkit revision a given release is synced to.

[James Robinson]

The uploaded histogram data includes the version, so we can fix up any version diffs after the fact (although it might be a bit annoying).  I wouldn't worry about it too much.

[Tab Atkins]

Created attachment 159223 [details]
Patch

Switch to COMPILE_ASSERT(), use a signed int to match with numCSSProperties.

[Tab Atkins]

(In reply to comment #24)
> The uploaded histogram data includes the version, so we can fix up any version diffs after the fact (although it might be a bit annoying).  I wouldn't worry about it too much.

Excellent, I don't need to do anything then.  We'll need to clean up the data anyway before we can actually analyze it, so that'll be fine.

Can I get another r+?

[WebKit Review Bot]

Comment on attachment 159223 [details]
Patch

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

[Build Bot]

Comment on attachment 159223 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13530197

[Build Bot]

Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13535008

[Early Warning System Bot]

Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13517920

[Early Warning System Bot]

Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13527244

[Gyuyoung Kim]

Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13521606

[Tab Atkins]

Created attachment 159499 [details]
Patch

Fix compiler errors. That'll teach me to try and submit without compiling first. :/

[WebKit Review Bot]

Comment on attachment 159499 [details]
Patch

Rejecting attachment 159499 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[James Robinson]

If you aren't a committer, you have to set cq?.  The convention is "flag?" means "can somebody with permissions on 'flag' set it to + for me please?"

[Tab Atkins]

(In reply to comment #36)
> If you aren't a committer, you have to set cq?.  The convention is "flag?" means "can somebody with permissions on 'flag' set it to + for me please?"

Yeah, I forgot that I'm just on the Contributor list. :/

[WebKit Review Bot]

Comment on attachment 159499 [details]
Patch

Clearing flags on attachment: 159499

Committed r126201: <http://trac.webkit.org/changeset/126201>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Paul Irish]

As the histogram results are just the propertyIDs, could someone publish a basic lookup table so reading the CSS.PrefixUsage is easier?

[Tony Chang]

FWIW, you can make the dashboard show the actual property names if you update an xml file.

Specifically, http://wiki/Main/ChromeUserExperienceMetrics#Histograms describes how to update the histograms.xml file.  There's probably some work required to get the id/name mapping into the xml file.