This is a spinoff from bug 198533 on the ideas brought up by bryner in bug 198533 comment 54. Basically it consists of creating an nsIElement interface with an mNodeInfo member and creating inlined versions of the functions on nsIContent that only access various aspects of that member, such as Tag() and NamespaceID(). Somewhat related is getting rid of the nsIXULContent interface. This can be done by casting directly to nsXULElement. We could possibly also move some of the methods to nsIElement or nsIContent. Note that with the patch in bug 198533 it would be trivial to move the implementation of the lazystate functions to nsGenericElement, however I'm not sure if that is the right thing to do since obviously all nonxul elements will ignore those flags.

So would we then move the functions involved off nsIContent? Or just use nsIElement when we want to optimize? The fear I have is of lots of QIs between nsIContent and nsIElement. We could possibly ameliorate this by changing some method signatures to take nsIElement, though...

(In reply to comment #1) > So would we then move the functions involved off nsIContent? Or just use > nsIElement when we want to optimize? The fear I have is of lots of QIs between > nsIContent and nsIElement. We could possibly ameliorate this by changing some > method signatures to take nsIElement, though... Yes, I'm worried about the potential for extra QI's as well. Just as a hypothetical, consider a loop like this: for (i = 0; i < childCount; ++i) { nsIContent *child = parent->GetChildAt(i); if (child->Tag() == nsHTMLAtoms::img) { // do something } } Suppose we're able to inline Tag(). Now the loop looks like this: for (i = 0; i < childCount; ++i) { nsCOMPtr<nsIElement> element = do_QueryInterface(parent->GetChildAt(i)); if (element && element->Tag() == nsHTMLAtoms::img) { ... } } This has now become 2 virtual method calls per loop iteration (QueryInterface and Release) plus the cost of the inline Tag() implementation. It could be improved some by eliminating the refcounting, using IsContentOfType() plus a cast, as I did in the patch I posted to bug 198533. That gets you down to 1 virtual method call, plus the cost of the inline Tag() implementation. Since that's obviously more than just the cost of a single virtual method call, this is a penalty, not a win. The only way it's a win is if you can get an nsIElement pointer _without_ QI'ing to it. To make that happen, we would have to change method signatures where it makes sense to do so. For example, nsIContent::GetParent() can return an nsIElement*. The nsIDocumentObserver methods that deal with content insertion and removal can use an nsIElement* for the containing element. There are probably lots of others. All of the calls to these methods from inside the element classes would become inline as well (or at least nonvirtual). It may also make sense, as you said, to have versions of the element methods on nsIContent, just to avoid making the cost of the call ever be _more_ than a single virtual method call. (It would be really great if frames could have an nsIElement pointer instead of an nsIContent pointer, too, with some special magic for text frames). I think this could be a big improvement, but it's hard to be sure without actually coding it up. I'll try to do this based on the current patch in bug 198533, and post my findings.

Another option is to put mNodeInfo in nsIContent. It would waste 4 bytes on every textnode (and comment and PI, but i doubt they matter), but it might be a nice simple way to save cycles. If nothing else it's a simple way to an estimate of how many cycles we could potentially win before changing a lot of signatures.

re comment 2, you can maybe avoid the second virtual call (Release) if you add a method like: virtual nsIElement* IsElement() = 0; to nsIContent, which returns |this| is the object is an element and |nsnull| otherwise.

Hmm.. why couldn't we replace mDocument with mNodeInfo in nsGenericDOMDataNode? That should fix bug 27382 as well as making it possible to inline the following functions in nsIContent for *all* nodes. GetDocument IsInDoc GetOwnerDoc GetCurrentDoc (sXBL might change this) GetNameSpaceID (actually, bug 103225 blocks this one) Tag GetNodeInfo (is there code relying on this being null for non-elements?) And with a bit of work possibly also GetIDAttributeName.

Comment on attachment 171389 [details] [diff] [review] remove nsIXULContent mmmm... decomtamination... >@@ -141,8 +137,8 @@ nsDragService::ComputeGlobalRectFromFram > #if USE_TRANSLUCENT_DRAGGING && defined(MOZ_XUL) > // until bug 41237 is fixed, only do translucent dragging if the drag is in > // the chrome or it's a link. >- nsCOMPtr<nsIXULContent> xulContent ( do_QueryInterface(aDOMNode) ); >- if ( !xulContent ) { >+ nsCOMPtr<nsIContent> content = do_QueryInterface(aDOMNode); >+ if (!content->IsContentOfType(nsIContent::eXUL)) { Not sure if it's needed, but you might want to test for null-content here too, just in case someone passes a js-object that doesn't implement nsIContent

Comment on attachment 171389 [details] [diff] [review] remove nsIXULContent sr=jst

This appears to have caused a regression. See attachment 173089 [details] for a test case. It works in the 2005-01-19-08 build but not the 2005-01-20-08 build.

Actually, that's a regression from bug 244366. Is there a bug filed on it?

WONTFIXing this. All the mentioned methods are now inlined on nsIContent.