570602 - GetAttribute iterates over attribute list twice

Status: RESOLVED FIXED

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

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Get rid of double-iterating

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: 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.

Comment 2

[Boris Zbarsky [:bzbarsky]]

> 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

[Boris Zbarsky [:bzbarsky]]

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?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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

[Boris Zbarsky [:bzbarsky]]

> 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 7

[:aceman]

Hi, what is the state of the patch? What needs to be done now?

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[Boris Zbarsky [:bzbarsky]]

This was fixed by the patch in bug 558516.