Closed Bug 636029 Opened 9 years ago Closed 9 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Keywords: memory-footprint, perf)

Attachments

(11 files)

81.04 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.90 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.64 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
105.89 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.91 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
86.97 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.19 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.44 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.46 KB, text/html; charset=UTF-8
Details
3.68 KB, text/plain; charset=UTF-8
Details
1.51 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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.
Attachment #514390 - Flags: review?(bzbarsky) → review+
Comment on attachment 514391 [details] [diff] [review]
patch 2: remove values for display: marker, run-in, and compact

r=me
Attachment #514391 - Flags: review?(bzbarsky) → review+
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.
Attachment #514392 - Flags: review?(bzbarsky) → review+
Comment on attachment 514393 [details] [diff] [review]
patch 4: add an initially-unused parsevariant_ field to the CSS_PROP macro

r=me
Attachment #514393 - Flags: review?(bzbarsky) → review+
Comment on attachment 514394 [details] [diff] [review]
patch 5: fix some incorrect keyword table parameters

r=me
Attachment #514394 - Flags: review?(bzbarsky) → review+
Comment on attachment 514396 [details] [diff] [review]
patch 7: add additional parse type for comma-separated lists of ParseVariant calls

r=me
Attachment #514396 - Flags: review?(bzbarsky) → review+
Comment on attachment 514397 [details] [diff] [review]
patch 8: use ParseSingleValueProperty inside parsing of 'transition' shorthand

r=me
Attachment #514397 - Flags: review?(bzbarsky) → review+
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.
Attachment #514395 - Flags: review?(bzbarsky) → review+
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)
Whiteboard: fixed-on-birch
Whiteboard: fixed-on-birch → fixed-in-birch
Depends on: 645750
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.
Attachment #531030 - Flags: review?(bzbarsky)
Comment on attachment 531030 [details] [diff] [review]
patch 9: fix mathml props

r=me
Attachment #531030 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.