Closed Bug 1456035 Opened 6 years ago Closed 6 years ago

Add native QueryInterface helper with fast path for XPCWrappedJS

Categories

(Core :: XPConnect, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Performance Impact high

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(6 files)

This has been one of my pet peeves for a while.

Our JS QueryInterface implementations get are expensive, and get called *a lot*. A native implementation could be much more efficient, even if it only cut out all of the XPC call overhead for equality checks. But removing the C++->JS call overhead should be a sizable added bonus.
Comment on attachment 8970070 [details]
Bug 1456035: Part 3 - Replace XPCOMUtils.generateQI with a stub for ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/238828/#review244812

::: js/xpconnect/loader/XPCOMUtils.jsm:104
(Diff revision 2)
>     * If the JS object has a classInfo property it'll be returned for the
>     * nsIClassInfo IID, generateCI can be used to generate the classInfo
>     * property.
>     */
>    generateQI: function XPCU_generateQI(interfaces) {
> -    /* Note that Ci[Ci.x] == Ci.x for all x */
> +    return ChromeUtils.generateQI(interfaces);

You should put in a comment here saying that it is better to call ChromeUtils.generateQI directly. I guess you didn't remove this entirely because you are worried about the handful of out-of-tree chrome JS users? Or is there some other reason you can't get rid of it after part 4?
Attachment #8970070 - Flags: review?(continuation) → review+
Comment on attachment 8970070 [details]
Bug 1456035: Part 3 - Replace XPCOMUtils.generateQI with a stub for ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/238828/#review244812

> You should put in a comment here saying that it is better to call ChromeUtils.generateQI directly. I guess you didn't remove this entirely because you are worried about the handful of out-of-tree chrome JS users? Or is there some other reason you can't get rid of it after part 4?

Yeah, partly out-of-tree callers, and partly some uses in devtools that are hard to rewrite without manually rejiggering some imports in the files.

I think we should probably aim to this when ChromeUtils.generateQI is available on all channels, so we don't need to worry about compatibility problems for system extensions.
Comment on attachment 8970071 [details]
Bug 1456035: Part 4 - Convert callers of XPCOMUtils.generateQI to ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/238830/#review244828

Be sure to post to dev-firefox and dev-platform about this after it lands.

::: browser/extensions/pdfjs/content/PdfStreamConverter.jsm:812
(Diff revision 3)
>    contractID: "@mozilla.org/streamconv;1?from=application/pdf&to=*/*",
>  
>    classID2: Components.ID("{d0c5195d-e798-49d4-b1d3-9324328b2292}"),
>    contractID2: "@mozilla.org/streamconv;1?from=application/pdf&to=text/html",
>  
> -  QueryInterface: XPCOMUtils.generateQI([
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsIStreamConverter, Ci.nsIStreamListener, Ci.nsIRequestObserver]),

I think you'll need to upstream these changes to pdf.js, or they'll get overwritten in the next update.

::: caps/tests/mochitest/test_extensionURL.html:94
(Diff revision 3)
>    SubstitutionObserver.prototype = {
>      onSetSubstitution: SpecialPowers.wrapCallback(function(root, uri) {
>        popSubstitutionCallback(root);
>      }),
>      QueryInterface:
> -      XPCOMUtils.generateQI([SpecialPowers.Ci.nsISupports,
> +      ChromeUtils.generateQI([SpecialPowers.Ci.nsISupports,

Can you remove this SpecialPowers.Ci.nsISupports here?

::: taskcluster/docker/periodic-updates/scripts/getHSTSPreloadList.js:163
(Diff revision 3)
>    asyncPromptAuth(channel, callback, context, level, authInfo) {
>      throw new Error(Cr.NS_ERROR_NOT_IMPLEMENTED);
>    },
>  
>    getInterface(iid) {
> -    return this.QueryInterface(iid);
> +    return this.QueryInterfChromeUtils.generateQIryInterface: XPCOMUtils.generateQI([Ci.nsIChannelEventSink, Ci.nsIAuthPrompt2])

Something has gone terribly wrong here. :)

::: toolkit/mozapps/extensions/internal/AddonTestUtils.jsm:161
(Diff revision 3)
>    getPluginBlocklistState(plugin, version, appVersion, toolkitVersion) {
>      return Ci.nsIBlocklistService.STATE_NOT_BLOCKED;
>    }
>  }
>  
> -MockBlocklist.prototype.QueryInterface = XPCOMUtils.generateQI(["nsIBlocklistService"]);
> +MockBlocklist.prototype.QueryInterface = ChromeUtils.generateQI(["nsIBlocklistService"]);

Does passing in a string like this work? Seems weird.
Attachment #8970071 - Flags: review?(continuation) → review+
Comment on attachment 8970071 [details]
Bug 1456035: Part 4 - Convert callers of XPCOMUtils.generateQI to ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/238830/#review244828

> Something has gone terribly wrong here. :)

Oof. I read over the patch twice. I guess that wasn't enough...
I guess bug 1425616 is basically a dupe of this now? :-)
Blocks: 1425466
(In reply to :Gijs (he/him) from comment #15)
> I guess bug 1425616 is basically a dupe of this now? :-)

Oh, yeah. I remembered talking with people about it, but I couldn't find an existing bug. Thanks
Comment on attachment 8970312 [details]
Bug 1456035: Part 5 - Convert manual QueryInterface to ChromeUtils.generateQI.

https://reviewboard.mozilla.org/r/239114/#review245080
Attachment #8970312 - Flags: review?(continuation) → review+
Comment on attachment 8970068 [details]
Bug 1456035: Part 1 - Add helper to generate native QueryInterface callbacks.

https://reviewboard.mozilla.org/r/238824/#review245082

::: dom/base/MozQueryInterface.h:17
(Diff revision 2)
> +#include "mozilla/ErrorResult.h"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class MozQueryInterface final : public NonRefcountedDOMObject

I think the name of this class, or at least a comment, should indicate that this is only intended for use from JS. Also, this needs a comment indicating that the reason for this is to make QIs faster.

As a side note, a nice aspect of the way this is set up is that you could potentially create alternate implementations with a statically fixed of interfaces if somehow the C++ QI itself shows up as being too slow. I guess at that level the JS to C++ overhead is probably higher.
You should add a failure test case with a non-empty QI. I think your current tests would all pass if you had |return true| instead of |return CompareIIDs(aIID, aOther)| in QueriesTo.
Comment on attachment 8970069 [details]
Bug 1456035: Part 2 - Add fast path for XPCWrappedJS QueryInterface with native helper.

https://reviewboard.mozilla.org/r/238826/#review245072

::: js/xpconnect/src/XPCWrappedJSClass.cpp:238
(Diff revision 2)
>          }
>      }
>  
> -    id = xpc_NewIDObject(cx, jsobj, aIID);
> -    if (id) {
> +    dom::MozQueryInterface* mozQI = nullptr;
> +    if (NS_SUCCEEDED(UNWRAP_OBJECT(MozQueryInterface, &fun, mozQI))) {
> +        success = mozQI->QueriesTo(aIID);

You could make this
if (mozQI->QueriesTo(aIID))
  return jsobj.get();
else
  return nullptr;
rather than dealing with this weird |success| thing. Then you also wouldn't need to touch the rest of the function.
Attachment #8970069 - Flags: review?(continuation) → review+
(In reply to Andrew McCreight [:mccr8] from comment #20)
> You should add a failure test case with a non-empty QI. I think your current
> tests would all pass if you had |return true| instead of |return
> CompareIIDs(aIID, aOther)| in QueriesTo.

This test should cover that case:

  Assert.throws(() => checkQI([], Ci.nsIPropertyBag),
                e => e.result == Cr.NS_ERROR_NO_INTERFACE);
Oh, right, an empty QI implicitly has nsISupports in the list.
(In reply to Andrew McCreight [:mccr8] from comment #19)
> As a side note, a nice aspect of the way this is set up is that you could
> potentially create alternate implementations with a statically fixed of
> interfaces if somehow the C++ QI itself shows up as being too slow. I guess
> at that level the JS to C++ overhead is probably higher.

I think the dynamic version would probably be faster at this point, because it sorts the IIDs and then does a binary search at query time. Unfortunately, there's no good way to sort a static list at compile time...

It might make the construction a bit faster if we had static lists, but I think the call-time overhead tends to be more important.

That said, in my profiles, the ~9ms of startup overhead from creating QueryInterface methods and ~15ms from calling them dropped to ~1ms after these changes. So we're probably in a pretty good situation for the time being.
This seems like a big win at face value.  JS-based QI implementations are a pretty big deal and speeding them up is good.  Unless presented with arguments otherwise, I'm inclined to mark this p1 for 64.  Seeing as a bunch of the work is done and waiting for review, the landing confidence seems high as well.
Whiteboard: [qf] → [qf:p1:f64]
Priority: -- → P3
Comment on attachment 8970068 [details]
Bug 1456035: Part 1 - Add helper to generate native QueryInterface callbacks.

https://reviewboard.mozilla.org/r/238824/#review245452

::: dom/base/MozQueryInterface.h:17
(Diff revision 2)
> +#include "mozilla/ErrorResult.h"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class MozQueryInterface final : public NonRefcountedDOMObject

A class comment in this file (after the include guards, in a /** */ block) that describes what it's implementing would be nice.

::: dom/base/MozQueryInterface.cpp:20
(Diff revision 2)
> +#include "xpcjsid.h"
> +
> +namespace mozilla {
> +namespace dom {
> +
> +constexpr size_t IID_SIZE = 16;

Is there a reason this can't be sizeof(nsIID)?

::: dom/base/MozQueryInterface.cpp:81
(Diff revision 2)
> +    if (val.isNullOrUndefined()) {
> +      continue;

Doing this means that mis-spelling an interface will just silently be ignored.  Why is that the behavior we want?  I guess that's what you get now if you hand-write your QI...

At the very least this continue needs some documentation explaining why this is the behavior.

::: dom/base/MozQueryInterface.cpp:84
(Diff revision 2)
> +    }
> +
> +    if (val.isNullOrUndefined()) {
> +      continue;
> +    }
> +    if (!val.isObject() || !xpc::IsReflector(&val.toObject())) {

Why do you need the IsReflector() check here?  If that fails, UnwrapReflectorToISupports will just return null and we'll throw anyway, right?  So the IsReflector() check is redundant.

::: dom/base/MozQueryInterface.cpp:109
(Diff revision 2)
> +  // We use BinarySearchIf here because nsTArray::ContainsSorted requires
> +  // twice as many comparisons.

Ah, because it does an Equals(), then a LessThan()?

This is worth filing a followup bug on nsTArray.  It seems like we should be able to support having a comparator that provides a "Compare" operation returning a tristate, with fallback to the current setup if there is no "Compare".

::: dom/base/MozQueryInterface.cpp:125
(Diff revision 2)
> +  } else if (!thisv.isObject()) {
> +    aRv.Throw(NS_ERROR_UNEXPECTED);

Do we really care about that case?  That is, can we just declare the legacycaller to return "any" and return thisv, whatever it is, if the IID matches.  One fewer compare in the common case, and if someone pulls a QI method off an object and calls it with a bogus "this".... well, it's no different from what happens right now with "return this" in XPCOMUtils_QueryInterface.

::: dom/bindings/Bindings.conf:616
(Diff revision 2)
>      'headerFile': 'mozilla/storage/mozStorageStatementRow.h',
>      'nativeType': 'mozilla::storage::StatementRow',
>  },
>  
> +'MozQueryInterface': {
> +    'wrapperCache': False,

I wonder whether we can just automatically do this for the non-refcounted DOM object case... May be worth a followup.

::: dom/chrome-webidl/ChromeUtils.webidl:8
(Diff revision 2)
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/.
>   */
>  
> +[ChromeOnly, Exposed=(Window,System,Worker)]
> +interface MozQueryInterface {

This needs to be documented in terms of its semantics (e.g. what it returns).

::: dom/chrome-webidl/ChromeUtils.webidl:213
(Diff revision 2)
> +   * Returns an optimized QueryInterface method which, when called from
> +   * JavaScript, acts as an ordinary QueryInterface function call, and when
> +   * called from XPConnect, circumvents JSAPI entirely.
> +   */
> +  [Affects=Nothing, NewObject, Throws]
> +  MozQueryInterface generateQI(sequence<(DOMString or IID)> interfaces);

Should document a few things:

1)  What the DOMString bits of the sequence should be.
2)  That nsISupports is auto-added and must not be passed in.
3)  What happens with unrecognized strings.
4)  What happens with "unrecognized" IID instances.
Attachment #8970068 - Flags: review?(bzbarsky) → review+
Comment on attachment 8970068 [details]
Bug 1456035: Part 1 - Add helper to generate native QueryInterface callbacks.

https://reviewboard.mozilla.org/r/238824/#review245452

> Is there a reason this can't be sizeof(nsIID)?

It probably can. I meant to add a `static_assert(IID_SIZE <= sizeof(nsIID))`, but forgot before I pushed, but just defining it to sizeof(nsIID) might make more sense.

> Doing this means that mis-spelling an interface will just silently be ignored.  Why is that the behavior we want?  I guess that's what you get now if you hand-write your QI...
> 
> At the very least this continue needs some documentation explaining why this is the behavior.

I don't particularly like it, but it's the behavior we get now with XPCOMUtils.generateQI, and there's some code that depends on it for interfaces that only exist in some places (e.g., Windows).

I guess I could try removing it and make the callers that break explicitly check for interfaces that might not exist.

> Why do you need the IsReflector() check here?  If that fails, UnwrapReflectorToISupports will just return null and we'll throw anyway, right?  So the IsReflector() check is redundant.

I guess I don't.

> Ah, because it does an Equals(), then a LessThan()?
> 
> This is worth filing a followup bug on nsTArray.  It seems like we should be able to support having a comparator that provides a "Compare" operation returning a tristate, with fallback to the current setup if there is no "Compare".

Yeah, agreed. I'll file a follow-up for that.

> Do we really care about that case?  That is, can we just declare the legacycaller to return "any" and return thisv, whatever it is, if the IID matches.  One fewer compare in the common case, and if someone pulls a QI method off an object and calls it with a bogus "this".... well, it's no different from what happens right now with "return this" in XPCOMUtils_QueryInterface.

I guess it can. We don't actually call this method from the C++ side, so it only matters for JS callers anyway. But returning a non-object from QueryInterface is technically invalid...

> This needs to be documented in terms of its semantics (e.g. what it returns).

Will document here and in the header.
I'm going to have to land this in two parts so that android hostutils can be updated again. :(

I'll land the first part with a check in XPCOMUtils.generateQI to use ChromeUtils.generateQI if it exists and fall back to the old method if it doesn't, then back that out when I can land the rest.
Keywords: leave-open
> and there's some code that depends on it for interfaces that only exist in some places

OK.  Just document the behavior clearly, then.
Depends on: 1457012
https://hg.mozilla.org/integration/mozilla-inbound/rev/a442c0b9dbbe4266c14928aad8e04b9b08e0429b
Bug 1456035: Part 1 - Add helper to generate native QueryInterface callbacks. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/0102a61e38ae9c52d35b8239ccb24f48718a2b5b
Bug 1456035: Part 2 - Add fast path for XPCWrappedJS QueryInterface with native helper. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/affc06c38acd82a0773dd09465c2602e636fc08e
Bug 1456035: Part 3 - Replace XPCOMUtils.generateQI with a stub for ChromeUtils.generateQI. r=mccr8

https://hg.mozilla.org/integration/mozilla-inbound/rev/2add2de30c229733a3d0f11843695fcda818fe94
Bug 1456035: Part 3.1 - Add temporary fallback XPCOMUtils.generateQI implementation for Android hostutils. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/63241afeab2051961234964ef091add0bde387cc
Bug 1456035: Follow-up: Delete redundant test_extensionURL.html, which fails with wrapper errors. r=me f=aswan CLOSED TREE
Kris, what do we need to port here? Part 4 and 5? When will the original code stop working?
Just part 4. Part 5 is mostly just an optimization.

XPCOMUtils.generateQI will keep working until at least 64, but if you have any calls with nsISupports in the interface list, they'll assert in debug builds.
Just noticed :-( - Bug 1457713.
See Also: → 1457728
Comment on attachment 8970068 [details]
Bug 1456035: Part 1 - Add helper to generate native QueryInterface callbacks.

https://reviewboard.mozilla.org/r/238824/#review245452

> Yeah, agreed. I'll file a follow-up for that.

Filed bug 1457728.
Commit pushed to master at https://github.com/mozilla/activity-stream

https://github.com/mozilla/activity-stream/commit/4cfb63564d2616726e67d63a761f529ceed767d7
chore(mc): Port Bug 1456035: Part 4 - Convert callers of XPCOMUtils.generateQI to ChromeUtils.generateQI. r=mccr8 (#4131)
Depends on: 1458723
Blocks: 1460092
Is this work pretty much wrapped? Can we remove the leave-open and close this out yet?
Flags: needinfo?(kmaglione+bmo)
Oh, yeah. Sorry.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(kmaglione+bmo)
Keywords: leave-open
Resolution: --- → FIXED
No worries! Thanks for your work here!
Performance Impact: --- → P1
Whiteboard: [qf:p1:f64]
You need to log in before you can comment on or make changes to this bug.