Simplify CSS shorthand handling.
Created attachment 124160 [details] Patch
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.
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.
Comment on attachment 124160 [details] Patch Attachment 124160 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11163455
Comment on attachment 124160 [details] Patch Attachment 124160 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11265446
Comment on attachment 124160 [details] Patch Attachment 124160 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11163490
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.
(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.
Created attachment 124341 [details] Patch
Comment on attachment 124341 [details] Patch Attachment 124341 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11350820
Created attachment 124345 [details] Patch
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
Created attachment 124358 [details] Patch
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
Created attachment 124542 [details] Patch
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?
(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.
Created attachment 125692 [details] Patch
(In reply to comment #18) > Created an attachment (id=125692) [details] > Patch Just want to gather feedback before going further.
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?
(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
Created attachment 125903 [details] Patch
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
(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?
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.
If the argument is about efficiency then we need to measure performance to show the gains.
(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.
(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.