Bug 93793 – Improve the use of strings in CSS

• Status: RESOLVED
• Resolution: WONTFIX
• Component: CSS

[Benjamin Poulain]

Many CSS text accessors still have awful memory access pattern. It is hurting us on system with small CPU cache and slow memory, we should improve that.

[Benjamin Poulain]

Created attachment 158025 [details]
Patch

[Benjamin Poulain]

Created attachment 158461 [details]
Patch

[Benjamin Poulain]

Created attachment 158466 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158466 [details]
Patch

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

New failing tests:
fast/css/variables/variable-chain.html
fast/backgrounds/repeat/parsing-background-repeat.html
fast/css/variables/invalid-font-reference.html
fast/css/remove-shorthand.html
fast/css/variables/use-before-defined.html

[WebKit Review Bot]

Created attachment 158477 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Benjamin Poulain]

Created attachment 158490 [details]
Patch

[Benjamin Poulain]

Comment on attachment 158490 [details]
Patch

Looks like I have addressed the two mistakes, so let's put this version for review.

Something to be careful when reviewing:
stringBuilder::toString() return String::empty(). Previously, some functions would have returned a null string instead of an empty string.

I think I got them covered but I may have missed something so a careful review would be appreciated.

[Perf EWS]

Created attachment 158506 [details]
Performance tests results for 158461

[Rafael Brandao]

Adding people that worked/reviewed similar stuff. From bug #81214, I recall some other optimizations like creating static strings for things like "-webkit-canvas(".

ap once wondered about whether concatenating strings would be a good idea. I don't know what was concluded from there, but I think we should get rid of +/+= operator there.

[Benjamin Poulain]

(In reply to comment #9)
> Adding people that worked/reviewed similar stuff. From bug #81214, I recall some other optimizations like creating static strings for things like "-webkit-canvas(".

What has been done in 81214 is a bad idea.
We should update the code changed by 81214 with StringBuilder::appendLiteral once this lands.

> ap once wondered about whether concatenating strings would be a good idea. I don't know what was concluded from there, but I think we should get rid of +/+= operator there.

There are use cases for StringOperators.
I do not agree at all we should get rid of operator+, it generally makes the code more readable and is efficient. If I have time, I have ideas to make it even faster.

[Benjamin Poulain]

Comment on attachment 158490 [details]
Patch

I take this down for now. It looks like we can get even faster than this by changing StringOperators.

[Benjamin Poulain]

I don't have time to get numbers for every single method. Let's kill this.