Closed
Bug 570602
Opened 15 years ago
Closed 12 years ago
GetAttribute iterates over attribute list twice
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: sicking, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: perf)
Attachments
(2 files)
(deleted),
patch
|
bzbarsky
:
feedback+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details |
Currently GetAttribute first calls InternalGetExistingAttrNameFromQName which iterates over the attribute list to find an existing attribute with the given name. If an attribute is found, GetAttribute the calls GetAttr which iterates over the list again.
It is possible to only iterate the list one. I'm attaching a patch that does this. However in tests this doesn't seem to improve performance significantly. Possibly in the order of 5-10%, however the noise in my test is higher than that so it's hard to compare.
If anyone has ideas for how to further speed this up, ideas welcome.
Attachment #449733 -
Flags: feedback?(bzbarsky)
Reporter | ||
Comment 1•15 years ago
|
||
Just a simple test I whipped together.
Turns out that all attribute-related Dromaeo tests get attributes that aren't set. Thus it misses the "common path" of getting attributes that actually exist.
Comment 2•15 years ago
|
||
> Turns out that all attribute-related Dromaeo tests get attributes that aren't
> set.
The "getAttribute" test under dom-attr gets an attribute that's set, no?
Comment 3•15 years ago
|
||
Comment on attachment 449733 [details] [diff] [review]
Get rid of double-iterating
This seems eminently reasonable to me. That said, I have two questions:
1) It looks like nsGenericElement had NS_IMETHOD in the header but nsresult in the .cpp before this change? How did that compile at all on Windows?
2) If we changed nsXULElement to not NS_FORWARD_NSIDOMELEMENT to nsGenericElement::, would we still need the check for XUL in nsGenericElement?
Attachment #449733 -
Flags: feedback?(bzbarsky) → feedback+
Reporter | ||
Comment 4•15 years ago
|
||
(In reply to comment #3)
> (From update of attachment 449733 [details] [diff] [review])
> This seems eminently reasonable to me.
But is there a point to this if it doesn't improve perf? My basic conclusion here is that it's not worth landing this unless we can prove that it improves things more than what I have measured. Or unless we can squeeze more improvements out of this.
> 1) It looks like nsGenericElement had NS_IMETHOD in the header but nsresult in
> the .cpp before this change? How did that compile at all on Windows?
IIRC as an interesting piece of "luck" tryserver didn't seemed to have a hiccup and didn't give me *any* results on windows (red nor green). So it's quite possible that this doesn't compile on windows right now.
> 2) If we changed nsXULElement to not NS_FORWARD_NSIDOMELEMENT to
> nsGenericElement::, would we still need the check for XUL in nsGenericElement?
We would not. However that would be a whole lot of typing to save one branch. Especially one that doesn't happen on HTML elements. An alternative would be to insert a class inbetween nsGenericElement and nsXULElement.
Reporter | ||
Comment 5•15 years ago
|
||
(In reply to comment #2)
> > Turns out that all attribute-related Dromaeo tests get attributes that aren't
> > set.
>
> The "getAttribute" test under dom-attr gets an attribute that's set, no?
http://dromaeo.com/tests/dom-attr.html
is getting the 'id' attribute on the first <a>, which only appears to have href set.
Comment 6•15 years ago
|
||
> unless we can prove that it improves things more than what I have measured.
Fair.
> So it's quite possible that this doesn't compile on windows right now.
It should; I'm just surprised (and a little worried) the old code did.
> However that would be a whole lot of typing to save one branch
Plus a few memory loads, right?
> Especially one that doesn't happen on HTML elements.
Ah, right. Probably not worth it, then. Looking at the HTML element code, I wonder whether we could win more by doing exact-case getting first and then lowercasing.... maybe not, though.
> is getting the 'id' attribute on the first <a>
No. The test is:
test( "getAttribute", function(){
for ( var i = 0; i < num; i++ )
ret = elem.getAttribute("id");
});
and the prep is:
var elem = document.getElementById("test1");
The first <a> is only used for the setter tests and the expando getter test.
Comment 8•13 years ago
|
||
Well, measuring whether the patch actually helps anything would be a good start.
Probably worth revisiting this bug and seeing whether it's still relevant once bug 558516 lands.
Depends on: 558516
Comment 9•12 years ago
|
||
This was fixed by the patch in bug 558516.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Assignee | ||
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
•