Bug 77129 – Simplify CSS property parsing.

• Component: CSS
• Status: RESOLVED INVALID

[Alexis Menard (darktears)]

Simplify CSS shorthand handling.

[Alexis Menard (darktears)]

Created attachment 124160 [details]
Patch

[WebKit Review Bot]

Attachment 124160 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/Target.pri', u'Source/WebCo..." exit_code: 1

Source/WebCore/css/CSSParserShorthands.cpp:215:  Should have only a single space after a punctuation in a comment.  [whitespace/comments] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

It's a WIP but I want to get feedback on it. It's obviously missing the build system parts of other ports but on the Qt one, I tested the patch successfully and fast/css layout tests are green.

The idea here is to have the shortand related code in one place. I like that we get rid of all the :

 const int properties[] = { CSSPropertyWebkitWrapFlow, CSSPropertyWebkitWrapMargin, CSSPropertyWebkitWrapPadding };

everywhere. This array for each shorthand is now static and shared by all elements which maybe use the shorthand and created at compile time. Each handler could also be extended to support more features (we could move and rename CSSParserShorthands.cpp and each shorthand handler could also support the calculation for getComputedStyle for example).

In the future in order to fix https://bugs.webkit.org/show_bug.cgi?id=73002 where we need to expand the longhands when the shorthand is initial or inherit, we could think about something like (on top of this patch):

https://gist.github.com/1684850

(which of course could be improved way more).

I'd like to get opinions if I should proceed more in that direction or if someone has a better idea, if people think it is simpler like this and the maintenance is easier. Also maybe someone see any major drawback with this approach.

Thanks.

[Gyuyoung Kim]

Comment on attachment 124160 [details]
Patch

Attachment 124160 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11163455

[WebKit Review Bot]

Comment on attachment 124160 [details]
Patch

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

[Gustavo Noronha (kov)]

Comment on attachment 124160 [details]
Patch

Attachment 124160 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11163490

[Tony Chang]

One thing that we lose with this approach is that it's harder to find where a CSS property is parsed (it could be in CSSParserShorthands.cpp or CSSParser.cpp).  In the no-svg case, we also get a compiler error because the switch doesn't have a default: case.

The code also looks like it would be slower than the giant switch statement since now every CSS property has to be looked up in the shorthand map, then in the giant switch statement.

[Alexis Menard (darktears)]

(In reply to comment #7)
> One thing that we lose with this approach is that it's harder to find where a CSS property is parsed (it could be in CSSParserShorthands.cpp or CSSParser.cpp).

Shorthand -> CSSParserShorthands.cpp, longhands -> CSSParser. In the long run maybe it is possible to have the same approach for all the giant switch case values making everything at the same place.

> In the no-svg case, we also get a compiler error because the switch doesn't have a default: case.

Each "case" I removed were returning directly (no break) and today if the shorthand is found it will return also without going to the big switch case. I tried to compile with --no-svg and it compiles fine.

> 
> The code also looks like it would be slower than the giant switch statement since now every CSS property has to be looked up in the shorthand map, then in the giant switch statement.

I'm not sure this is entirely true. The shorthand handler container is defined like this :

ShorthandHandler m_handlerMap[numCSSProperties];

Maybe the name is misleading (granted) but it's an array.

So for longhands the overhead would be : array access, function call (handler.isValid) and a pointer comparison (the isValid function) then we fallback to the switch case.

For the shorthand depending on how the switch case is implemented by the compiler (jump table or not) it is probably faster : array lookup, function call (isValid), pointer comparison (isValid), and then we go straight to parseXXX where we return. Before It would enter in the switch case, construct the properties array for the shorthand and call parseXXX.

[Alexis Menard (darktears)]

Created attachment 124341 [details]
Patch

[WebKit Review Bot]

Comment on attachment 124341 [details]
Patch

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

[Alexis Menard (darktears)]

Created attachment 124345 [details]
Patch

[WebKit Review Bot]

Comment on attachment 124345 [details]
Patch

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

New failing tests:
fast/exclusions/wrap-parsing.html
inspector/styles/styles-update-from-js.html

[Alexis Menard (darktears)]

Created attachment 124358 [details]
Patch

[WebKit Review Bot]

Comment on attachment 124358 [details]
Patch

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

New failing tests:
inspector/styles/styles-update-from-js.html

[Alexis Menard (darktears)]

Created attachment 124542 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 124542 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        Move the CSS shorthand handling into a separate class inspired
> +        a bit from CSSStyleApplyValue. It uses handler that each shorthand
> +        specify. This handler is responsible to parse the shorthand correctly.
> +        In the future we could extend this handler to expand the longhands
> +        and set them to initial or inherit. 

The English needs some improvement here.

Does this patch have performance impact?

[Alexis Menard (darktears)]

(In reply to comment #16)
> (From update of attachment 124542 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=124542&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        Move the CSS shorthand handling into a separate class inspired
> > +        a bit from CSSStyleApplyValue. It uses handler that each shorthand
> > +        specify. This handler is responsible to parse the shorthand correctly.
> > +        In the future we could extend this handler to expand the longhands
> > +        and set them to initial or inherit. 
> 
> The English needs some improvement here.

:D

> 
> Does this patch have performance impact?

Well I'm uploading a new version which extends this pattern to all properties. It will be online in a couple of minutes.

[Alexis Menard (darktears)]

Created attachment 125692 [details]
Patch

[Alexis Menard (darktears)]

(In reply to comment #18)
> Created an attachment (id=125692) [details]
> Patch

Just want to gather feedback before going further.

[Darin Adler]

Comment on attachment 125692 [details]
Patch

Looks more complicated to me rather than simpler. But maybe it’s just because I’m not used to it yet.

What is the performance impact of this change? Does it make the code faster? Or slower? Or neither?

[Alexis Menard (darktears)]

(In reply to comment #20)
> (From update of attachment 125692 [details])
> Looks more complicated to me rather than simpler. But maybe it’s just because I’m not used to it yet.
> 
> What is the performance impact of this change? Does it make the code faster? Or slower? Or neither?

https://gist.github.com/1761318 (green being values with the patch).

are the result on my Linux machine with the Chromium port running the perf tests.

https://gist.github.com/1761724

are the result on my Mac with the Mac port running the perf tests.

I think the results are almost the same but the tests are not necessarily covering what the patch is improving. Multiple CSSParser creation and avoiding the switch case for most common properties.

So I wrote a benchmark which I believe cover more the use case :

<!DOCTYPE html>
<body>
<script src="../resources/runner.js"></script>
<link id="styles" rel="stylesheet" type="text/css" href="resources/style.css"/>
<script>
PerfTestRunner.run(function() {
    var styles = document.getElementById("styles");
    styles.setAttribute("href", "resources/style.css");
}, 200);

</script>
</body>

loading a huge CSS file (taken from https://bugs.webkit.org/show_bug.cgi?id=70107).

I got those results :

-Without the patch : 

Running Parser/css-parser.html (1 of 1)
RESULT Parser: css-parser= 937.6 ms
median= 935.0 ms, stdev= 4.24735211632 ms, min= 934.0 ms, max= 947.0 ms

- With the patch :

Running Parser/css-parser.html (1 of 1)
RESULT Parser: css-parser= 918.15 ms
median= 917.0 ms, stdev= 3.10282129682 ms, min= 915.0 ms, max= 927.0 ms

[Alexis Menard (darktears)]

Created attachment 125903 [details]
Patch

[WebKit Review Bot]

Comment on attachment 125903 [details]
Patch

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

New failing tests:
svg/custom/getPresentationAttribute.svg

[Alexis Menard (darktears)]

(In reply to comment #20)
> (From update of attachment 125692 [details])
> Looks more complicated to me rather than simpler. But maybe it’s just because I’m not used to it yet.

The idea I had when I created this patch was to be able to do more without copy and pasting the switch case.

The patch in https://bugs.webkit.org/show_bug.cgi?id=73002 shows the limit of the current approach.

Today I would replace the patch of 73002 by adding a new function to CSSPropertyParser so it could set/add the longhands for initial and inherit cases. I would do :

// For initial and inherit cases.
const CSSPropertyParser& handler = m_parserPropertyArray.propertyParser(propertyId);
if (handler.isValid() && handler.isShorthandHandler())
   return handler.setLonghands(...)

which is more efficient than having again a switch case where I need to compare the current propId and then finally set everything.

You could argue that to solve 73002, I could still enter in the existing switch case of parseValue and do some magic to make sure the longhands are added but it looked like an horrible hack when I did it.

One thing that this patch bring is that every properties that is handled by the array will be parse faster O(1) whereas the switch is such big that O(1) is not guarantee.

In a similar approach CSSStyleApplyProperty is not sexy to read, but is efficient.

I'm open on ideas on how to improve the patch to make it nicer.

> 
> What is the performance impact of this change? Does it make the code faster? Or slower? Or neither?

[Antti Koivisto]

Comment on attachment 125903 [details]
Patch

This patch clearly doesn't simplify the CSS property parsing and so fails to fix the bug. It does the opposite, making it very hard to see how a given property gets parsed.

[Darin Adler]

If the argument is about efficiency then we need to measure performance to show the gains.

[Alexis Menard (darktears)]

(In reply to comment #26)
> If the argument is about efficiency then we need to measure performance to show the gains.

I ran the perf test css-parser-yui and the differences are not enough to motivate that change. It is slightly faster but I think it's not worth the big move. It's called research, I failed. I will close that bug. Thanks for the time.

[Luke Macpherson]

(In reply to comment #27)
> (In reply to comment #26)
> > If the argument is about efficiency then we need to measure performance to show the gains.
> 
> I ran the perf test css-parser-yui and the differences are not enough to motivate that change. It is slightly faster but I think it's not worth the big move. It's called research, I failed. I will close that bug. Thanks for the time.

Trying something that didn't work out is not failure - that's called research success! You now know something that you could only have speculated about otherwise.