Bug 21158 - reduce use of virtual functions in Node for speed
: reduce use of virtual functions in Node for speed
Status: RESOLVED FIXED
: WebKit
XML DOM
: 528+ (Nightly build)
: All Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-09-26 17:44 PST by
Modified: 2008-09-28 02:35 PST (History)


Attachments
patch (29.84 KB, patch)
2008-09-26 17:57 PST, Darin Adler
sam: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-09-26 17:44:53 PST
Use the element and container bits more.
------- Comment #1 From 2008-09-26 17:57:43 PST -------
Created an attachment (id=23864) [details]
patch
------- Comment #2 From 2008-09-27 13:02:45 PST -------
(From update of attachment 23864 [details])
This looks good, but I am concerned about the change to ChildNodeList.  A ChildNodeList can contain non-Elements members and you have changed nodeMatches to take an Element (which makes the name weird now).  Does this work because nodeMatches is never called for a ChildNodeList as it implements its own item method?  If that is the case, can we remove the nodeMatches method from this subclass?
------- Comment #3 From 2008-09-27 15:17:02 PST -------
(In reply to comment #2)
> Does this work because
> nodeMatches is never called for a ChildNodeList as it implements its own item
> method?

Yes, that's why. The nodeMatches function there was just dead code!

There was no caller of nodeMatches that ever called it without first checking it was an Element. So the patch makes no behavior change.

I agree that we could give nodeMatches a better name. I didn't think of one while making this change; elementMatches does not seem like a better name. I also figured renaming in a separate patch would be better.

> If that is the case, can we remove the nodeMatches method from this
> subclass?

We can't, because nodeMatches is a pure virtual function in DynamicNodeList. But we could have it be empty with an ASSERT_NOT_REACHED() in it.

A cleaner fix would be to make all a class derived from DynamicNodeList that implements, length, item, and itemWithName and moving nodeMatches to that derived class, then using that as the base class for TagNodeList, NameNodeList, and ClassNodeList.

I really don't feel strongly about either of these.
------- Comment #4 From 2008-09-27 15:18:39 PST -------
Hmm, no, that's wrong. It looks like nodeMatches can get called by itemWithName. But since a non-Element can't have a name, that's still fine.

I think the patch is probably fine as-is. I didn't change the design at all. Let me know what you think.
------- Comment #5 From 2008-09-27 15:59:51 PST -------
(From update of attachment 23864 [details])
Ah, I forgot about the itemWithName case.  That makes sense.  r=me
------- Comment #6 From 2008-09-28 02:35:34 PST -------
http://trac.webkit.org/changeset/37037