Bug 78114

Summary: Add a perf test for the CSS parser.
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: CSSAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: kling, koivisto, rniwa, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alexis Menard (darktears) 2012-02-08 07:30:36 PST
Add a perf test for the CSS parser.
Comment 1 Alexis Menard (darktears) 2012-02-08 07:39:59 PST
Created attachment 126083 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-08 07:43:08 PST
Attachment 126083 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'PerformanceTests/ChangeLog', u'Performance..." exit_code: 1

Last 3072 characters of output:
r/resources/style.css:4189:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4190:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4191:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4192:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4193:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4194:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4195:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4196:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4197:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4198:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4199:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4200:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4201:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4202:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4203:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4206:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4207:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4208:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4212:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4213:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4214:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4218:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4219:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4220:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4397:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4433:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4485:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4557:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:4638:  Line contains tab character.  [whitespace/tab] [5]
PerformanceTests/Parser/resources/style.css:5401:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 34 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Tony Chang 2012-02-08 10:07:18 PST
Comment on attachment 126083 [details]
Patch

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

> PerformanceTests/Parser/resources/style.css:2
> +/* Facebook */
> +/*1328647392,176833978*/

We probably have to get permission from Facebook et al to use their styles.

> PerformanceTests/Parser/resources/style.css:2913
> +.view-error .oops-msg .icon{margin-left:13px}.view-error .oops-msg .shadow{background:url(http://l.yimg.com/a/i/ww/met/shadow_icon_32_101508.png) no-repeat;margin-left:8px;height:6px;width:34px;float:left;clear:left}

We probably also need to sanitize URLs in the css.  We don't want to hit a live page when running the perf tests.

> PerformanceTests/Parser/resources/style.css:6147
> +/* lib/yui/2.8.1/container/assets/container-core-min.css */

YUI might be OK.  You'd have to check the license.
Comment 4 Ryosuke Niwa 2012-02-08 12:09:00 PST
YUI appears to be BSD license: http://yuilibrary.com/license/
Comment 5 Alexis Menard (darktears) 2012-02-08 12:10:54 PST
(In reply to comment #4)
> YUI appears to be BSD license: http://yuilibrary.com/license/

Yeah I will update the patch on Friday with the Wikipedia CSS and the YUI CSS code, that should be enough.
Comment 6 Alexis Menard (darktears) 2012-02-13 03:51:39 PST
Created attachment 126747 [details]
Patch
Comment 7 Tony Chang 2012-02-13 10:21:45 PST
Comment on attachment 126747 [details]
Patch

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

> PerformanceTests/Parser/resources/style.css:3
> +/* Wikipedia */
> +
> +#interwiki-completelist{font-weight:bold}

Where is the license/copyright notice for the wikipedia CSS file?
Comment 8 Alexis Menard (darktears) 2012-02-13 11:34:15 PST
(In reply to comment #7)
> (From update of attachment 126747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126747&action=review
> 
> > PerformanceTests/Parser/resources/style.css:3
> > +/* Wikipedia */
> > +
> > +#interwiki-completelist{font-weight:bold}
> 
> Where is the license/copyright notice for the wikipedia CSS file?

http://www.mediawiki.org/wiki/Download says it is GPL v2. Is that acceptable?
Comment 9 Ryosuke Niwa 2012-02-13 12:16:45 PST
Comment on attachment 126747 [details]
Patch

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

>>> PerformanceTests/Parser/resources/style.css:3
>>> +#interwiki-completelist{font-weight:bold}
>> 
>> Where is the license/copyright notice for the wikipedia CSS file?
> 
> http://www.mediawiki.org/wiki/Download says it is GPL v2. Is that acceptable?

GPL v2 might be incompatible with LGPL (v2.1).
Comment 10 Alexis Menard (darktears) 2012-02-13 13:05:38 PST
Created attachment 126811 [details]
Patch
Comment 11 Alexis Menard (darktears) 2012-02-13 13:06:58 PST
(In reply to comment #10)
> Created an attachment (id=126811) [details]
> Patch

Got rid of Wikipedia.

Mozilla.org is not an option too.

Seems pretty hard to find some big CSS chunk out there that is compatible with BSD licensing.
Comment 14 Ryosuke Niwa 2012-02-15 12:03:33 PST
Comment on attachment 126811 [details]
Patch

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

> PerformanceTests/Parser/css-parser.html:8
> +    var styles = document.getElementById("styles");
> +    styles.setAttribute("href", "resources/style.css");

Should we be removing the old element before attaching new one?
Comment 15 Alexis Menard (darktears) 2012-02-15 13:03:53 PST
Created attachment 127221 [details]
Patch
Comment 16 Alexis Menard (darktears) 2012-02-15 13:04:46 PST
(In reply to comment #15)
> Created an attachment (id=127221) [details]
> Patch

Ten consecutive runs output this :

http://paste.kde.org/423104/

results seems pretty close which is good for stability.
Comment 17 Ryosuke Niwa 2012-02-15 13:37:28 PST
(In reply to comment #16)
> (In reply to comment #15)
> > Created an attachment (id=127221) [details] [details]
> > Patch
> 
> Ten consecutive runs output this :
> 
> http://paste.kde.org/423104/
> 
> results seems pretty close which is good for stability.

Yup, that looks like a solid performance test.
Comment 18 Alexis Menard (darktears) 2012-02-16 03:09:53 PST
(In reply to comment #17)
> (In reply to comment #16)
> > (In reply to comment #15)
> > > Created an attachment (id=127221) [details] [details] [details]
> > > Patch
> > 
> > Ten consecutive runs output this :
> > 
> > http://paste.kde.org/423104/
> > 
> > results seems pretty close which is good for stability.
> 
> Yup, that looks like a solid performance test.

Please bless it when you want :D
Comment 19 Andreas Kling 2012-02-16 03:21:44 PST
(In reply to comment #18)
> (In reply to comment #17)
> > (In reply to comment #16)
> > > (In reply to comment #15)
> > > > Created an attachment (id=127221) [details] [details] [details] [details]
> > > > Patch
> > > 
> > > Ten consecutive runs output this :
> > > 
> > > http://paste.kde.org/423104/
> > > 
> > > results seems pretty close which is good for stability.
> > 
> > Yup, that looks like a solid performance test.
> 
> Please bless it when you want :D

I think we should give this test a more specific name, e.g css-parser-yui. We may want to add more parser test in the future :)
Comment 20 Alexis Menard (darktears) 2012-02-16 03:31:59 PST
Created attachment 127349 [details]
Patch
Comment 21 WebKit Review Bot 2012-02-16 12:14:54 PST
Comment on attachment 127349 [details]
Patch

Clearing flags on attachment: 127349

Committed r107964: <http://trac.webkit.org/changeset/107964>
Comment 22 WebKit Review Bot 2012-02-16 12:15:02 PST
All reviewed patches have been landed.  Closing bug.