Closed Bug 582228 Opened 14 years ago Closed 14 years ago

Getting style attribute is very slow

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: smaug, Assigned: sicking)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

testcase
1.35 KB, text/html
Details
Patch to fix
16.58 KB, patch
bzbarsky
: review+
dbaron
: approval2.0+
Details | Diff | Splinter Review
Bustage fix.
1.03 KB, patch
Details | Diff | Splinter Review
Attached file testcase — 
Something like 95% of getAttribute time is spent under css::Declaration::ToString().

Could we cache the serialized value, and clear the cache if style somehow changes?

The slowness to serialize style attribute shows up a bit for example
in Zimbra.
We certainly could, if nsAttrValue can manage to store both a style rule and a string.  In fact, we could then set the string during parsing, and fix the bug whereby we don't round-trip @style by default, even in the absence of CSSOM manipulation.

This shouldn't need any work on the CSS side; should be doable entirely in the attr value code.
That said, I wonder why zimbra is doing getAttribute("style").  Are they changing the inline style in between these calls?
Blocks: 444379
I didn't see any getAttribute usage, but innerHTML. We get the attribute value
when serializing for ::GetInnerHTML
(Serializing should probably copy the attribute values in quite a bit different way, but that is not this bug.)
Oh, I see.  Then yeah, just caching might do the trick.  Can you shoehorn both pieces of data into an nsAttrValue?
Working on a patch
Assignee: nobody → jonas
Attached patch Patch to fix (obsolete) — — Splinter Review 
Just pushed this to try, so want to see it cycle there before asking reviews.
FWIW, on WinXP i get
* Today's Trunk Build: 213ms
* Tryserver's Build: 12ms
as Comparison:
* Chrome 6.0.472.14: 3ms
* Opera 10.61.3467: 15ms
Nice!
For what it's worth, the remaining time use seems to be:

6% nsAString destructor in the quickstub
4% nsAString constructors in the quickstub
10% instructions in the quickstub itself
4% on-trace code
22% converting the return value to a JSString (JS_NewExternalString, addrefs,
    etc).  See bug 558515.
30% calling InternalGetExistingAttrNameFromQName, with its lowercasing string
    ops stuff.
17% calling GetAttr.
(In reply to comment #9)
> 30% calling InternalGetExistingAttrNameFromQName, with its lowercasing string
>     ops stuff.
> 17% calling GetAttr.

I have patches to only construct a new string if we really need to lowercase and adding a GetExistingAttrValueFromQName to avoid GetAttr. I guess I'll file new bugs.
Those should probably go in bug 462353.
Jonas, how did try go?  We should get this fix in...
blocking2.0: --- → ?
Attached patch Patch to fix — — Splinter Review 
This should do it. Turns out we had a few tests that were relying on us parsing and "cleaning up" the style attribute. With this patch this no longer happens, similar to how we don't do it for class lists and enums etc.

There is a little bit of wasted performance here. In nsXULElement we end up cloning nsAttrValues holding style attributes. The current API causes a little bit extra string buffer refcounting. I suspect it won't really be a problem though.
Attachment #460605 - Attachment is obsolete: true
Attachment #463609 - Flags: review?(bzbarsky)
Comment on attachment 463609 [details] [diff] [review]
Patch to fix

r=bzbarsky.  Merge carefully on nsXULElement!
Attachment #463609 - Flags: review?(bzbarsky) → review+
I'm surprised those were the only tests.

In any case, if any tests *want* to be testing the parsing/serialization behavior, they can use element.style.cssText.  (In the past I've tried to remember to use that when I wanted to test parsing/serialization, though, since I knew we needed to change getAttribute to behave correctly.)
(In reply to comment #13)
> The current API causes a little
> bit extra string buffer refcounting.
This is only with xul style attribute, not with html style attribute?
Because in general string buffer refcounting is slow.
(In reply to comment #16)
> (In reply to comment #13)
> > The current API causes a little
> > bit extra string buffer refcounting.
> This is only with xul style attribute, not with html style attribute?
> Because in general string buffer refcounting is slow.

Yes, in HTML we always have done things even more slowly in the relevant code paths. The relevant code path is element cloning.
Comment on attachment 463609 [details] [diff] [review]
Patch to fix

I don't think this bug is a blocker, bug it's a nice perf win that I think we should take.
Attachment #463609 - Flags: approval2.0?
Could you please fix the following tests:

layout/style/test/test_bug373293.html
layout/style/test/test_bug382027.html

to use element.style.cssText instead of element.getAttribute("style"), since with this change they'll no longer be testing what they should be testing.
Er, actually, please fix test_bug373293 but NOT test_bug382027.  The second one should stay as it is.
(And it might be nice to add a test in the layout/style mochitests to check that element.style.cssText does do value reserialization and that element.getAttribute("style") does not, since tests rely on it.)
blocking2.0: ? → betaN+
This patch didn't apply cleanly, but with bz's help I figured it out and landed it on mozilla-central.

http://hg.mozilla.org/mozilla-central/rev/c0a3fe7e99a9
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: