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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: Couple DOMCI with specific C++ classes v1

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Use casting for unwrapping this pointers and arguments v1

Comment 3

[Peter Van der Beken [:peterv]]

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

Comment 4

[Johnny Stenback (:jst)]

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.

r=jst

Comment 5

[Peter Van der Beken [:peterv]]

http://hg.mozilla.org/mozilla-central/rev/8e6cbbc07c71
http://hg.mozilla.org/mozilla-central/rev/88f469d18c97
http://hg.mozilla.org/mozilla-central/rev/466f5249ac33

Seems like this gave about a 4% win on Dromaeo DOM numbers on Linux and OS X.

Comment 6

[[e-mail]]

Comment on attachment 434926 [details] [diff] [review]
Use casting for unwrapping this pointers and arguments v1

>+#define DOMCI_CASTABLE_INTERFACES(_extra)                                     \
...
>+DOMCI_CASTABLE_INTERFACES()
Even if this is legal C++ it generates a slew of warnings in my build :-(

Comment 7

[Peter Van der Beken [:peterv]]

Yes, I know. I'll check in a fix soon.