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.

Array indexing for the win? ;)

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.

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 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 on attachment 514391 [details] [diff] [review] patch 2: remove values for display: marker, run-in, and compact r=me

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 on attachment 514393 [details] [diff] [review] patch 4: add an initially-unused parsevariant_ field to the CSS_PROP macro r=me

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

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

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

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

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

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)

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

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 on attachment 531030 [details] [diff] [review] patch 9: fix mathml props r=me

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