Bug 93420

Summary: Track -webkit property usage.
Product: WebKit Reporter: Tab Atkins <tabatkins>
Component: CSSAssignee: Tab Atkins <tabatkins>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, dglazkov, eric, esprehn, gustavo, jamesr, macpherson, menard, ojan, paulirish, peter+ews, philn, rniwa, tony, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tab Atkins 2012-08-07 18:17:58 PDT
Track -webkit property usage.
Comment 1 Tab Atkins 2012-08-07 18:18:54 PDT
Created attachment 157070 [details]
Patch

First draft of an attempt to track all -webkit property usage across the web.
Comment 2 WebKit Review Bot 2012-08-07 18:21:00 PDT
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.
Comment 3 Tab Atkins 2012-08-07 18:26:05 PDT
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.
Comment 4 Tab Atkins 2012-08-07 18:29:21 PDT
Created attachment 157077 [details]
Patch
Comment 5 Eric Seidel (no email) 2012-08-11 02:49:49 PDT
Comment on attachment 157077 [details]
Patch

You'll need a ChangeLog, and to fix the style issues.
Comment 6 Eric Seidel (no email) 2012-08-11 02:50:44 PDT
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...
Comment 7 Tab Atkins 2012-08-13 12:03:03 PDT
(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!
Comment 8 Tab Atkins 2012-08-17 14:37:01 PDT
Created attachment 159206 [details]
Patch

First draft of an attempt to track all -webkit property usage across the web.
Comment 9 WebKit Review Bot 2012-08-17 14:39:49 PDT
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.
Comment 10 Tab Atkins 2012-08-17 14:44:00 PDT
Created attachment 159208 [details]
Patch

Ugh, move the include again so that it sorts correctly.
Comment 11 James Robinson 2012-08-17 14:56:48 PDT
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?
Comment 12 Elliott Sprehn 2012-08-17 15:14:16 PDT
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
Comment 13 Tab Atkins 2012-08-17 15:17:26 PDT
Created attachment 159215 [details]
Patch

Address jamesr's comments.
Comment 14 Tab Atkins 2012-08-17 15:19:27 PDT
(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.
Comment 15 Tab Atkins 2012-08-17 15:22:42 PDT
Created attachment 159216 [details]
Patch

Avoid histogramming invalid properties.
Comment 16 Ojan Vafai 2012-08-17 15:25:30 PDT
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.
Comment 17 Elliott Sprehn 2012-08-17 15:26:54 PDT
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?
Comment 18 Peter Beverloo (cr-android ews) 2012-08-17 15:28:00 PDT
Comment on attachment 159216 [details]
Patch

Attachment 159216 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13531170
Comment 19 Tab Atkins 2012-08-17 15:30:57 PDT
(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.
Comment 20 Tab Atkins 2012-08-17 15:32:51 PDT
(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?
Comment 21 James Robinson 2012-08-17 15:34:43 PDT
(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
Comment 22 Tab Atkins 2012-08-17 15:35:28 PDT
(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.
Comment 23 Ojan Vafai 2012-08-17 16:05:36 PDT
(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.
Comment 24 James Robinson 2012-08-17 16:09:20 PDT
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.
Comment 25 Tab Atkins 2012-08-17 16:15:11 PDT
Created attachment 159223 [details]
Patch

Switch to COMPILE_ASSERT(), use a signed int to match with numCSSProperties.
Comment 26 Tab Atkins 2012-08-17 16:16:44 PDT
(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+?
Comment 27 WebKit Review Bot 2012-08-17 16:28:59 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13535002
Comment 28 Build Bot 2012-08-17 16:34:42 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13531181
Comment 29 Peter Beverloo (cr-android ews) 2012-08-17 16:38:30 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13530197
Comment 30 Build Bot 2012-08-17 16:49:46 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13535008
Comment 31 Early Warning System Bot 2012-08-17 17:16:04 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13517920
Comment 32 Early Warning System Bot 2012-08-17 17:45:47 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13527244
Comment 33 Gyuyoung Kim 2012-08-17 19:18:15 PDT
Comment on attachment 159223 [details]
Patch

Attachment 159223 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13521606
Comment 34 Tab Atkins 2012-08-20 13:12:34 PDT
Created attachment 159499 [details]
Patch

Fix compiler errors. That'll teach me to try and submit without compiling first. :/
Comment 35 WebKit Review Bot 2012-08-21 10:41:56 PDT
Comment on attachment 159499 [details]
Patch

Rejecting attachment 159499 [details] from commit-queue.

tabatkins@google.com 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.
Comment 36 James Robinson 2012-08-21 10:46:17 PDT
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?"
Comment 37 Tab Atkins 2012-08-21 10:47:45 PDT
(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. :/
Comment 38 WebKit Review Bot 2012-08-21 15:51:30 PDT
Comment on attachment 159499 [details]
Patch

Clearing flags on attachment: 159499

Committed r126201: <http://trac.webkit.org/changeset/126201>
Comment 39 WebKit Review Bot 2012-08-21 15:51:35 PDT
All reviewed patches have been landed.  Closing bug.
Comment 40 Paul Irish 2012-08-28 14:24:46 PDT
As the histogram results are just the propertyIDs, could someone publish a basic lookup table so reading the CSS.PrefixUsage is easier?
Comment 41 Tony Chang 2012-08-28 15:29:00 PDT
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.