Closed Bug 407812 Opened 13 years ago Closed 13 years ago

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

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jst, Assigned: jst)

References

Details

Attachments

(2 files)

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).
Attachment #292487 - Flags: superreview?(jonas)
Attachment #292487 - Flags: review?(jonas)
Assignee: nobody → jst
Flags: blocking1.9+
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.
Attachment #292487 - Flags: superreview?(jonas)
Attachment #292487 - Flags: superreview+
Attachment #292487 - Flags: review?(jonas)
Attachment #292487 - Flags: review+
jst, what's the reason for the nsImageDocument QI change?
The macro it used made the identity pointer not be the nsINode pointer, which we can't have with this change.
Fix checked in.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #292513 - Attachment mime type: application/octet-stream → text/plain
Attachment #292513 - Attachment is patch: true
Flags: in-testsuite-
> 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? 
(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
(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).
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.
Fair enough. You want to roll that change into your nsDocument patch while you're there? If not, I can file a followup bug...
Yeah, I can do that.
Depends on: 518498
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.