288574 - [FIXr]Don't call ValueAppended for DOM changes that don't create the value

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is spun off of bug 208727 comment 1.  On the testcase at
http://www.speich.net/computer/3d.php the patch I'll post to implement this cuts
our time in TransferTempData by a factor of 5 (from about 1% of total time, to
about 0.2%).

Given that this also gives better behavior with editor in general (per bug
208727 comment 2), I think it's worth the bit of extra code.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

This would be simpler if we could depend on the bit on mData, but since we
can't...

Comment 2

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

Comment on attachment 179246 [details] [diff] [review]
Like so

>@@ -752,17 +758,18 @@ CSSParserImpl::ParseAndAppendDeclaration
>+    if (!ParseDeclaration(errorCode, aDeclaration, PR_FALSE,
>+                          PR_FALSE, aChanged)) {

I think this should be passing PR_TRUE.  Otherwise duplicate declarations in
CSSStyleDeclaration::cssText setter won't be serialized consistently with other
duplicate declarations.

That will mess up the case:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/style/nsDOMCSSDeclar
ation.cpp&rev=3.60&mark=215-221#194
but that code should probably be fixed anyway.

>+  if (aMustCallValueAppended || !PropertySetInData(aPropID)) {

Why can't you just check the property bit in mData instead of writing this new
PropertySetInData function?

Comment 3

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

Was it because of this?

    /*
     * mPropertiesSet stores a bit for every property that may be
     * present, to optimize compression of blocks with small numbers of
     * properties (the norm).  The code does not rely on it to be exact;
     * it is allowable, although slower, if a bit is erroneously set
     * even though the property is not present.
     */

If so, perhaps you should make it exact.  IIRC, it shouldn't be hard, if it
requires any changes at all.

Comment 4

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

(See bug 208732.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Yes, the extra code was because the bit is not exact.  You're right that we
should just fix bug 208732 first.  I'll look into it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch that makes the bit exact

This makes the bit exact, except for mTempData in the parser where a null value
with the bit set has a "special" meaning....  I've tested that this doesn't
change our behavior for system fonts.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: Same as diff -w

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180304 [details] [diff] [review]
Same as diff -w

This actually gives even more improvement on DHTML, because we do less work for
ComputeSize and Compress.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 180304 [details] [diff] [review]
Same as diff -w

Requesting 1.8b approval.  This helps out with DHTML a little bit, makes our
serialization of inline style better, and is pretty safe....

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 180304 [details] [diff] [review]
Same as diff -w

a=asa

Comment 11

[Boris Zbarsky [:bzbarsky]]

Fixed