Closed Bug 533637 Opened 12 years ago Closed 12 years ago

Speed up unwrapping a node in quickstubs that use nsINode (dromaeo)


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

Not set





(Reporter: bzbarsky, Assigned: peterv)




(2 files)

We have some quickstubs that use nsINode; for those cases it would be nice if we could effectively bypass our current offset table stuff and just bake the offset of nsINode from the canonical nsISupports (which should be the same for all nodes!) into the generated quickstub.  Peter was saying something about using a classinfo flag here, but I'm not sure why we need one.
OS: Mac OS X → All
Hardware: x86 → All
Assignee: nobody → peterv
Depends on: 554432, 519614
I'm not too happy about the macros in these patches, but don't really have better ideas. I inlined castNativeFromWrapper, it doesn't seem to increase size too much and did make a large difference in performance.

This is actually a bit scary, because we can't guarantee that the interfaces are castable from the canonical nsISupports pointer. I don't think we have a way to enforce that. Also, we share DOMCI between C++ classes, and this patch doesn't enforce that all the C++ classes have the same inheritance chain for the interfaces. For nsINode/nsIContent/... we already rely on that, but for other interfaces not so much.

I tried this out with GetNodeType, since we now have identical versions on nsINode and nsIDOMNode, so the only difference in these numbers is from this patch. These are the Shark profiles for calling .nodeType 1000000 times in a loop, focused just on the quickstub:

Without patch:

+ 2800, nsIDOMNode_GetNodeType(JSContext*, JSObject*, long, long*), XUL
| + 997, getWrapper(JSContext*, JSObject*, JSObject*, XPCWrappedNative**, JSOb
| |   364, XPCWrappedNative::GetWrappedNativeOfJSObject(JSContext*, JSObject*,
|   919, castNative(JSContext*, XPCWrappedNative*, JSObject*, XPCWrappedNative
|   102, non-virtual thunk to nsHTMLParagraphElement::GetNodeType(unsigned sho
|   30, nsGenericElement::GetNodeType(unsigned short*), XUL
|   25, XPCWrappedNative::GetWrappedNativeOfJSObject(JSContext*, JSObject*, JS
|   16, nsHTMLParagraphElement::GetNodeType(unsigned short*), XUL

With patch:

+ 607, nsIDOMNode_GetNodeType(JSContext*, JSObject*, long, long*), XUL
|   66, nsHTMLParagraphElement::GetNodeType(unsigned short*), XUL
|   34, nsGenericElement::GetNodeType(unsigned short*), XUL
Attachment #434924 - Flags: review?(jst)
Attachment #434926 - Flags: review?(jst)
Attachment #434924 - Flags: review?(jst) → review+
Comment on attachment 434926 [details] [diff] [review]
Use casting for unwrapping this pointers and arguments v1

- In dom/base/nsDOMClassInfoID.h:

+// WARNING: Be very careful when adding interfaces to this list. Every object
+//          that implements one of these interfaces must be directly castable
+//          to that interface from the *canonical* nsISupports!
+#define DOMCI_CASTABLE_INTERFACES(_extra)                                     \
+DOMCI_CASTABLE_INTERFACE(nsINode, 0, _extra)                                  \
+DOMCI_CASTABLE_INTERFACE(nsIContent, 1, _extra)                               \
+DOMCI_CASTABLE_INTERFACE(nsIDocument, 2, _extra)                              \
+DOMCI_CASTABLE_INTERFACE(nsINodeList, 3, _extra)                              \
+DOMCI_CASTABLE_INTERFACE(nsICSSDeclaration, 4, _extra)

Now this, and its uses, did take a while to pre-process in my head :) Maybe add some comments, pick a class and write out what sequence of Implements_Foo_interface() calls etc.

It does all look good though (as good as this amount of macro magic can look), and I can't think of a better way to do this.

Attachment #434926 - Flags: review?(jst) → review+
Depends on: 557757

Seems like this gave about a 4% win on Dromaeo DOM numbers on Linux and OS X.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
Depends on: 557919
Comment on attachment 434926 [details] [diff] [review]
Use casting for unwrapping this pointers and arguments v1

>+#define DOMCI_CASTABLE_INTERFACES(_extra)                                     \
Even if this is legal C++ it generates a slew of warnings in my build :-(
Yes, I know. I'll check in a fix soon.
Depends on: 583727
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.