Bug 21158 - reduce use of virtual functions in Node for speed
: reduce use of virtual functions in Node for speed
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: XML DOM
: 528+ (Nightly build)
: All Mac OS X 10.5
: P2 Normal
Assigned To: Darin Adler
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-09-26 17:44 PDT by Darin Adler
Modified: 2008-09-28 02:35 PDT (History)
1 user (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2008-09-26 17:44:53 PDT
Use the element and container bits more.
Comment 1 Darin Adler 2008-09-26 17:57:43 PDT
Created attachment 23864 [details]
patch
Comment 2 Sam Weinig 2008-09-27 13:02:45 PDT
Comment on attachment 23864 [details]
patch

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 Darin Adler 2008-09-27 15:17:02 PDT
(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 Darin Adler 2008-09-27 15:18:39 PDT
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 Sam Weinig 2008-09-27 15:59:51 PDT
Comment on attachment 23864 [details]
patch

Ah, I forgot about the itemWithName case.  That makes sense.  r=me
Comment 6 Darin Adler 2008-09-28 02:35:34 PDT
http://trac.webkit.org/changeset/37037