Kill 0-arg .getService()

VERIFIED WONTFIX

Status

()

Core
XPConnect
VERIFIED WONTFIX
17 years ago
17 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: David Bradley)

Tracking

({perf})

Trunk
mozilla0.9.9
x86
Windows 95
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

17 years ago
I've decided that I've contributed so much to Mozilla that it's time to take
something away so I've set my sights on such marvels as this example taken from
charsetOverlay.js:
Components.classes['@mozilla.org/rdf/datasource;1?name=charset-menu'].getService().QueryInterface(Components.interfaces.nsICurrentCharsetListener);
which I think would be more efficient as follows:
Components.classes['@mozilla.org/rdf/datasource;1?name=charset-menu'].getService(Components.interfaces.nsICurrentCharsetListener);
i.e. s/\.getService()\.QueryInterface(/.getService(/g
Sadly LXR won't search on .getService() but here's a few cases for starters.
(Reporter)

Comment 1

17 years ago
Created attachment 68303 [details] [diff] [review]
Sample changes

Created using PatchMaker.
(Reporter)

Updated

17 years ago
Keywords: patch, perf, review
(Reporter)

Comment 2

17 years ago
Ideally this LXR search should have no matches.

Comment 3

17 years ago
jband, care to look at this?

Comment 4

17 years ago
We should do a grep-find through a local tree to see if there are any more
places that this optimization can be applied.

Updated

17 years ago
Target Milestone: --- → mozilla0.9.9
(Reporter)

Comment 5

17 years ago
Created attachment 68555 [details] [diff] [review]
More examples
Attachment #68303 - Attachment is obsolete: true

Comment 6

17 years ago
A few thoughts...

- I doubt that the efficiency difference is great (though I'm sure there is
*some* difference).
- Note that zero-arg getService is not inherently bad. As use of nsIClassInfo
spreads though the codebase then the QI and the explicit mention of an interface
become unnecessary in the cases of services with classinfo declaring the desired
interface.
- If you're in the mood for a jihad then you might consider replacing the
pattern of checking for null the result of createInstance and getService with
appropriate try/catch blocks - those methods *never* return null, they throw an
exception on error.
- On the (even more) pessimistic downside, one or two mistakes in your
conversions (and perhaps the cost of getting the changes properly reviewed) can
cause more cost to the project than the savings of some explicit QI calls. I
think it is *far* more interesting if you can identify instances where the same
pattern executes *many* times at runtime. And, in those cases, it is likely that
caching the service reference in the JS code - rather than getting it over and
over - would be the better fix anyway.
(Reporter)

Comment 7

17 years ago
> Note that zero-arg getService is not inherently bad. As use of nsIClassInfo
> spreads though the codebase then the QI and the explicit mention of an interface
> become unnecessary in the cases of services with classinfo declaring the desired
> interface.

And what about those services with more than one interface? e.g.
Components.classes["@mozilla.org/preferences-service;1"]
          .QueryInterface(Components.interfaces.nsIClassInfo)
          .getInterfaces([])
Oh wait, that use QI which I was trying to avoid anyway :-(
(Reporter)

Comment 8

17 years ago
Created attachment 68850 [details] [diff] [review]
98 examples

I've also taken on board the comments about changing if to try/catch.
Attachment #68555 - Attachment is obsolete: true

Comment 9

17 years ago
neil: xpconnect (for the past 9 months) does what is called "interface
flattening". This means that on each wrapped object it automatically exposes all
of the methods and constants of all of the interfaces that it knows that object
implements. It can discover which interfaces those are by either: nsIClassInfo
(if the object implments it), any successful QI calls made on the object, and
any cases where the object is passed to some other method *as* some particular
interface. So, as I said, as more services expose nsIClassInfo, less QI calls on
services - or interfaceID params passed to getService - will be *required*.

I'm not happy to try to discourage you from your goal, but it is important at
this stage of the project to focus effort on the things that need fixing and to
leave the stuff that works - even if sloppily - alone. Regressions due to small
mistakes in code cleanup are expensive. I don't think we want to risk paying
that cost useless the payoff can be shown to be significant. And I don't see
that here.

I don't know how many good or bad changes your patch has so far. But, I'm sad to
have to point out that the very first block in the patch would lead to a runtime
error:

 var prefs = Components.classes["@mozilla.org/preferences;1"];
-            if (prefs) {
-              prefs = prefs.getService();
-              if (prefs)
-                prefs = prefs.QueryInterface(Components.interfaces.nsIPref);
-            }
-            if (prefs) {
-              var url = prefs.CopyCharPref("browser.chromeURL");
-              if (url)
-                return url;
-            }
+                                  .getService(Components.interfaces.nsIPref);

...becomes...

 var prefs = Components.classes["@mozilla.org/preferences;1"];
                                  .getService(Components.interfaces.nsIPref);

...note the semi-colon at the end of the first line.

I realize that this is only one case and easily fixed. I'm just using it as an
illustration of the trade off between risk and gain.
(Reporter)

Comment 10

17 years ago
I understand your point about risk. What I don't understand is how I can call
getBranch on the prefs service without a QI.

Components.classes["@mozilla.org/preferences-service;1"].QueryInterface(Components.interfaces.nsIClassInfo)
> [xpconnect wrapped nsIClassInfo]
Components.classes["@mozilla.org/preferences-service;1"].getService()
> [xpconnect wrapped nsISupports]
Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefService)
> [xpconnect wrapped nsIPrefService]
Components.classes["@mozilla.org/preferences-service;1"].getService()
> [xpconnect wrapped nsIPrefService]

Fixing the first call to getService() will make this bug invalid.

Comment 11

17 years ago
dougt sez xpconnect?
Assignee: dougt → dbradley
Component: XPCOM → XPConnect
QA Contact: scc → pschwartau
(Assignee)

Comment 12

17 years ago
I find a total of 197 instances of this. So this patch still has a ways to go to
be complete.

Consider also that
Components.classes['@mozilla.org/rdf/datasource;1?name=charset-menu'].getService(Components.interfaces.nsICurrentCharsetListener);
is only one pattern. I found other patterns, that break this out into multiple
statements. So for many of these cases there will be some additional work to
make sure that these intermediate variables aren't reused later in the code.

I guess I'm more than a little leary of removing such a widely used element,
even if inefficient. I'm thinking of outstanding patches waiting to land for 0.9.9.

I'd like to see what the real gain is before going down this path.

We could depricate this, adding a warning, but I suspect with 197 cases it could
turn out to be a very noisy warning.

Comment 13

17 years ago
No, not passing a param to getService is fine. Nothing to be deprecated or
warned about. The pattern cited is only slightly inefficient. There are better
things to worry about. If you find actual broken code then that's a differnt thing.

FWIW, As things stand now there are very few factory constructed objects that
implement nsIClassInfo. The DOM objects all do - and that is why no one needs to
QI DOM objects in JS code. And the new SOAP stuff also implements the interface.

At some point we may go through and add that support to lots of classes. 

If you want to see this in action then look at the implementation and module
declaration for nsIXPCTestCallJS.

Here is an example use...

js> foo = Components.classes["@mozilla.org/js/xpc/test/CallJS;1"].getService();
js> foo
[object xpcTestCallJS @ 0x15230a8]
js> for(n in foo) dump(n+"\n");
QueryInterface
CallMethodNoArgs
Evaluate
EvaluateAndReturnError
SetJSObject
EvaluateAndEatErrors
(Assignee)

Comment 14

17 years ago
Then I don't see why this ended up in XPConnect? Sure the alternative pattern
may be more efficient, but changing the instances to use a new pattern doesn't
require a change XPConnect right?

Is there a better component to put this under. It's not really any specific
component, so is this browser-general or something else?
(Reporter)

Comment 15

17 years ago
Resolved as per various comments.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → WONTFIX

Comment 16

17 years ago
Neil, show that your changes have a measurable performance increase.  If it
does, not one of us cc'ed here can seriously argue that this patch has a risk
that is greater than a performance win. 

If you need help measuring the performance delta's, Mozilla has some tools that
will help:

http://www.mozilla.org/performance/startup-perf-brownbag.html

Comment 17

17 years ago
In the meantime, marking Verified for now - 
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.