395444 - Fast-path QueryInterface in XPCWrappedNative::CallMethod

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Benjamin Smedberg]

Attachment: Special-case QI, rev. 1

Right now calls to QueryInterface from script go through the ordinary path where we use typeinfo to convert params and then call NS_InvokeByIndex. Because we know the signature at compile-time we can optimize this (common) path pretty significantly.

Comment 1

[Johnny Stenback (:jst)]

Comment on attachment 280118 [details] [diff] [review]
Special-case QI, rev. 1

sr=jst

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 280118 [details] [diff] [review]
Special-case QI, rev. 1

>Index: js/src/xpconnect/src/xpcwrappednative.cpp
>+    // fast-path QueryInterface: we already know the signature and can avoid a lot of work
>+    if (vtblIndex == 0)

Should we also check that the interface is nsISupports? 

>+    {
>+        if (argc < 1)

Nit: No space after |if|. (this applies to all added if statements).

>+        {
>+            Throw(NS_ERROR_XPC_NOT_ENOUGH_ARGS, ccx);
>+            return JS_FALSE;
>+        }
>+        nsID *iid;
>+        JSObject *obj;

Nit: * cuddles with the type in surrounding code.

>+        if (NS_FAILED(invokeResult)) {

Nit: { on its own line (and no space...).

Another general comment, perhaps we should be setting retval and using |goto done;|. It appears that the only code that we care about there at this point is the timing code, if we don't care about that, then there's some additional cleanup possible to do around here.

Other than that, this looks good!

Comment 3

[Benjamin Smedberg]

> Should we also check that the interface is nsISupports? 

Not necessary, because there is already xpconnect code that prevents reflecting interfaces that don't inherit from nsISupports, and xpidl prevents such interfaces anyway.

> Another general comment, perhaps we should be setting retval and using |goto
> done;|. It appears that the only code that we care about there at this point is
> the timing code, if we don't care about that, then there's some additional
> cleanup possible to do around here.

I don't think that that #ifdef off_jband code is useful... we don't have enough resolution in most cases for it to be useful, unless we start instrumenting this stuff with dtrace and then we'll need some different stuff anyway. I like skipping the extra cleanup code if we don't need it, so a second patch coming up.
> 
> Other than that, this looks good!
> 

Comment 4

[Benjamin Smedberg]

Attachment: Special-case QI, rev. 1.1

Comment 5

[Benjamin Smedberg]

Fixed on trunk. Didn't appear to have any appreciable perf impact, but this is necessary for the automated QI rewrite (bug 391275) so I'll leave it in unless there are reported regressions.