Optimize XPCOMUtils.generateQI

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)

Comment 2

2 years ago
mozreview-review
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?
Attachment #8888594 - Flags: review?(florian) → review+
(Assignee)

Comment 3

2 years ago
mozreview-review-reply
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.
(Assignee)

Comment 4

2 years ago
mozreview-review
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.
(Assignee)

Updated

2 years ago
See Also: → 1383298
https://hg.mozilla.org/mozilla-central/rev/2513f47a01ea
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.