Closed Bug 570602 Opened 12 years ago Closed 9 years ago

GetAttribute iterates over attribute list twice

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: sicking, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files)

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)
Attached file Testcase
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.
> 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 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+
(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.
(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.
> 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.
Hi, what is the state of the patch? What needs to be done now?
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
This was fixed by the patch in bug 558516.
Status: NEW → RESOLVED
Closed: 9 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.