1382904 - Optimize XPCOMUtils.generateQI

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement)

Description

[Kris Maglione [:kmag]]

We call XPCOMUtils.generateQI *a lot* during startup, and its implementation relies on calling the stringification slow path for nsIJSIDs twice for every interface. That adds up fast.

This patch reduces the time spent in generateQI at startup on my machine from ~7ms to ~2.8ms.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1382904: Optimize XPCOMUtils.generateQI.

PCOMUtils.generateQI is called a lot, and the current implementation is
especially inefficient. It relies on calling the stringification slow path
twice for each interface ID, which begins to add up fast given how often it's
called.

Review commit: https://reviewboard.mozilla.org/r/159580/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159580/

Comment 2

[Florian Quèze [:florian]]

Comment on attachment 8888594 [details]
Bug 1382904: Optimize XPCOMUtils.generateQI.

https://reviewboard.mozilla.org/r/159580/#review165184

r+ because it looks like an improvement, but I wonder if we could do something even faster by changing makeQI to take an array of interfaces instead of an array of interface names.

When searching the code base, it seems there are very few callers that provide an array of strings, so maybe we should drop support for that at some point? After 57 I guess as add-ons could use it.

::: js/xpconnect/loader/XPCOMUtils.jsm:114
(Diff revision 1)
>      /* Note that Ci[Ci.x] == Ci.x for all x */
>      let a = [];
>      if (interfaces) {
>        for (let i = 0; i < interfaces.length; i++) {
>          let iface = interfaces[i];
> -        if (Ci[iface]) {
> +        let name = iface.name || String(iface);

Is the 'String()' here needed or only to make the type explicit for the next reader?

::: js/xpconnect/loader/XPCOMUtils.jsm:115
(Diff revision 1)
>      let a = [];
>      if (interfaces) {
>        for (let i = 0; i < interfaces.length; i++) {
>          let iface = interfaces[i];
> -        if (Ci[iface]) {
> -          a.push(Ci[iface].name);
> +        let name = iface.name || String(iface);
> +        if (name in Ci) {

Should this be Ci.hasOwnProperty(name) so that it's false when name is something from the default object prototype?

Comment 3

[Kris Maglione [:kmag]]

Comment on attachment 8888594 [details]
Bug 1382904: Optimize XPCOMUtils.generateQI.

https://reviewboard.mozilla.org/r/159580/#review165184

I considered that, but this was changed a long time ago to store strings rather than nsIJSIDs in order to avoid leaks, and I'd like to avoid the risk of regressing that for the moment. I agree we can probably take a more aggressive approach after 57.

> Is the 'String()' here needed or only to make the type explicit for the next reader?

It's mainly just to avoid the possibility of add-ons doing something weird, and us winding up storing a non-string reference that causes performance issues, or other weird problems.

> Should this be Ci.hasOwnProperty(name) so that it's false when name is something from the default object prototype?

I tried that initially, but it turned out to be much slower. I'm not all that worried about someone passing in a string that's a name of some non-interface property, so it doesn't seem worth the extra overhead.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8888594 [details]
Bug 1382904: Optimize XPCOMUtils.generateQI.

https://reviewboard.mozilla.org/r/159580/#review165328

::: js/xpconnect/loader/XPCOMUtils.jsm:114
(Diff revision 1)
>      /* Note that Ci[Ci.x] == Ci.x for all x */
>      let a = [];
>      if (interfaces) {
>        for (let i = 0; i < interfaces.length; i++) {
>          let iface = interfaces[i];
> -        if (Ci[iface]) {
> +        let name = iface.name || String(iface);

Heh. It looks like we have a bunch of code that relies on being able to pass `Ci.nsISomeInterfaceThatNoLongerExists` here. So there's another thing to fix after 57.

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2513f47a01eae7797edcea19ca09036cecce02c1
Bug 1382904: Optimize XPCOMUtils.generateQI. r=florian

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2513f47a01ea