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

RESOLVED FIXED in mozilla1.8beta2

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
mozilla1.8beta2
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Priority: -- → P1
Target Milestone: --- → mozilla1.8beta2
Created attachment 179246 [details] [diff] [review]
Like so

This would be simpler if we could depend on the bit on mData, but since we
can't...
Attachment #179246 - Flags: superreview?(dbaron)
Attachment #179246 - Flags: review?(dbaron)
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?
Attachment #179246 - Flags: superreview?(dbaron)
Attachment #179246 - Flags: superreview-
Attachment #179246 - Flags: review?(dbaron)
Attachment #179246 - Flags: review-
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.
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.
Depends on: 208732
Created attachment 180303 [details] [diff] [review]
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.
Attachment #179246 - Attachment is obsolete: true
Created attachment 180304 [details] [diff] [review]
Same as diff -w
Attachment #180304 - Flags: superreview?(dbaron)
Attachment #180304 - Flags: review?(dbaron)
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.
Attachment #180304 - Flags: superreview?(dbaron)
Attachment #180304 - Flags: superreview+
Attachment #180304 - Flags: review?(dbaron)
Attachment #180304 - Flags: review+
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....
Attachment #180304 - Flags: approval1.8b2?
Summary: [FIX]Don't call ValueAppended for DOM changes that don't create the value → [FIXr]Don't call ValueAppended for DOM changes that don't create the value

Comment 10

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

a=asa
Attachment #180304 - Flags: approval1.8b2? → approval1.8b2+
Fixed
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.