We have a bunch of services that are used all over the place that we should have an easy way to access. Candidates include the IO service, Prefs service, XPConnect, and probably a bunch of others. This can go into xpcom, even if the services themselves aren't implemented in there, because they will be obtained the first time via the normal do_GetService call and can just forward-declare the class pointer. Suggested API is CommonServices.h, mozilla::services namespace, and then mozilla::services::GetIOService() mozilla::services::GetPrefService() mozilla::services::GetXPConnectService() so that they can be imported via 'using mozilla::services;' and then just used via GetIOService()->Stuff(); taras suggested that in addition to this, we can pork-rewrite current getService users to use the new getters.

(observer service, uuid generator, script security manager, xpconnect context stack, etc.) Would be helpful to get a list of services requested and counts for startup and a full run through tp4.

(In reply to comment #1) > Would be helpful to get a list of services requested and counts for startup Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20090911 Minefield/3.7a1pre Contract IDs passed to nsComponentManagerImpl::GetServiceByContractID more than once (full data including singles at [1]): count % service 1471 38.73 @mozilla.org/network/io-service;1 440 11.59 @mozilla.org/xpti/interfaceinfomanager-service;1 263 6.92 @mozilla.org/chrome/chrome-registry;1 205 5.40 @mozilla.org/charset-converter-manager;1 168 4.42 @mozilla.org/preferences-service;1 139 3.66 @mozilla.org/js/xpc/ContextStack;1 129 3.40 @mozilla.org/fast-load-service;1 107 2.82 @mozilla.org/observer-service;1 99 2.61 @mozilla.org/xbl;1 95 2.50 @mozilla.org/scriptsecuritymanager;1 75 1.97 @mozilla.org/uuid-generator;1 71 1.87 @mozilla.org/js/xpc/XPConnect;1 66 1.74 @mozilla.org/intl/charsetalias;1 43 1.13 @mozilla.org/network/stream-transport-service;1 30 0.79 @mozilla.org/file/directory_service;1 28 0.74 @mozilla.org/categorymanager;1 27 0.71 @mozilla.org/moz/jsloader;1 26 0.68 @mozilla.org/network/protocol;1?name=about 24 0.63 @mozilla.org/intl/stringbundle;1 21 0.55 @mozilla.org/network/protocol;1?name=default 20 0.53 @mozilla.org/network/protocol;1?name=rdf 16 0.42 @mozilla.org/network/protocol;1?name=moz-safe-about 15 0.39 @mozilla.org/xre/runtime;1 12 0.32 @mozilla.org/docloaderservice;1 11 0.29 @mozilla.org/content/document-loader-factory;1 11 0.29 @mozilla.org/network/protocol/about;1?what=blank 10 0.26 @mozilla.org/network/protocol;1?name=data 10 0.26 @mozilla.org/xre/app-info;1 8 0.21 @mozilla.org/gfx/screenmanager;1 8 0.21 @mozilla.org/xpcom/version-comparator;1 6 0.16 @mozilla.org/intl/nslanguageatomservice;1 6 0.16 @mozilla.org/webnavigation-info;1 6 0.16 @mozilla.org/mime;1 6 0.16 @mozilla.org/appshell/window-mediator;1 6 0.16 @mozilla.org/layout/content-policy;1 5 0.13 @mozilla.org/content/style-sheet-service;1 5 0.13 @mozilla.org/widget/dragservice;1 5 0.13 @mozilla.org/js/xpc/RuntimeService;1 5 0.13 @mozilla.org/uriloader;1 4 0.11 @mozilla.org/chrome/chrome-native-theme;1 3 0.08 @mozilla.org/appshell/appShellService;1 3 0.08 @mozilla.org/browser/global-history;2 3 0.08 @mozilla.org/network/idn-service;1 3 0.08 @mozilla.org/rdf/datasource;1?name=window-mediator 3 0.08 @mozilla.org/storage/service;1 3 0.08 @mozilla.org/docshell/urifixup;1 3 0.08 @mozilla.org/rdf/rdf-service;1 2 0.05 @mozilla.org/network/socket-transport-service;1 2 0.05 @mozilla.org/extensions/manager;1 2 0.05 @mozilla.org/intl/unicharutil;1 2 0.05 @mozilla.org/intl/nslocaleservice;1 2 0.05 @mozilla.org/exceptionservice;1 2 0.05 @mozilla.org/embedcomp/window-watcher;1 2 0.05 @mozilla.org/toolkit/app-startup;1 2 0.05 @mozilla.org/browser/clh;1 2 0.05 @mozilla.org/rdf/datasource;1?name=local-store 2 0.05 @mozilla.org/image/loader;1 2 0.05 @mozilla.org/privatebrowsing-wrapper;1 2 0.05 @mozilla.org/privatebrowsing;1 2 0.05 @mozilla.org/xpcom/error-service;1 [1] https://wiki.mozilla.org/Firefox/Projects/Startup_Time_Improvements/adw_notes#September_11.2C_2009

Note that some places do local coalescing of these already (e.g. via nsContentUtils); we would presumably be able to get rid of those, right?

Yep; I'd like to unify all of those. It'd be interesting to see the above getService-capture for a Tp run as well.

I filed bug 512784 which is similar though that was focused on easy access from DOM windows/JS.

Top 20 contract IDs passed to nsComponentManagerImpl::GetServiceByContractID during one cycle of Tp4 (full data and CIDs passed to nsComponentManagerImpl::GetService at [1]): count % contract_id 95079 47.31 @mozilla.org/network/io-service;1 16468 8.19 @mozilla.org/intl/stringbundle;1 12862 6.40 @mozilla.org/preferences-service;1 10478 5.21 @mozilla.org/scriptsecuritymanager;1 8273 4.12 @mozilla.org/network/cache-service;1 8255 4.11 @mozilla.org/js/xpc/ContextStack;1 8113 4.04 @mozilla.org/network/http-activity-distributor;1 7697 3.83 @mozilla.org/xpti/interfaceinfomanager-service;1 6145 3.06 @mozilla.org/observer-service;1 2095 1.04 @mozilla.org/cookieService;1 1941 0.97 @mozilla.org/network/mime-hdrparam;1 1798 0.89 @mozilla.org/intl/charsetalias;1 1744 0.87 @mozilla.org/docshell/urifixup;1 1624 0.81 @mozilla.org/plugin/host;1 1445 0.72 @mozilla.org/uriclassifierservice 1445 0.72 @mozilla.org/url-classifier/utils;1 1074 0.53 @mozilla.org/charset-converter-manager;1 1052 0.52 @mozilla.org/network/protocol;1?name=http 1031 0.51 @mozilla.org/network/protocol;1?name=javascript 1009 0.50 @mozilla.org/uuid-generator;1 [1] https://wiki.mozilla.org/Firefox/Projects/Startup_Time_Improvements/adw_notes#September_21.2C_2009

Oh my. We should do this ASAP, I think.

I would go out on a limb and assume that a huge percentage of the IOService callers are trying to create URIs. I know we have the NS_NewURI helper, but I only just noticed this: http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#126 It has a handy extra param that you can pass in to keep a copy of the IOService laying around. A brief survey of callers indicates that almost nobody uses that param though: http://mxr.mozilla.org/mozilla-central/ident?i=NS_NewURI

(In reply to comment #8) > I would go out on a limb and assume that a huge percentage of the IOService > callers are trying to create URIs. I know we have the NS_NewURI helper, but I I would need to rerun Tp4 to find out there, but I worked out that on startup 701 of the 1471 (48%) io-service gets are from NS_NewURI [1]. There are 64 total sites that call NS_NewURI: 3 pass in an io-service, 61 do not [2]. [1] https://wiki.mozilla.org/Firefox/Projects/Startup_Time_Improvements/adw_notes#September_15.2C_2009 [2] https://wiki.mozilla.org/Firefox/Projects/Startup_Time_Improvements/adw_notes#September_10.2C_2009

The CSSParserImpl codepath should be trivial to fix even without this bug: it can just use the nsContentUtils ioService.

I was going to say that we have do_GetIOService, but that just gets it by contract id. Sadfaces. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#102

It's a tossup as to whether we'd want to fix this by itself for io service, or just start adding services into some global framework, with io service being the first. With the io service, though, it'd be really nice if all of these consumers could avoid a branch when dealing with it -- that is, if we could initialize it once somewhere, and then have everything else refer to it just directly via a static pointer. For the other services, I was thinking of something like this: Services.h: namespace mozilla { namespace services { class nsIFooService; static nsIFooService *gFooServicePointer; void InitializeFooService(nsresult *rv); nsIFooService* GetFooService(nsresult *rv = nsnull) { if (!gFooServicePointer) InitializeFooService(rv); else if (rv) rv = NS_OK; return gFooServicePointer; } ... void ReleaseServices(); } } Services.cpp: static nsIFooService *gFooServicePointer = nsnull; // whatever the right way is to do this; yes, I'm proposing that // we duplicate the IID here NS_DECLARE_STATIC_IID(nsIFooService_iid, { 0xabcdefgh, .... }); /* a bit of pseudocode */ void InitializeFooService(nsresult *rv) { nsISupports *obj = do_GetService("foo-service;1"); NS_IF_ADDREF(obj); gFooServicePointer = QueryInterface(obj, nsIFooService_iid); } ... void ReleaseServices() { if (gFooServicePointer) { NS_RELEASE(gFooServicePointer); gFooServicePointer = nsnull; } } This gets us, in the common inlined rv=0 case, 1 compare, 1 branch and a global memory read to get a pointer to a service (because I'm assuming that the compiler will optimize out that else if (rv) if rv == 0, when inlined). Maybe that's enough even for the IO service?

Drew, can you do a Tp run with this? I doubt we'll see much on Ts

This is similar to Vlads, idea. This patch avoids adding new global variables, instead if moves the existing variable used by nsIOService::GetInstance() to where it can used by the rest of the program. I'm undecided as to whether we should use accessor methods to wrap these variables.On one hand it would be nice to ensure that things are initialized and well, but on the other hand if the variable is ever null it's a grave error, we should not be creating new services anyway. The services in this file should be explicitly inititialized early on in startup, so it's reasonable to assume they are not null

(In reply to comment #15) > The services in this file should be explicitly inititialized early on in > startup, so it's reasonable to assume they are not null If all consumers switch to using this variable, how can this be true? Doesn't this mandate that we need an accessor method?

(In reply to comment #16) > (In reply to comment #15) > > The services in this file should be explicitly inititialized early on in > > startup, so it's reasonable to assume they are not null > If all consumers switch to using this variable, how can this be true? Doesn't > this mandate that we need an accessor method? stick service init for popular services earlyon into XRE_main. As a side-effect, it'll be easier to spot-check service-initialization time for profiling

Comment on attachment 402162 [details] [diff] [review] proof of concept >+#include "nsServices.h" should probably be "mozilla/Services.h"

(In reply to comment #17) > stick service init for popular services earlyon into XRE_main. As a > side-effect, it'll be easier to spot-check service-initialization time for > profiling Then any patch better have a test to make sure this is true ;)

After this patch, we should probably get rid of net_EnsureIOService.

Oh, another question. If we do put the ioservice init in xre_main, could extensions and such still override that contract?

(In reply to comment #21) > Oh, another question. If we do put the ioservice init in xre_main, could > extensions and such still override that contract? I don't know. Do we seriously expect extensions to override ioservice?

I do; in fact I'm pretty surethere are some that do that right now. They tend to do something with the data they're given and then pass the call on to the real ioservice (some by CID, others by getting it via contract before registering their own contract).

(In reply to comment #23) > I do; in fact I'm pretty surethere are some that do that right now. They tend > to do something with the data they're given and then pass the call on to the > real ioservice (some by CID, others by getting it via contract before > registering their own contract). well that settles that then. We'll use a wrapper and lazy init it so we don't change semantics too much. Extensions would still have to make sure that they overwrite that global, but I think that's reasonable.

Do we really want to encourage that kind of insanity?

I do not think we want to support that behavior, and I'd be surprised if it worked at all, given the necko-internal access to nsIOService::gIOService.

> Do we really want to encourage that kind of insanity? If we want to drop XPCOM in general (with its whole mess of contracts etc), fine. Do we have alternative proposals for people trying to customize our code to their needs in relatively pain-free ways? Maybe the answers are "we want to do that" and "no", but I'd like us to consciously (and publicly, in the newsgroups, so those who use the functionality right now have a chance to explain their usecases, not in a bug no one is reading) make those decisions. Necko-internal access to gIOService is not for access to its public-facing API in my brief skim (e.g. newURI or newChannel calls). One thought I had last night. If we go the lazy init way and clear things whenever a new contractid is registered, how much would that cost us? Would it avoid breaking backwards compat here?

Do not go breaking APIs add-ons rely on to customize without interfering. If we break things too quickly without a plan B, we're removing our big advantage over the competition, and tending to drive developers away. It may be "insane" but it is an API. See Raymond Chen's blog. To get to better APIs we need to keep the old while designing the new, better APIs, then promoting those -- and only after they are solid and well-understood and seeing adoption, even think about removing the old. We could declare Mozilla 2 now, break all API compat. Without better APIs that are used (not just on paper or in a dream), that would be suicide. /be

Fair enough, we should figure out what extensions are actually doing this, and why, but it doesn't have to happen here.

(In reply to comment #27) > One thought I had last night. If we go the lazy init way and clear things > whenever a new contractid is registered, how much would that cost us? Would it > avoid breaking backwards compat here? I don't think we need to clear things, though we certainly could; even now, places cache things like the IO service so if it's overwritten they'll have the stale one. Though I guess the things that cache it do so sufficiently late in the process. Doesn't xpconnect enumerate all contract IDs on init? Are you worried about the case of someone registering something like the io service dynamically?

> Doesn't xpconnect enumerate all contract IDs on init? Not quite, as I recall.... > Are you worried about the case of someone registering something like the io > service dynamically? Not terribly; I'm worried about someone triggering init of the ioservice before component registration finishes.

(In reply to comment #14) > Drew, can you do a Tp run with this? Gets of io-service dropped from 95079 to 39402 (-56%).

(In reply to comment #32) > (In reply to comment #14) > > Drew, can you do a Tp run with this? > > Gets of io-service dropped from 95079 to 39402 (-56%). so no noticable change in Tp runtime?

(In reply to comment #32) > Gets of io-service dropped from 95079 to 39402 (-56%). Uh, it's -58.55867226201369% actually. (Next time I should just copy and paste.)

(In reply to comment #33) > so no noticable change in Tp runtime? No noticeable change in Tp4 runtime. Now, I just ran a single cycle with the patch and without, so I can run the full 20 to get more data. But on my machines each get of io-service is about 0.01ms on average, so a difference of 55677 gets is only just north of 0.5s.

Likely to have a larger impact on mobile, where XPCOM is way more expensive.

Ok, ran into a problem. If I try to add a method called GetIOService() to xpcom I can't figure out how to call some variant of GetService with an incomplete class. Is there some API similar to do_GetService that will work with an incomplete pointer? As an alternative I tried declaring GetIOService in xpcom and implementing that in netwerk/, but that runs into linking problems with there being lots ofusers of nsNetUtil.h who do not link against netwerk :(

I think you need to have the IID for the main interface, so that you can call QueryInterface.. but you want to call GetService on nsIServiceManager directly, and not do_GetService (which is the helper). You then just need to get the actual pointer from the nsQIResult and cast it to the appropriate forward-declared type (in this case nsIIOService) and return that.

Vlad, it looks like I need both CID and IID for this. Is this solution reasonable? Once we settle on a solution, we can start marking GetService calls as deprecated and switching code over.

Yep, looks great to me.

Comment on attachment 410303 [details] [diff] [review] prototype v2 > nsIOService* > nsIOService::GetInstance() { Preexisting nit: left brace on its own line. >- if (!gIOService) { >- gIOService = new nsIOService(); >- if (!gIOService) >+ if (!mozilla::services::gIOService) { >+ nsIOService* ioService = new nsIOService(); >+ if (!ioService) > return nsnull; >- NS_ADDREF(gIOService); >- >- nsresult rv = gIOService->Init(); >+ NS_ADDREF(ioService); >+ >+ // this assigns mozilla::services::gIOService >+ nsresult rv = ioService->Init(); > if (NS_FAILED(rv)) { >- NS_RELEASE(gIOService); >+ NS_RELEASE(ioService); > return nsnull; > } >- return gIOService; >+ } else { >+ NS_ADDREF(mozilla::services::gIOService); Hot path first (invert if condition and swap then with else clause)? >+++ b/xpcom/build/nsServices.cpp >@@ -0,0 +1,70 @@ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- >+ * vim: sw=4 ts=4 sts=4 et filetype=javascript tab-width: 2 is not consistent with the vim equivalent, or with c-basic-offset; and the rest of the file uses c-basic-offset: 2 so something is amiss here. >+++ b/xpcom/build/nsServices.h >@@ -0,0 +1,49 @@ >+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 4 -*- >+ * vim: sw=4 ts=4 sts=4 et filetype=javascript Ditto. >+class nsIIOService; >+namespace mozilla { >+ namespace services { >+ extern NS_COM nsIIOService* gIOService; >+ NS_COM nsIIOService* GetIIOService(); >+ } >+} Nit: 4- after 2-space indentation. /be

This is getting a little bloated. I think from now on the strategy should be to convert a few services at a time, doing them all is going to make for some massive matches. Turns out there are 2 kinds of services. Those that keep their own 'singleton' static variable and those that merely rely on the service manager to store a reference to them. So I implement 2 kinds of getters, one that steals the underlying singleton variable and one the merely "caches" the service. Since the code for getters is nearly identical, I'm implementing them in macros :(. nsNetUtil is a mess because it consists of popular inline functions that are accessed from internal/external mozilla code. Would appreciate any drive-by-comments before I start assigning r?es to people.

(In reply to comment #42) > Created an attachment (id=411577) [details] > complete category manager + ioservice api switchover > > This is getting a little bloated. I think from now on the strategy should be to > convert a few services at a time, doing them all is going to make for some > massive matches. I agree -- I'd suggest actually splitting out the stub creation and the actual implementation per service. > Turns out there are 2 kinds of services. Those that keep their own 'singleton' > static variable and those that merely rely on the service manager to store a > reference to them. > > So I implement 2 kinds of getters, one that steals the underlying singleton > variable and one the merely "caches" the service. Since the code for getters is > nearly identical, I'm implementing them in macros :(. nsNetUtil is a mess > because it consists of popular inline functions that are accessed from > internal/external mozilla code. Any reason to not just implement them all as caches? That would work fine with the first case, and would mean only one set of boilerplate code so someone adding services in the future wouldn't have to think about which style to use. Seems like you'd have fewer linkage issues that way, too. > Would appreciate any drive-by-comments before I start assigning r?es to people. One question -- the services we're moving here are essentially core/required, right? What do you think about making these getters infallible? It would save on a ton of if (!catman) { return ...; }

> I agree -- I'd suggest actually splitting out the stub creation and the actual > implementation per service. Indeed. > Any reason to not just implement them all as caches? That would work fine with > the first case, and would mean only one set of boilerplate code so someone > adding services in the future wouldn't have to think about which style to use. > Seems like you'd have fewer linkage issues that way, too. No good reason. Felt cleaner to centralize all of those global variables instead of leaving them scattered through the tree. Random code happens to use those, at least this way it'd be more clear as what those variables are doing. > One question -- the services we're moving here are essentially core/required, > right? What do you think about making these getters infallible? It would save > on a ton of if (!catman) { return ...; } Actually, I was wondering the same thing. If it was up to me I'd make them infallible as it makes life easier and if they are set to null then something very bad is going on (also looks like browser shutdown may cause getService to bail).

I forgot to mention that I did not make these getters return already_AddRefed<>. I don't see the benefit to ref counting singletons. I think we should allow non-shutdown code to call GetService()->foo() without having to check that service is non-null or deref via a nsCOMPtr. A silly optimization would be to enforce that nobody is allowed to store services on the heap via static analysis(ie they can't leak and don't need refcounting).

> nobody is allowed to store services on the heap Not enforceable as long as we reflect them into js via xpconnect, no?

Hm, that only matters because the service might "go away"? In that case something can just keep a strong ref to it, right? That is, it should store a nsCOMPtr, and not a bare pointer. For centralizing the random pointers... I'd much rather fix code that uses the pointers directly to use this new method, since it'd be cleaner and more maintainable, right? Then we can do what we want with the internal stuff (and at some point should probably change the services that just return a singleton to assert in their constructor that they are the only one and trust the service manager... anyone who misuses them with CreateInstance should get fixed).

(In reply to comment #47) > Hm, that only matters because the service might "go away"? In that case > something can just keep a strong ref to it, right? That is, it should store a > nsCOMPtr, and not a bare pointer. I was thinking of disallowing services leaking onto heap purely a space optimization for old code. Now that getting services costs a function call, there is absolutely no point in keeping copies in globals/class-members. > > For centralizing the random pointers... I'd much rather fix code that uses the > pointers directly to use this new method, since it'd be cleaner and more > maintainable, right? Then we can do what we want with the internal stuff (and > at some point should probably change the services that just return a singleton > to assert in their constructor that they are the only one and trust the service > manager... anyone who misuses them with CreateInstance should get fixed). ok. Sounds reasonable.

Comment on attachment 411761 [details] [diff] [review] service getters Just doing a drive-by you asked for... >@@ -117,6 +118,7 @@ EXPORTS_NAMESPACES = mozilla > SDK_HEADERS = \ > nsXPCOM.h \ > nsXPCOMCID.h \ >+ nsServices.h \ Why don't we call this Services.h, and place it in the mozilla namespace? Then folks just #include "mozilla/Services.h". Obviously, you should change the cpp filename too. >diff --git a/xpcom/build/nsServices.cpp b/xpcom/build/nsServices.cpp >+ * The Initial Developer of the Original Code is >+ * Mozilla Corporation. Mozilla Foundation >+ * Portions created by the Initial Developer are Copyright (C) 2009 >+ * the Initial Developer. All Rights Reserved. >+ * Missing the spot for contributors here, and you should add yourself. Everything after the #includes should be in the mozilla namespace, right? >+// Set to true from NS_ShutdownXPCOM. >+extern PRBool gXPCOMShuttingDown; Why not just use bool here? >+const nsCID kIOServiceCID = >+ {0x9ac9e770, 0x18bc, 0x11d3, {0x93, 0x37, 0x00, 0x10, 0x4b, 0xa0, 0xfd, 0x40}}; Your indenting is different for a bunch of these. Pick one, and make them all use it please? >+++ b/xpcom/build/nsServices.h ditto on the license comments >+#ifndef SERVICES_H__ >+#define SERVICES_H__ bsmedberg has had me do mozilla_Services_h__ since this would be mozilla/Services.h. It has the benefit of being more readable and less shouting. >+namespace mozilla { >+ namespace services { >+ GET_SERVICE_DECL_GETTER(IOService) >+ GET_SERVICE_DECL_GETTER(DirectoryService) >+ GET_SERVICE_DECL_GETTER(CategoryManager) >+ GET_SERVICE_DECL_GETTER(ChromeRegistry) >+ GET_SERVICE_DECL_GETTER(ObserverService) >+ } >+} AFAIK, we don't usually indent for namespaces. It's also really handy to place a comment stating which namespace you are closing: } // namespace services } // namespace mozilla This way, when we add more functions in there, the braces are clear as to what they are for.

Got rid of macros in header, namespaced the file, etc.

The functions in this file are heavily used from core and non-core code.

Comment on attachment 411822 [details] [diff] [review] inline function mods in nsnetutil.h going to wait on more guidance from bsmedberg before proceeding further. Working on unrelated bits of bug 512584 in meantime.

Comment on attachment 411822 [details] [diff] [review] inline function mods in nsnetutil.h Why not have the internal do_GetIOService use the same magic that do_QueryInterface does for the return type, so that you don't have to cast it to a separate COMPtr?

The 2 main differences in this patch are a) There is no more IID/CID hardcoding due to dependency on unified build tiers b) There is now a shutdownobserver for wiping cached services c) alreadyAddrefed things are returned and refcount is adjusted to account for the cached variable. This is much messier than I'd like, but I'm not quite sure as to how to clean it up. Suggestions are welcome This passes all xpcshell tests, but mochi-* tests T-FAIL on try server. I'm guessing that the try server isn't conspiring against me and something is indeed going wrong, but I haven't yet figured out which tests are being screwed up(every failing test i checked so far on my machine seems to continue to fail when patch is removed).

(In reply to comment #55) > This passes all xpcshell tests, but mochi-* tests T-FAIL on try server. Correction this patch itself does nothing and doesn't fail. The forthcoming nsNetUtil patch activates this and likely is the cause of the failures.

A bunch of my comments in comment 50 aren't addressed in this patch. Did you choose not to implement some of them (and could I get an explanation as to why please?)?

The main annoyance here is hacky do_GetNetUtil that I added to avoid bloating everything that used to QI on do_GetIOService.

Shawn, I somehow uploaded an ancient patch. Sorry about that.

Comment on attachment 416190 [details] [diff] [review] I think this is what we want >+#define CLEAN_GLOBAL(GLOBAL) \ >+ if (GLOBAL) { \ >+ GLOBAL->Release(); \ >+ GLOBAL = nsnull; \ >+ } \ This is nearly identical to NS_IF_RELEASE. I think you just want to use that instead. http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsUtils.h#165 >+NS_IMETHODIMP >+ShutdownObserver::Observe(nsISupports*, const char*, const PRUnichar *) >+{ Might be a good idea to assert you are getting the expected topic just in case this gets registered for something else down the line. >+ ShutdownObserver::gShutdownObserver->Release(); >+ ShutdownObserver::gShutdownObserver = nsnull; This is equivalent to NS_RELEASE(ShutdownObserver::gShutdownObserver);

Comment on attachment 416190 [details] [diff] [review] I think this is what we want >diff --git a/xpcom/build/Services.cpp b/xpcom/build/Services.cpp >+// Set to true from NS_ShutdownXPCOM. >+extern PRBool gXPCOMShuttingDown; Hrm... can we put a declaration of this in a header file? nsXPCOMPrivate.h seems like a good fit. >+class ShutdownObserver : public nsIObserver { >+ NS_DECL_ISUPPORTS >+ NS_DECL_NSIOBSERVER >+ >+ static void EnsureObserver(); >+ static ShutdownObserver *gShutdownObserver; >+}; We don't need XPCOM for this... just make a helper function and call that from the shutdown path in NS_ShutdownXPCOM. >+// this one just caches result of GetService >+#define GET_CACHED_SERVICE_IMPL(TYPE, SERVICE_CID) \ 1. You overload TYPE for nsI##TYPE and Get##TYPE. I'd slightly prefer a 3-argument macro: GET_CACHED_SERVICE_IMPL(ServiceName, InterfaceName, ServiceContractID); 2. Use contractIDs, not CIDs >+ nsComponentManagerImpl::gComponentManager->GetService(cid, \ >+ nsI##TYPE::GetIID(),\ Use NS_GET_IID(InterfaceName) >+GET_CACHED_SERVICE_IMPL(IOService, NS_IOSERVICE_CID) Hrm, it seems that this list of cached services will grow over time, and we're going to have to keep this bit, the CLEAN_GLOBAL list, and the list in the header in sync. Why not have an include file with this list and use it in all three places? >+static nsIObserverService* gObserverService = 0; >+NS_COM already_AddRefed<nsIObserverService> mozilla::services::GetObserverService() { Why is this written out manually instead of using GET_CACHED_SERVICE_IMPL? >+ if (gObserverService) >+ gObserverService->AddRef(); Definitely use NS_IF_ADDREF(...) here. >diff --git a/xpcom/build/Services.h b/xpcom/build/Services.h >+#ifndef mozilla_Services_h__ >+#define mozilla_Services_h__ no trailing underscores please >+class nsIDirectoryService; >+class nsICategoryManager; >+class nsIChromeRegistry; >+class nsIObserverService; Some of these are forward-declared in preparation for the future? You don't use them currently.

Once I get review on this, I'd like to replace ioservice mozilla-wide and then commit both changes together(no use landing this without any major users). After that other services can be added on incrementally. Benjamin, I addressed your review comments, but prefer to stick to a single macro parameter that specifies both name&type of getter. If the need arises for the two to diverge, that's an easy change to do later. As a reminder, this patch replaces the horrendously slow GetService with an API that's basically free. Getting the IOService for things like making urls is the biggest user of the call.

Comment on attachment 432843 [details] [diff] [review] fast service getters Please rename _MOZ_SERVICE: identifiers that start with underscores are reserved for the compiler or OS. I'd prefer MOZ_SERVICE to take three parameters so that we don't have to hard code nsI##Foo. Some of the newer services use mozIFoo or even just IFoo. I don't think the gXPCOMShuttingDown check in the getter is useful: it duplicates a check in nsComponentManagerImpl::GetService. mozilla::services::shutdown should be in a private header, e.g. nsXPCOMPrivate, not the public mozilla/Services.h header. And it deserves a file-scope doccomment.

(In reply to comment #64) > (From update of attachment 432843 [details] [diff] [review]) > Please rename _MOZ_SERVICE: identifiers that start with underscores are > reserved for the compiler or OS. Fixed. > I'd prefer MOZ_SERVICE to take three parameters so that we don't have to hard > code nsI##Foo. Some of the newer services use mozIFoo or even just IFoo. Good reason. Fixed. > > I don't think the gXPCOMShuttingDown check in the getter is useful: it > duplicates a check in nsComponentManagerImpl::GetService. You are right. That's a holdover from an earlier revision. > > mozilla::services::shutdown should be in a private header, e.g. nsXPCOMPrivate, > not the public mozilla/Services.h header. And it deserves a file-scope > doccomment. Done.

This is the least intrusive/ugly patch I could come up with to get rid of the slowpath IOService getting. Problem is that traditionally nsCOMPtr is what triggers the actual getService() call, One can assign the traditional do_GetIOService() to any nsCOMPtr and the equivalent to qi magic(in do_GetNetUtil) in this patch would happen implicitly.

Comment on attachment 436755 [details] [diff] [review] switch nsNetUtil.h to an efficient IOService getter + // This seems far too convoluted, there has to be a reasonable way to avoid the extra addref Well, XPCOM doesn't guarantee that the two interfaces will be implemented by the same object (i.e. in principle nsINetUtil could be implemented by a tearoff). So I don't think there's a way to avoid the addref without using dynamic_cast. A semi-reasonable way might be to offer a QueryIOServiceInterface function or somesuch on mozilla::services, to which you could pass an IID. What's the reason for returning an already_AddRefed from mozilla::services though? Your comment 55 doesn't say why you made that change. Is this to force people not to store raw pointers to these services? + nsCOMPtr<nsINetUtil> netUtil = do_QueryInterface(io); + ret = netUtil.forget(); Could replace this with: CallQueryInterface(io, &ret.mRawPtr); but I'm not sure that that's better... + // Could just call do_GetIOService(error) in assignment of 'io' above to simplify this function, right? I'd prefer that you didn't check this comment in. Either make that change, or remove that comment. Seems like the change is safe to make as long as the QueryInterface doesn't fail, which it shouldn't.

(In reply to comment #67) > > A semi-reasonable way might be to offer a QueryIOServiceInterface function or > somesuch on mozilla::services, to which you could pass an IID. I'll consider something like that if I run into this repeatedly. > What's the reason for returning an already_AddRefed from mozilla::services > though? Your comment 55 doesn't say why you made that change. Is this to force > people not to store raw pointers to these services? Yes. Raw pointers would be even more efficient, but they cause null-dereferences on shutdown, etc. > > + nsCOMPtr<nsINetUtil> netUtil = do_QueryInterface(io); > + ret = netUtil.forget(); > > Could replace this with: > CallQueryInterface(io, &ret.mRawPtr); > > but I'm not sure that that's better... Good idea. > > + // Could just call do_GetIOService(error) in assignment of 'io' above to > simplify this function, right? > > I'd prefer that you didn't check this comment in. Either make that change, or > remove that comment. Seems like the change is safe to make as long as the > QueryInterface doesn't fail, which it shouldn't. Got rid of the comment.

(In reply to comment #68) > Yes. Raw pointers would be even more efficient, but they cause > null-dereferences on shutdown, etc. Same for already_AddRefed... so your point is that this way forces people not to write mozilla:::services::GetIOService()->NewURI()?

Comment on attachment 437053 [details] [diff] [review] switch nsNetUtil.h to an efficient IOService getter in any case, this patch is fine.

http://hg.mozilla.org/mozilla-central/rev/9737e32c8662 http://hg.mozilla.org/mozilla-central/rev/695bd6dde693

I propose documenting this here: https://developer.mozilla.org/en/XPCOM/Getting_references_to_services And then updating these pages to link to it: https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIServiceManager/getService https://developer.mozilla.org/en/JavaScript_code_modules/Services.jsm

Comment on attachment 437053 [details] [diff] [review] switch nsNetUtil.h to an efficient IOService getter >+ already_AddRefed<nsINetUtil> ret = nsnull; >+ if (io) >+ CallQueryInterface(io, &ret.mRawPtr); >+ >+ if (error) >+ *error = ret.get() ? NS_OK : NS_ERROR_FAILURE; >+ return ret; Named return value optimisation might help you out here but you could have simply used a raw nsINetUtil* pointer.

(In reply to comment #73) > Named return value optimisation might help you out here but you could have > simply used a raw nsINetUtil* pointer. This isn't in any hotpath, so little point in microoptimizing. I believe the intentions of the code are clear this way.

Comment on attachment 436738 [details] [diff] [review] fast service getters >+ nsCOMPtr<TYPE> os = do_GetService(CONTRACT_ID); \ >+ g##NAME = os.forget().get(); \ This should be os.swap(g##NAME); Or use CallGetService(CONTRACT_ID, &g##NAME); in the first place.

(In reply to comment #72) > I propose documenting this here: > > https://developer.mozilla.org/en/XPCOM/Getting_references_to_services I picked https://developer.mozilla.org/en/XPCOM/mozilla::services_namespace > > And then updating these pages to link to it: > > https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIServiceManager/getService > https://developer.mozilla.org/en/JavaScript_code_modules/Services.jsm Done. Thanks for the pointers. (In reply to comment #75) \ > This should be os.swap(g##NAME); > Or use CallGetService(CONTRACT_ID, &g##NAME); in the first place. Thanks will land that in a followup patch.

(In reply to comment #74) > (In reply to comment #73) > > Named return value optimisation might help you out here but you could have > > simply used a raw nsINetUtil* pointer. > This isn't in any hotpath, so little point in microoptimizing. I believe the > intentions of the code are clear this way. Actually it was the direct access to mRawPtr which I found least readable but I now see that it wasn't your idea after all anyway.

Added a mention and link here: https://developer.mozilla.org/en/XPCOM/XPCOM_changes_in_Gecko_2.0

(In reply to comment #74) > (In reply to comment #73) > > Named return value optimisation might help you out here but you could have > > simply used a raw nsINetUtil* pointer. > > This isn't in any hotpath, so little point in microoptimizing. I believe the > intentions of the code are clear this way. I believe the code is about as far from clear as you'll get.