316416 - Make nsIComponentLoader a nsIModuleLoader

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Benjamin Smedberg]

The current design of nsIComponentLoader leaves a lot of logic to each individual component loader, and each is implemented rather inconsistently WRT how AutoRegister works and timestamp comparisons for "new" modules, etc. I have been wanting to make this sane by making each loader a module loader (hands back nsIModule) instead of a component loader (hands back factory objects). This doesn't appear to affect JavaXPCOM (which doesn't load components), but it will affect PyXPCOM and of course XPConnect.

Mark, if you have build instructions for PyXPCOM I'd love to see them (and one of us should put them up on devmo like the JavaXPCOM instructions are up).

Comment 1

[Darin Fisher]

This sounds like a great plan to me.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

I'm afraid the pyxpcom build docs are all out of date.  The instructions now are theoretically:

* Install Python (for Windows, use 2.3 or earlier.  For recent Linux distros, the builtin Python should be fine).

* Configure with ac_add_options --enable-extensions=python/xpcom

And build.

The changes you mentions may not be as obvious to you as you think though - the pyxpcom C++ code delegates to Python which has the actual implementation of these interfaces.  The extensions/python/xpcom/server/*.py files are where the real work happens (although the C++ code will obviously still need its signature changed).  The changes themselves will not be hard though and I'm happy to help by updating a patch to work with Python.

Comment 3

[Benjamin Smedberg]

Attachment: Change nsIComponentLoader into nsIModuleLoader, rev. 1

I am giddy. I suggest reviewing this in "diff" mode, the raw patch looks uglier than it really is. Overview of changes:

All of the logic "register static components/GRE components/xpcom components dir/xpcom components dir list" has been moved from xpcominit to nsComponentManagerImpl::AutoRegister(null).

When .autoreg indicates that compreg.dat is out of date, we don't even bother reading it, we just start fresh.

The component manager does not keep a file->module cache, it relies on the module loader to do so. This is documented in nsIModuleLoader.idl. nsNativeComponentLoader.mLibraries is this hash for the native module loader.

The component mananger now keeps track of component last-modified times internally, instead of relying on the component loaders to do that and attach "loader-specific data".

The static component loader is special in every way: since it doesn't do file->module mapping but rather string->module it does not implement nsIModuleLoader and it is special-cased in the component manager.

PR logging is forced-on for the native component loader, for ease of extension/app development; I'm pretty sure it won't adversely affect critical codepaths.

xcDll is gone. All the logic thas was there is now wrapped in the native component loader with a PRLibrary* in the hashtable.

I'll have a separate patch for xpconnect

Comment 4

[Benjamin Smedberg]

Attachment: xpconnect nsIModuleLoader, rev. 1

I love my life, and I'm even conceited enough to think that this code explains itself without needing a summary.

Comment 5

[Darin Fisher]

Comment on attachment 204699 [details] [diff] [review]
Change nsIComponentLoader into nsIModuleLoader, rev. 1

>Index: xpcom/build/nsXPComInit.cpp

>+    if (NS_FAILED(compregFile->Exists(&exists)) || !exists)
>+        return PR_TRUE;
>+
>     // Don't need to check whether compreg exists; if it doesn't
>     // we won't even be here.
> 
>     PRInt64 compregModTime, autoregModTime;
>     compregFile->GetLastModifiedTime(&compregModTime);
>     file->GetLastModifiedTime(&autoregModTime);
> 
>     return LL_CMP(autoregModTime, >, compregModTime);
> }

OK, at the very least you should change the comment since it seems
to say that you don't need to call Exists, but you are.  Also, why
are you calling Exists?  Is there some logic error without it, or
do you just find it clearer?


>+    // Pay the cost at startup time of starting this singleton.
>+    nsIInterfaceInfoManager* iim =
>+        xptiInterfaceInfoManager::GetInterfaceInfoManagerNoAddRef();
> 
>+    if (CheckUpdateFile()) {
>+        // If the component registry is out of date, don't bother reading
>+        // it; just make a new one.
>+        (void) iim->AutoRegisterInterfaces();
>+        nsComponentManagerImpl::gComponentManager->AutoRegister(nsnull);
>+    }
>+    else {
>+        rv = nsComponentManagerImpl::gComponentManager->ReadPersistentRegistry();
>+        if (NS_FAILED(rv)) {
>+            // If the persistent registry is malformed or incomplete,
>+            // try to autoregister the default components directories.
>+            (void) iim->AutoRegisterInterfaces();
>+            nsComponentManagerImpl::gComponentManager->AutoRegister(nsnull);
>         }
>     }

How about something like this:

      if (CheckUpdateFile() || NS_FAILED(
          nsComponentManagerImpl::gComponentManager->ReadPersistentRegistry())) {
          // component registry needs to be generated
          iim->...
          nsComponentManagerImpl::gComponentManager->...
      }


>Index: xpcom/components/nsComponentManager.cpp

>+    mLoaderData((nsLoaderdata *)
>+                PR_Malloc(sizeof(nsLoaderdata) * NS_LOADER_DATA_ALLOC_STEP)),
>+    mNLoaderData(0),

It seems like nsTArray<nsLoaderdata> would be a nice alternative here.

In the Init routine you could even call SetCapacity(NS_LOADER_DATA_ALLOC_STEP)
if desired.


>     PR_LOG(nsComponentManagerLog, PR_LOG_DEBUG, ("nsComponentManager: Shutdown complete."));

It might be nice to "#define LOG(args) PR_LOG(nsComponentManagerLog, PR_LOG_DEBUG, args)"
and then just use LOG(("foopy")) in the code.


>+        rv = FileForRegistryLocation(nsDependentCString(values[0], lengths[0]),

Be careful... values[0] still needs to be null terminated at lengths[0].
Maybe you want to use nsDependentSubstring instead?


>+    nsComponentManagerImpl::gComponentManager->

nit: Isn't it annoying to have to type such a long variable name?

/me votes for "gCompMgr" at global scope


>+nsresult
>+nsComponentManagerImpl::FileForRegistryLocation(const nsCString &aLocation,
>+                                                nsILocalFile **aSpec)
>+{
>     // i18n: assuming aLocation is encoded for the current locale
> 
>     nsresult rv;
>+
>+    nsDependentCSubstring prefix = Substring(aLocation, 0, 4);

nit: make that "const nsDependentCSubstring prefix = ..."


>+nsresult
>+nsComponentManagerImpl::AutoRegisterComponent(PRInt32 /* irrelevant */,

I seem to recall some compiler complaining about unnamed parameters.
I used to use unnamed parameters all the time up until that point.
I suggest naming the parameter "irrelevant" or "unused" instead of
not naming it at all.


>+    if (StringEndsWith(registryLocation, NS_LITERAL_CSTRING(".dat"),
>+                       nsCaseInsensitiveCStringComparator()))

Maybe: StringTail(registryLocation, 4).LowerCaseEqualsLiteral(".dat") ?

That should work even if registryLocation has less than 4 characters.


>+            LL_EQ(cachedModTime, modTime))

There's no reason to use LL_ macros anymore.  Even NSPR does raw 64-bit
integer arithmetic now.


>+        for (;
>+             minLoader < GetLoaderCount();
>+             ++minLoader) {

nit: why not put that all on one line?  i think it would read better.


>+        DeferredModule d(registryType, aComponentFile,
>+                         registryLocation, module);
>+
>+        aDeferred.AppendElement(d);

This invokes the copy-ctor on DeferredModule.  (Make sure you are
OK with that.)  Since the copy-ctor is non-trivial in this case:
two AddRef calls and a nsCString copy (though ref counted), it 
might be better to append an empty element, and then call a method
on it to initialize it.


>+    for (PRInt32 i = aLeftovers.Count() - 1;
>+         i >= 0;
>+         --i) {

nit: gratuitous newlines, no?  it could all fit on one line.


>+        for (PRInt32 i = aDeferred.Length() - 1;
>+             i >= 0;
>+             --i) {

ditto


>+RegisterStaticModule(const char *key, nsIModule* module,
...
>+        DeferredModule d(staticComponentType,
>+                         nsnull,
>+                         nsDependentCString(key),
>+                         module);
>+        deferred.AppendElement(d);

same deal here with d's copy-ctor


>+    NS_CreateServicesFromCategory(NS_XPCOM_AUTOREGISTRATION_OBSERVER_ID,
>+                                  aSpec,
>+                                  "start");

nit: "start" could fit on the same line as aSpec.  same with "end" in
the subsequent call.  just a style nit, so whatever.


>+        nsCOMPtr<nsISimpleEnumerator> dirList;
>+        rv = nsDirectoryService::gService->Get(NS_XPCOM_COMPONENT_DIR_LIST,
>+                                               NS_GET_IID(nsISimpleEnumerator),
>+                                               getter_AddRefs(dirList));
>+        if (NS_SUCCEEDED(rv) && dirList) {
>+            PRBool hasMore;
>+            nsCOMPtr<nsISupports> elem;
>+
>+            while (NS_SUCCEEDED(dirList->HasMoreElements(&hasMore)) &&
>+                   hasMore) {
>+                dirList->GetNext(getter_AddRefs(elem));
>+                nsCOMPtr<nsIFile> dir(do_QueryInterface(elem));
>+                if (dir) {
>+                    AutoRegisterImpl(dir, leftovers, deferred);
>+                }
>+            }
>+        }

Can dirList be QI'd to nsIDirectoryEnumerator up front?


>+            for (PRInt32 i = 0; i < leftovers.Count(); ++i) {
>+                ReportLoadFailure(leftovers[i], cs);

Very nice!


>+    mLocationKey(
>+      ArenaStrdup(aLocationKey,
>+                  &nsComponentManagerImpl::gComponentManager->mArena)),

gah... see how nice "gCompMgr" would be?! :-)


>+            rv = nsComponentManagerImpl::gComponentManager->
>+                mStaticComponentLoader.
>+                GetModuleFor(mLocationKey,
>+                             getter_AddRefs(module));

yeah, we are way to verbose in naming variables.


>Index: xpcom/components/nsComponentManager.h

>+    const char* StringForLoaderType(LoaderType aType) {
>+        return mLoaderData[aType].type; }

waah... move closing bracket to a newline :)


>Index: xpcom/components/nsIComponentRegistrar.idl

>+     *                   directory will be registered. 
>+     *                   If aSpec is null, static components, GRE components,
>+     *                   and the application's component directories will be
>+     *                   registered. See NS_GRE_DIR, NS_XPCOM_COMPONENT_DIR,
>+     *                   and NS_XPCOM_COMPONENT_DIR_LIST in
>+     *                   nsDirectoryServiceDefs.h.

Can you similarly update "http://developer.mozilla.org/en/docs/nsIComponentRegistrar"?
(Thanks in advance!)


>Index: xpcom/components/nsIModuleLoader.idl

>+   * the module and return the same module for subsequent requests for the same
>+   * file.

nit: might be worth it to specify that you mean the same physical file,
and not just the same nsIFile object.


>Index: xpcom/components/nsNativeComponentLoader.cpp

>+static PRLogModuleInfo *nsNativeComponentLoaderLog =
>+    PR_NewLogModule("nsNativeComponentLoader");

nit: again, let me recommend defining a LOG macro so you don't have
to type the name of this log module everywhere (it makes the code
more verbose than it needs to be).  might also be good to null check
the log module somewhere, though we don't do that in any other prlog
using code :-(


>+    // Only load components that end in the proper dynamic library suffix
>+    nsCAutoString filePath;
>+    aFile->GetNativePath(filePath);
>+    if (!StringEndsWith(filePath, NS_LITERAL_CSTRING(MOZ_DLL_SUFFIX),
>+                        nsCaseInsensitiveCStringComparator()))
>+        return NS_ERROR_INVALID_ARG;

Again, recommending StringTail + LowerCaseEqualsLiteral.


>+    char *env = getenv("XPCOM_BREAK_ON_LOAD");
>+    char *blist;
>+    if (env && *env && (blist = strdup(env))) {
>+        char *nextTok = blist;
>+        while (char *token = NS_strtok(":", &nextTok)) {
>+            if (leafName.Find(token, PR_TRUE)) {
>+
>+#if defined(_WIN32) && defined(_M_IX86)
>+                ::DebugBreak();
>+#elif (defined(__i386) || defined(__x86_64__)) && defined(__GNUC__)
>+                asm("int $3");
>+#elif defined(VMS)
>+                lib$signal(SS$_DEBUG);
>+#elif defined(XP_MACOSX)
>+                raise(SIGTRAP);
> #endif

Perhaps this code should be moved into a macro or helper function
that can be shared between this code and nsDebugImpl.  Or, why not
just use NS_BREAK()?  Then, the "break on load" behavior could be
controlled by XPCOM_DEBUG_BREAK :-)


>+        else {
>+            PR_LOG(nsNativeComponentLoaderLog, PR_LOG_WARNING,
>+                   ("nsNativeComponentLoader::LoadModule(\"%s\") - Call to NSGetModule failed, rv: %lx", filePath.get(), rv));

nit: wrap long lines


>+    if (PR_LOG_TEST(nsNativeComponentLoaderLog, PR_LOG_DEBUG)) {

nit: it's nice to define a "LOG_ENABLED()" macro that expands to this.


>Index: xpcom/components/nsNativeComponentLoader.h

>+class nsNativeComponentLoader : public nsIModuleLoader

nit: Seems like this guy should be named the nsNativeModuleLoader.


>+    ~nsNativeComponentLoader() {}

make dtor private?


>Index: xpcom/components/nsStaticComponentLoader.cpp

>+    for (StaticModuleInfo *c = mFirst;
>+         c;
>+         c = c->next) {

nit: one line please :-)


>+            nsresult rv = c->info.
>+                getModule(nsComponentManagerImpl::gComponentManager, nsnull,
>+                          getter_AddRefs(c->module));

gCompMgr!


r=darin

Comment 6

[Benjamin Smedberg]

> to say that you don't need to call Exists, but you are.  Also, why
> are you calling Exists?  Is there some logic error without it, or
> do you just find it clearer?

> nit: Seems like this guy should be named the nsNativeModuleLoader.

True, I was just trying to avoid renaming the files, but renaming the class shouldn't be that bad.

> make dtor private?

Can't, it's used directly as a member of nsComponentManagerImpl

Comment 7

[Benjamin Smedberg]

Oops, and the exists() check was necessary because I changed the control flow so you can hit the lastmodified check when the file doesn't exist.

Comment 8

[Benjamin Smedberg]

> Can dirList be QI'd to nsIDirectoryEnumerator up front?

Actually no, since the enumerators from the directory service do not support nsIDirectoryEnumerator.

And it turns out that nsDependentCString is ok since the compreg parser does add null terminators as it parses.

Comment 9

[Benjamin Smedberg]

Attachment: XPCOM bits, rev. 1.1 (nits picked)

Patch with nits picked (includes patch for bug 318625).

Comment 10

[Mike Shaver (:shaver emeritus)]

Comment on attachment 204700 [details] [diff] [review]
xpconnect nsIModuleLoader, rev. 1

r=shaver

Comment 11

[[e-mail]]

Comment on attachment 204735 [details] [diff] [review]
XPCOM bits, rev. 1.1 (nits picked)

>+    nsFactoryEntry(const nsCID    &aClass,
>+                   nsIFactory     *aFactory,
>+                   nsFactoryEntry *aParent = nsnull) :
>+        mCid(aClass),
>+        mLoaderType(NS_LOADER_TYPE_INVALID),
>+        mFactory(aFactory),
>+        mParent(aParent) {
>+        mLocationKey = nsnull;
>+    }
Nit: mLocationKey should be in the initializer list (like in the other ctor)

Comment 12

[Benjamin Smedberg]

I landed this, and it provoked a significant Ts cost on seamonkey-windows tboxen, a little bit on the firefox-windows tboxen, and basically flat on the non-windows tboxen... I suspect that this works out to nsIHashable nsIFiles being slow to hash on windows (due to GetShortPathName) and having to hash a lot of them in seamonkey because it's not static.

Seamonkey monkey tbox is also orange because of failed DOMToTextConversion test, but I can't reproduce on what appears to be an identical build setup.

Any suggestions for how windows nsIFile hashing could be made faster (is it worth it to even consider a cached hashcode?)

Comment 13

[Robert Kaiser]

As a side note, the RLk number on brad went up slightly (82%, 308B -> 560B) as well, the log from the first cycle after checkin says the following:

----------------------------------------------leaks------leaks%------bloat------bloat%
TOTAL                                           560      81.82%   12974852       3.49%
--NEW-LEAKS-----------------------------------leaks------leaks%-----------------------
nsSystemPrincipal                                32          -
BackstagePass                                    16          -
XPCWrappedNative                                144     200.00%
XPCNativeScriptableShared                       216     100.00%

Comment 14

[Robert Kaiser]

From the object counts in http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1133548080.22485.gz&fulltext=1 it looks like this comes down to new leaks of 1 XPCNativeScriptableShared, 2 XPCWrappedNative, 1 BackstagePass and 1 nsSystemPrincipal objects.

Comment 15

[Simon Fraser [no longer active]]

Ts also increased in Camino when this was in.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 204735 [details] [diff] [review]
XPCOM bits, rev. 1.1 (nits picked)

nsComponentManagerImpl::RegisterComponentCommon now only initializes |rv| if a contract ID is given. this means that it will often return a failure value even though it succeeded. that means that RegisterFactoryLocation will fail, which means that component registration for that library will be aborted.

that made my opt build with this patch not start up.

Comment 17

[Christian :Biesinger (don't email me, ping me on IRC)]

(that was quite awful to debug)

Comment 18

[Benjamin Smedberg]

Re-landed on trunk with the windows hashing and uninitialized-rv fixed. Windows is a lot better (completely flat Ts numbers). Mac is still bad.

I'm going to leave this in and do some investigation on my mac: the guys in #camino say that CFHash() on a CFURLRef doesn't stat or anything like that, but it could be a very non-unique hash function because it only hashes the first and last 8 bytes of the URL string. I'm going a build now with PL_DHASHMETER to check the stats.

Comment 19

[Robert Kaiser]

(In reply to comment #18)
> Re-landed on trunk with the windows hashing and uninitialized-rv fixed. Windows
> is a lot better (completely flat Ts numbers). Mac is still bad.

Erm, SeaMonkey Windows (creature) is bad again as well, Ts went from 1031ms to 1266ms, that's again the 25% increase we have seen when it landed the first time :(

Comment 20

[Benjamin Smedberg]

Give creature a day to settle: it can take several runs for Ts numbers to stabilize after a change like this (for reasons that are mysterious to me). Take a look at the comet Ts graph, which looked horrible for a while today.

Comment 21

[Benjamin Smedberg]

The consensus seems to be to leave this in and deal with perf issues in another bug, I'll file it shortly (right now I suspect nsLocalFile::Equals on both mac and windows as culprits). I've gotten Ts added to the firefox tboxen to help track this info. Note that the monkey Ts leap is my first priority: creature went up but pacifica didn't and dynamic builds are not nearly as high a priority.

Comment 22

[Doug Shelton]

Looks like this may have introduced a regression:  nsNativeComponentLoader.cpp no longer builds under BeOS.

Comment 23

[Benjamin Smedberg]

Doug, please file a new bug with the details of the BeOS bustage: there aren't any BeOS tinderboxes (that I know of).

Comment 24

[Doug Shelton]

you are correct.  there are no BeOS tinderboxen at the moment, but we in the community are pursuing several avenues to rectify the situation.  Frequent builds are trying to fill the gap in the meantime.

Comment 25

[Christian :Biesinger (don't email me, ping me on IRC)]

It would've been nice to continue showing the actual error text... and to continue using uppercase FAIL so that log searching works as expected.

And continuing printing the error to stderr would've been immensely helpful as well.

-        if (!dll->Load()) {
 
-            PR_LOG(nsComponentManagerLog, PR_LOG_ALWAYS,
-                   ("nsNativeComponentLoader: load FAILED"));
-        
-            char errorMsg[1024] = "<unknown; can't get error from NSPR>";
-
-            if (PR_GetErrorTextLength() < (int) sizeof(errorMsg))
-                PR_GetErrorText(errorMsg);
-
-            DumpLoadError(dll, "GetFactory", errorMsg);

Comment 26

[Boris Zbarsky [:bzbarsky]]

So what's the plan for the refcount leaks this introduced?  The ones mentioned in comment 13?

See also http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1134504360.21620.gz (the last time this was checked in); it shows exactly those leaks.

Comment 27

[Benjamin Smedberg]

bz, I'm planning on dealing with the refcnt leaks as outlined in the xpcom shutdown doc in general, the xpconnect leaks are going to be fairly late in the process, since I'll have to do the xpcom-shutdown-gc notifications in order to properly release the xpconnect globals during shutdown.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  Wasn't sure the leak cause was known.

Comment 29

[Christian :Biesinger (don't email me, ping me on IRC)]

filed bug 321673 on comment 25

Comment 30

[[e-mail]]

Comment on attachment 204735 [details] [diff] [review]
XPCOM bits, rev. 1.1 (nits picked)

>+    // We haven't loaded this module before
>+#ifdef NS_BUILD_REFCNT_LOGGING
>+    nsTraceRefcntImpl::SetActivityIsLegal(PR_FALSE);
> #endif
> 
>+    rv = aFile->Load(&data.library);
>+    if (NS_FAILED(rv)) {
Sadly you neglected to make activity legal after the load fails.

Comment 31

[[e-mail]]

Attachment: Fix failed loads

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #31)
> Created an attachment (id=211505) [edit]
> Fix failed loads

I checked this patch in:
Checking in xpcom/components/nsNativeComponentLoader.cpp;
/cvsroot/mozilla/xpcom/components/nsNativeComponentLoader.cpp,v  <--  nsNativeComponentLoader.cpp
new revision: 1.117; previous revision: 1.116
done

Comment 33

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: pass the right argument to NSGetModule (checked in 2006-11-22 10:01)

You passed the same argument twice to NSGetModule.

Comment 34

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 246236 [details] [diff] [review]
pass the right argument to NSGetModule (checked in 2006-11-22 10:01)

That said, this file object is already available as the __LOCATION__ property of the global object, so I'm not really sure what the point is...

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

Added bug 396509 to the dependency list because it's very clearly fallout from this patch (based on the regression window and a scouring of the checkins).