636029 - don't use two huge switches over all CSS properties to parse CSS properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P4)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I have some patches that avoid the two huge switches in CSSParserImpl::ParseProperty(nsCSSProperty) and CSSParserImpl::ParseSingleValueProperty for most cases, although some properties will still go through one switch.  This is a code simplification and should also be a performance win.  It will mean that adding "simple-valued" CSS properties will no longer require any changes to the parser at all, since the necessary data will be stored in nsCSSPropList.h.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Array indexing for the win?  ;)

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So this is unfortunately tangled up with a *lot* of other stuff in my patch queue.  I think my strategy is going to be to try to get everything reviewed and then land things in the order they are in the queue.  (Otherwise I'd have to reorder the queue, which basically means dealing with merge conflicts I have with myself.)

I haven't yet pushed this to try, but I think it's in good shape, so I'll upload the patches for review.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 1: add parse types for different ways of parsing properties, and branch using them

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2: remove values for display: marker, run-in, and compact

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 3: separate values parsed in ParseSingleValueProperty with something other than ParseVariant

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 4: add an initially-unused parsevariant_ field to the CSS_PROP macro

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 5: fix some incorrect keyword table parameters

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 6: move the variant flags into nsCSSPropList, and drive ParseVariant calls by data

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 7: add additional parse type for comma-separated lists of ParseVariant calls

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 8: use ParseSingleValueProperty inside parsing of 'transition' shorthand

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that the "main" patches are in some sense patch 1 and patch 6, since they're the two patches that actually eliminate the two switches.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514390 [details] [diff] [review]
patch 1: add parse types for different ways of parsing properties, and branch using them

ParsePropertyFlags isn't really returning flags (in that its return value is not a set of bits, but just a value that's expected to be compared with ==).  Perhaps it should be called PropertyParseType?

> Don't use 0 so that
>+// we can verify that every property sets one of the bits.

s/bits/values/

r=me with those nits.  I did verify that all the parse bits in the prop list match the old code, for what it's worth.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514391 [details] [diff] [review]
patch 2: remove values for display: marker, run-in, and compact

r=me

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514392 [details] [diff] [review]
patch 3: separate values parsed in ParseSingleValueProperty with something other than ParseVariant

r=me, but add a PR_STATIC_ASSERT that CSS_PROPERTY_VALUE_PARSER_FUNCTION and CSS_PROPERTY_PARSE_PROPERTY_MASK  have no bits in common, please.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514393 [details] [diff] [review]
patch 4: add an initially-unused parsevariant_ field to the CSS_PROP macro

r=me

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514394 [details] [diff] [review]
patch 5: fix some incorrect keyword table parameters

r=me

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514396 [details] [diff] [review]
patch 7: add additional parse type for comma-separated lists of ParseVariant calls

r=me

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514397 [details] [diff] [review]
patch 8: use ParseSingleValueProperty inside parsing of 'transition' shorthand

r=me

Comment 19

[Boris Zbarsky [:bzbarsky]]

Working on patch 6, but it might overflow into tomorrow.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I did some benchmarking of this set of 8 patches, and it shows some
measureable improvement in property parsing, as measured by repeatedly
exercising property setters.  It's probably not measurable outside of
such a focused testcase, though.

In particular, I'm profiling a 64-bit Linux optimized build on Ubuntu
10.10 using this mozconfig:
https://hg.mozilla.org/users/dbaron_mozilla.com/mozconfigs/file/87e238e356cc/firefox-opt
on this revision of mozilla-central:
https://hg.mozilla.org/mozilla-central/rev/a820f7f5e50e with patches
from this patch queue: 
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/rev/f72dd08ddfa3
and in particular, comparing the state with the patch queue pushed to
prop-list-methods ("without the patches") vs. with the patch queue 
pushed to transition-shorthand-use-value-tables ("with the patches").

The testcase, which I'll attach, does 200000 property sets for each of
three property-value pairs, and times how long the loop takes.  I run
this testcase by starting up the browser with its file path on the
command line.  (The times seem to vary more within a run, getting higher
over time, perhaps due to the need for GC.)  I did 5 runs without the 
patches, 5 runs with, 5 more runs without, and 5 more runs with.

The basic results are:

Property-value pair      Per set before     Per set after    Faster
color: red                  3.68µs             3.48µs         5.4%
background: rgb(...)        5.94µs             5.80µs         2.4%
width: 20em                 3.09µs             2.76µs        10.8%

I claim statistical significance by eyeballing the numbers (see raw data,
also to be attached).

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: property setting benchmark

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: raw measurements

Comment 23

[jag (Peter Annema)]

Comment on attachment 514395 [details] [diff] [review]
patch 6: move the variant flags into nsCSSPropList, and drive ParseVariant calls by data

> PRBool
> CSSParserImpl::ParseSingleValueProperty(nsCSSValue& aValue,
>                                         nsCSSProperty aPropID)
> {

...

>+  if (aPropID < 0 || aPropID > eCSSProperty_COUNT_no_shorthands) {

Drive-by review: >=

FWIW, I've verified (with vim and regexp substitution) that the new annotations in the CSSPropList + new code match the old code in the parser for the common cases, and (as far as I can tell) script_level, stroke_miterlimit, and Extra_x_none_value are still handled the same.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 514395 [details] [diff] [review]
patch 6: move the variant flags into nsCSSPropList, and drive ParseVariant calls by data

> CSSParserImpl::ParseSingleValueProperty(nsCSSValue& aValue,
>+  if (aPropID < 0 || aPropID > eCSSProperty_COUNT_no_shorthands) {

That second test should be >=, right?

r=me with that.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I landed these patches on birch, though I made two small adjustments for test failures related to the added tests in patch 5:

 * first, I pulled a small part of patch 6 into a separate patch between patches 2 and 3, so that the state with patch 3 applied but not patch 6 applied would run without aborting (due to trying to get property flags for a nonproperty).  See https://hg.mozilla.org/projects/birch/rev/27ccd5dffb2c

 * second, I needed to add a nsRuleNode.cpp change to patch 5 so that the tests it added passed - -moz-column-rule-color: -moz-use-text-color was also missing its nsRuleNode code.  See https://hg.mozilla.org/projects/birch/rev/ab2102419355

https://hg.mozilla.org/projects/birch/rev/2a618c3fd2bc (1)
https://hg.mozilla.org/projects/birch/rev/25fc5805800b (2)
https://hg.mozilla.org/projects/birch/rev/27ccd5dffb2c (new, see above)
https://hg.mozilla.org/projects/birch/rev/eabdbf415b00 (3)
https://hg.mozilla.org/projects/birch/rev/8e8568fda119 (4)
https://hg.mozilla.org/projects/birch/rev/ab2102419355 (5; see above)
https://hg.mozilla.org/projects/birch/rev/3df5d287f516 (6, minus "new")
https://hg.mozilla.org/projects/birch/rev/412c1f51d631 (7)
https://hg.mozilla.org/projects/birch/rev/f0f1632c19e8 (8)

Comment 26

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/mozilla-central/rev/2a618c3fd2bc
https://hg.mozilla.org/mozilla-central/rev/25fc5805800b
https://hg.mozilla.org/mozilla-central/rev/27ccd5dffb2c
https://hg.mozilla.org/mozilla-central/rev/eabdbf415b00
https://hg.mozilla.org/mozilla-central/rev/8e8568fda119
https://hg.mozilla.org/mozilla-central/rev/ab2102419355
https://hg.mozilla.org/mozilla-central/rev/3df5d287f516
https://hg.mozilla.org/mozilla-central/rev/412c1f51d631
https://hg.mozilla.org/mozilla-central/rev/f0f1632c19e8

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 9: fix mathml props

While working on bug 653842, I noticed I did something a little odd here with the internal MathML props -- they probably should have been marked as INACCESSIBLE to correspond to the old code in ParseSingleValueProperty, but somehow I missed them.  I don't think it's actually harmful since the variant mask is 0, so we won't actually accept any values, but I think we should still fix it.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 531030 [details] [diff] [review]
patch 9: fix mathml props

r=me

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Landed patch 9:
https://hg.mozilla.org/mozilla-central/rev/cc57061c1258