Make nsIComponentLoader a nsIModuleLoader

RESOLVED FIXED in mozilla1.9alpha1

Status

()

defect
P2
normal
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: benjamin, Assigned: benjamin)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

14 years ago
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

14 years ago
This sounds like a great plan to me.
(Assignee)

Updated

14 years ago
Depends on: 316558
(Assignee)

Updated

14 years ago
Assignee: dougt → benjamin
(Assignee)

Updated

14 years ago
Depends on: 316732
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.
(Assignee)

Comment 3

14 years ago
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
Attachment #204699 - Flags: review?(darin)
(Assignee)

Comment 4

14 years ago
I love my life, and I'm even conceited enough to think that this code explains itself without needing a summary.
Attachment #204700 - Flags: review?(shaver)

Comment 5

14 years ago
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
Attachment #204699 - Flags: review?(darin) → review+
(Assignee)

Comment 6

14 years ago
> 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
(Assignee)

Comment 7

14 years ago
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.
(Assignee)

Comment 8

14 years ago
> 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.
(Assignee)

Updated

14 years ago
Depends on: 318625
(Assignee)

Comment 9

14 years ago
Patch with nits picked (includes patch for bug 318625).
Comment on attachment 204700 [details] [diff] [review]
xpconnect nsIModuleLoader, rev. 1

r=shaver
Attachment #204700 - Flags: review?(shaver) → review+
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)

Updated

14 years ago
Blocks: 238324
(Assignee)

Comment 12

14 years ago
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

14 years ago
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

14 years ago
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.
Ts also increased in Camino when this was in.
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.
(that was quite awful to debug)
(Assignee)

Updated

14 years ago
Priority: -- → P2
(Assignee)

Comment 18

14 years ago
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

14 years ago
(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 :(
(Assignee)

Comment 20

14 years ago
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.
(Assignee)

Comment 21

14 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
(Assignee)

Updated

14 years ago
Depends on: 320328

Comment 22

14 years ago
Looks like this may have introduced a regression:  nsNativeComponentLoader.cpp no longer builds under BeOS.
(Assignee)

Comment 23

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

Updated

14 years ago
Depends on: 320542

Comment 24

14 years ago
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.
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);
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.
(Assignee)

Comment 27

14 years ago
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.
Ah, ok.  Wasn't sure the leak cause was known.
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.
Attachment #211505 - Flags: review?(benjamin)
Attachment #211505 - Flags: review?(benjamin) → review+
(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
You passed the same argument twice to NSGetModule.
Attachment #246236 - Flags: review?(benjamin)
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...
(Assignee)

Updated

13 years ago
Attachment #246236 - Flags: review?(benjamin) → review+
Attachment #246236 - Attachment description: pass the right argument to NSGetModule → pass the right argument to NSGetModule (checked in 2006-11-22 10:01)
Depends on: 396509
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).
You need to log in before you can comment on or make changes to this bug.