Summary: | Track -webkit property usage. | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tab Atkins <tabatkins> | ||||||||||||||||||
Component: | CSS | Assignee: | 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
Tab Atkins
2012-08-07 18:17:58 PDT
Created attachment 157070 [details]
Patch
First draft of an attempt to track all -webkit property usage across the web.
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.
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. Created attachment 157077 [details]
Patch
Comment on attachment 157077 [details]
Patch
You'll need a ChangeLog, and to fix the style issues.
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... (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! Created attachment 159206 [details]
Patch
First draft of an attempt to track all -webkit property usage across the web.
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.
Created attachment 159208 [details]
Patch
Ugh, move the include again so that it sorts correctly.
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 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 Created attachment 159215 [details]
Patch
Address jamesr's comments.
(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. Created attachment 159216 [details]
Patch
Avoid histogramming invalid properties.
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. 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 on attachment 159216 [details] Patch Attachment 159216 [details] did not pass cr-android-ews (chromium-android): Output: http://queues.webkit.org/results/13531170 (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. (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? (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 (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. (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. 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. Created attachment 159223 [details]
Patch
Switch to COMPILE_ASSERT(), use a signed int to match with numCSSProperties.
(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 on attachment 159223 [details] Patch Attachment 159223 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13535002 Comment on attachment 159223 [details] Patch Attachment 159223 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13531181 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 on attachment 159223 [details] Patch Attachment 159223 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13535008 Comment on attachment 159223 [details] Patch Attachment 159223 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/13517920 Comment on attachment 159223 [details] Patch Attachment 159223 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13527244 Comment on attachment 159223 [details] Patch Attachment 159223 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13521606 Created attachment 159499 [details]
Patch
Fix compiler errors. That'll teach me to try and submit without compiling first. :/
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. 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?" (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 on attachment 159499 [details] Patch Clearing flags on attachment: 159499 Committed r126201: <http://trac.webkit.org/changeset/126201> All reviewed patches have been landed. Closing bug. As the histogram results are just the propertyIDs, could someone publish a basic lookup table so reading the CSS.PrefixUsage is easier? 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. |