Bug 78114 – Add a perf test for the CSS parser.

• Component: CSS
• Status: RESOLVED FIXED

[Alexis Menard (darktears)]

Add a perf test for the CSS parser.

[Alexis Menard (darktears)]

Created attachment 126083 [details]
Patch

[WebKit Review Bot]

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.

[Tony Chang]

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.

[Ryosuke Niwa]

YUI appears to be BSD license: http://yuilibrary.com/license/

[Alexis Menard (darktears)]

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

[Alexis Menard (darktears)]

Created attachment 126747 [details]
Patch

[Tony Chang]

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?

[Alexis Menard (darktears)]

(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?

[Ryosuke Niwa]

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

[Alexis Menard (darktears)]

Created attachment 126811 [details]
Patch

[Alexis Menard (darktears)]

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

[Ryosuke Niwa]

Apparently slashdot open-sources some of its code. e.g.:
http://slashcode.git.sourceforge.net/git/gitweb.cgi?p=slashcode/slashcode;a=tree;f=themes/slashcode/htdocs;h=dedbc999441cb971b9092fbcaeb588aaa45a0289;hb=live

[Alexis Menard (darktears)]

(In reply to comment #12)
> Apparently slashdot open-sources some of its code. e.g.:
> http://slashcode.git.sourceforge.net/git/gitweb.cgi?p=slashcode/slashcode;a=tree;f=themes/slashcode/htdocs;h=dedbc999441cb971b9092fbcaeb588aaa45a0289;hb=live

But http://slashcode.git.sourceforge.net/git/gitweb.cgi?p=slashcode/slashcode;a=blob;f=themes/slashcode/htdocs/index.pl;h=fdc7527f03a936175e1e0c9a0c08b8de65aa6470;hb=live seems to say it is GPL :(.

[Ryosuke Niwa]

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?

[Alexis Menard (darktears)]

Created attachment 127221 [details]
Patch

[Alexis Menard (darktears)]

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

[Ryosuke Niwa]

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

[Alexis Menard (darktears)]

(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

[Andreas Kling]

(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 :)

[Alexis Menard (darktears)]

Created attachment 127349 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127349 [details]
Patch

Clearing flags on attachment: 127349

Committed r107964: <http://trac.webkit.org/changeset/107964>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.