Closed
Bug 582228
Opened 14 years ago
Closed 14 years ago
Getting style attribute is very slow
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: smaug, Assigned: sicking)
References
(Blocks 1 open bug)
Details
Attachments
(3 files, 1 obsolete file)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
bzbarsky
:
review+
dbaron
:
approval2.0+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review |
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.
Comment 1•14 years ago
|
||
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.
Comment 2•14 years ago
|
||
That said, I wonder why zimbra is doing getAttribute("style"). Are they changing the inline style in between these calls?
Reporter | ||
Comment 3•14 years ago
|
||
I didn't see any getAttribute usage, but innerHTML. We get the attribute value
when serializing for ::GetInnerHTML
Reporter | ||
Comment 4•14 years ago
|
||
(Serializing should probably copy the attribute values in quite a bit different way, but that is not this bug.)
Comment 5•14 years ago
|
||
Oh, I see. Then yeah, just caching might do the trick. Can you shoehorn both pieces of data into an nsAttrValue?
Assignee | ||
Comment 7•14 years ago
|
||
Just pushed this to try, so want to see it cycle there before asking reviews.
Comment 8•14 years ago
|
||
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!
Comment 9•14 years ago
|
||
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.
Comment 10•14 years ago
|
||
(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.
Comment 11•14 years ago
|
||
Those should probably go in bug 462353.
Assignee | ||
Comment 13•14 years ago
|
||
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 14•14 years ago
|
||
Comment on attachment 463609 [details] [diff] [review]
Patch to fix
r=bzbarsky. Merge carefully on nsXULElement!
Attachment #463609 -
Flags: review?(bzbarsky) → review+
Comment 15•14 years ago
|
||
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.)
Reporter | ||
Comment 16•14 years ago
|
||
(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.
Assignee | ||
Comment 17•14 years ago
|
||
(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.
Assignee | ||
Comment 18•14 years ago
|
||
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?
Updated•14 years ago
|
Attachment #463609 -
Flags: approval2.0? → approval2.0+
Comment 19•14 years ago
|
||
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.
Comment 20•14 years ago
|
||
Er, actually, please fix test_bug373293 but NOT test_bug382027. The second one should stay as it is.
Comment 21•14 years ago
|
||
(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.)
Updated•14 years ago
|
blocking2.0: ? → betaN+
Comment 22•14 years ago
|
||
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
Comment 23•14 years ago
|
||
Comment 24•14 years ago
|
||
Bustage fixed.
http://hg.mozilla.org/mozilla-central/rev/53bf594b6fb6
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•