564575 - [FIX]Use iteration, not recursion, for registering/unregistering named items

Status: RESOLVED WONTFIX

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

Description

[Boris Zbarsky [:bzbarsky]]

Attachment: Testcase

As the summary says.

This patch wins us about 6-7% on the attached testcase; still a bit behind tip webkit (which started out 15% ahead of us).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Comment 2

[Peter Van der Beken [:peterv]]

Would it make sense to have a GetFirstElement (inline, returns this if IsElement is true or calls GetNextElement) and GetNextElement (inline, calls GetNextNode until IsElement is true)? I don't have data, but it seems like that would be a common thing to want to do?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 444234 [details] [diff] [review]
Like so

> Would it make sense to have a GetFirstElement (inline, returns this if
> IsElement is true or calls GetNextElement) and GetNextElement (inline, calls
> GetNextNode until IsElement is true)? 

Probably, yes.  I'll roll that into the patch in bug 564435.

This patch is about to become irrelevant when bug 564863 lands.

Comment 4

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

(In reply to comment #2)
> Would it make sense to have a GetFirstElement (inline, returns this if
> IsElement is true or calls GetNextElement) and GetNextElement (inline, calls
> GetNextNode until IsElement is true)? I don't have data, but it seems like that
> would be a common thing to want to do?

Ooh, i like this idea. Don't think it'll speed up anything, but it should make for cleaner code through more code reuse.

Comment 5

[Boris Zbarsky [:bzbarsky]]

I just tried that in the patch for bug 564435.  On the testcase in that bug, it makes the code somewhat slower (about 5% or so), presumably due to the extra loop and corresponding null-checks.  Core cleanliness is not obviously much better than it used to be, due to having to start loops with GetFirstElement/GetNextElement instead of whatever the actual object you have is.

I filed bug 564883 on adding that api, if we decide to do it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Wontfix, per bug 564863.