Closed Bug 1382904 Opened 7 years ago Closed 7 years ago

Optimize XPCOMUtils.generateQI

Categories

(Core :: XPConnect, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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 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+
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 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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: