407812 - nsNodeSH::PreCreate() shouldn't QI to nsINode.

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

Attachment: Remove QI in nsNodeSH::PreCreate() and nsElementSH::PostCreate().

Part of the data coversion overhead in XPConnect's function calling code ends up being wrapping of nodes, and part of that code is calling into the scriptable helper hooks, especially nsNodeSH::PreCreate(). That function QI's the native to nsINode, but we already know that all natives that come in there are classes that implement nsINode. If we change the QI implementation in a few classes, we can replace the QI with a cast to nsINode. Patch attached.

(this is one of several performance optimizations that I'm about to file).

Comment 1

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

Comment on attachment 292487 [details] [diff] [review]
Remove QI in nsNodeSH::PreCreate() and nsElementSH::PostCreate().

I don't think you really need the assertion in nsDocument::Init as the other assertions should catch that. Do add comments to the other assertions describing what is wrong and how to fix it though.

Comment 2

[Boris Zbarsky [:bzbarsky]]

jst, what's the reason for the nsImageDocument QI change?

Comment 3

[Johnny Stenback (:jst)]

The macro it used made the identity pointer not be the nsINode pointer, which we can't have with this change.

Comment 4

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in.

Comment 5

[Johnny Stenback (:jst)]

Fix checked in.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> The macro it used made the identity pointer not be the nsINode pointer

Ah, true.  Can we switch to NS_INTERFACE_TABLE_INHERITED4, which is what it should have been to start with? 

Comment 7

[Brendan Eich [:brendan]]

(In reply to comment #6)
> > The macro it used made the identity pointer not be the nsINode pointer
> 
> Ah, true.  Can we switch to NS_INTERFACE_TABLE_INHERITED4, which is what it
> should have been to start with? 

Followup bug needed...

/be

Comment 8

[Johnny Stenback (:jst)]

(In reply to comment #6)
> Ah, true.  Can we switch to NS_INTERFACE_TABLE_INHERITED4, which is what it
> should have been to start with? 
> 

We could, but I seriously doubt it's worth it, it's a class with only 4 interfaces, and no other document, or DOM nodes in general bother with the table based QI impls (favors code sharing by hand-coding QI impls).

Comment 9

[Boris Zbarsky [:bzbarsky]]

Actually, all HTML node and document classes use table-based QI (or did up to this checkin).  I have patches in my tree to make nsDocument use it as well that I just haven't had time to make presentable for 1.9.

When I landed those, there was a small codesize win, with a slightly win in terms of moving things from the code to data segment.

Comment 10

[Johnny Stenback (:jst)]

Fair enough. You want to roll that change into your nsDocument patch while you're there? If not, I can file a followup bug...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yeah, I can do that.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Filed bug 408403