570488 - Remove unneeded xpti.dat cache file complexity

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

The current xpti.dat cache file doesn't actually cache XPT data, it only caches name/IID data. With linked XPT files in Firefox (and Thunderbird, and hopefully everyone else), this optimization is probably hurting runtime, and is definitely hurting in terms of code complexity.

I have a patch which just removes all the complexity and the xpti.dat caching. Running through tryserver now, but it looks good.

Comment 1

[(not currently active) Ted Mielczarek]

I'm fairly certain everyone uses a packaging manifest now. If not, then they can use one if this bothers them.

http://mxr.mozilla.org/comm-central/source/mail/installer/package-manifest.in
http://mxr.mozilla.org/comm-central/source/suite/installer/package-manifest.in

Comment 2

[Benjamin Smedberg]

Attachment: Stop reading/writing xpti.dat and incrementally loading XPT files, rev. 1

More details about how the prior XPT code worked, and how it now works:

* previously, each file was read independently into a "working set" of XPT information. The appropriate data from each working set was then merged into the main working set owned by the interface info manager. This was done as a memory optimization so that duplicate data from xpt arenas could be discarded. Given the small number of XPTs we typically load, and the small amount of duplicate data in extension XPT files, this arena optimization isn't worth worrying about, so we now load all the XPTs into the main arena

* previously, an xptiInterfaceEntry had an UNRESOLVED state: this happens when we read xpti.dat and know that foo.xpt provides some IID/name, but haven't loaded that XPT yet. Now that xpti.dat doesn't exist any more, xptiInterfaceEntry is always has interface information from the XPT. I've merged xptiInterfaceGuts into xptiInterfaceEntry.

* previously, all interface info was associated with a file or ZIP through xptiTypeLib. We no longer need to record any of these associations, so xptiTypeLib/xptiFile/xptiZipItem have been removed, and interface into has pointers directly to its xptiTypelibGuts.

Comment 3

[Benjamin Smedberg]

Attachment: part B: Fix loading of XPTs nested in JARs, rev. 1

This fixes and tests the XPT-in-JAR loading to skip XPCOM (which isn't registered yet) and use C++ directly.

Comment 4

[Michael Wu [:mwu]]

Comment on attachment 449653 [details] [diff] [review]
part B: Fix loading of XPTs nested in JARs, rev. 1

Looks good to me and it simplifies the omnijar xpt patch a bunch. Thanks!

Comment 5

[Dave Townsend [:mossop]]

Comment on attachment 449652 [details] [diff] [review]
Stop reading/writing xpti.dat and incrementally loading XPT files, rev. 1

>diff --git a/xpcom/build/nsXPComInit.cpp b/xpcom/build/nsXPComInit.cpp

>     NS_TIME_FUNCTION_MARK("Next: try to load compreg.dat");
> 
>+#if 0 // The constructor does this, don't do it twice!
>+    // If the component registry is out of date, malformed, or incomplete,
>+    // autoregister the default component directories.
>+    (void) iim->AutoRegisterInterfaces();
>+#endif,

Any reason for keeping this?

>diff --git a/xpcom/reflect/xptinfo/src/xptiInterfaceInfoManager.cpp b/xpcom/reflect/xptinfo

You're mixing styles a lot in this file. The original seems to be braces on a new line so I suggest sticking with that in your new code.

>+XPTHeader* 
>+xptiInterfaceInfoManager::ReadXPTFile(nsILocalFile* aFile)
>+{
>+    AutoCloseFD fd;
>+    if (NS_FAILED(aFile->OpenNSPRFileDesc(PR_RDONLY, 0444, &fd)) || !fd)
>+        return NULL;

Any reason for using NSPR here rather than just opening a file stream and calling ReadXPTFileFromInputStream? If not how about moving the actual read of the xpt into a shared function, your two implementations of it don't quite match.

>+XPTHeader*
>+xptiInterfaceInfoManager::ReadXPTFileFromInputStream(nsIInputStream *stream)
> {
>-    NS_ASSERTION(aFile, "loser!");
>-
>-    XPTHeader *header = nsnull;
>-    char *whole = nsnull;
>-    PRFileDesc*   fd = nsnull;
>-    XPTState *state = nsnull;
>-    XPTCursor cursor;
>-    PRInt32 flen;
>-    PRInt64 fileSize;
>+    PRUint32 flen;
>+    stream->Available(&flen);
>     
>-    PRBool saveFollowLinks;
>-    aFile->GetFollowLinks(&saveFollowLinks);
>-    aFile->SetFollowLinks(PR_TRUE);
>-
>-    if(NS_FAILED(aFile->GetFileSize(&fileSize)) || !(flen = nsInt64(fileSize)))
>-    {
>-        aFile->SetFollowLinks(saveFollowLinks);
>+    nsAutoArrayPtr<char> whole(new char[flen]);
>+    if (!whole)
>         return nsnull;
>-    }
>-
>-    whole = new char[flen];
>-    if (!whole)
>-    {
>-        aFile->SetFollowLinks(saveFollowLinks);
>-        return nsnull;
>-    }
> 
>     // all exits from on here should be via 'goto out' 

This comment is obsolete

>diff --git a/xpcom/reflect/xptinfo/src/xptiTypelibGuts.cpp b/xpcom/reflect/xptinfo/src/xptiTypelibGuts.cpp

>+xptiInterfaceEntry*
>+xptiTypelibGuts::GetEntryAt(PRUint16 i)
> {
>-    // empty
>+    static const nsID zeroIID =
>+        { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } };
>+
>+    NS_ASSERTION(mHeader, "bad state");
>+    NS_ASSERTION(i < GetEntryCount(), "bad index");
>+
>+    xptiInterfaceEntry* r = mEntryArray[i];
>+    if (r)
>+        return r;
>+
>+    XPTInterfaceDirectoryEntry* iface = mHeader->interface_directory + i;
>+
>+    xptiWorkingSet* set =
>+        xptiInterfaceInfoManager::GetSingleton()->GetWorkingSet();
>+
>+    if (iface->iid.Equals(zeroIID))
>+        r = set->mNameTable.Get(iface->name);
>+    else
>+        r = set->mIIDTable.Get(iface->iid);

Add a newline here please.

>+    if (r)
>+        SetEntryAt(i, r);

Comment 6

[Dave Townsend [:mossop]]

Comment on attachment 449652 [details] [diff] [review]
Stop reading/writing xpti.dat and incrementally loading XPT files, rev. 1

r- just for some answers to my previous comments, the rest seems to be ok though

Comment 7

[Benjamin Smedberg]

review comments addressed at http://hg.mozilla.org/users/bsmedberg_mozilla.com/static-xpcom-registration/rev/267bf53413a9

* Switched all of xptiInterfaceInfoManager.cpp to "if {" to match the standard style and my preference as module owner.
* The #if 0 was removed in the static-xpcom patch followup.
* We can't use file streams because necko hasn't been registered yet when this code runs (and I want to switch this code to use memory mapping probably).

Comment 8

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/894850aef55c
http://hg.mozilla.org/mozilla-central/rev/0def0bbb620d

The jar xptloading test was disabled after these commits since it doesn't seem to work. Will reenable after making sure it works on try.

Comment 9

[Alfred Kayser]

P.s. zip-reader-cache is now no longer needed to be a public interface.
http://mxr.mozilla.org/comm-central/search?string=zip-reader-cache
only returns a reference to a test case and the registration of zip-reader-cache itself.
This will allow to change the zipcache stuff internal to nsJar, which allows for more optimizations/cleanup.

Comment 10

[Mark Banner (:standard8)]

Note: this bug seems to have caused a 0.87 MB increase in tracemalloc leaks on Mac, and a 0.96 MB increase in tracemalloc leaks on Windows.

Michael says he's going to take a look in the morning.

Comment 11

[Michael Wu [:mwu]]

(In reply to comment #10)
> Note: this bug seems to have caused a 0.87 MB increase in tracemalloc leaks on
> Mac, and a 0.96 MB increase in tracemalloc leaks on Windows.
> 
> Michael says he's going to take a look in the morning.
This appears to be intentional according to this comment in xptiWorkingSet.cpp:
    // Don't destroy the arena; we're shutting down, why touch the
    // memory when we don't have to?

I'm going to try destroying the arena on try and see if it cleans things up.

Comment 12

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

This, or something else in
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7523ad2f395e&tochange=91851c94fb73
caused a huge regression in the windows trace-malloc leak numbers:
http://graphs.mozilla.org/#tests=[[28,1,107]]&sel=1276470890,1276596525

Comment 13

[Mark Banner (:standard8)]

(In reply to comment #12)
> This, or something else in
> http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7523ad2f395e&tochange=91851c94fb73
> caused a huge regression in the windows trace-malloc leak numbers:
> http://graphs.mozilla.org/#tests=[[28,1,107]]&sel=1276470890,1276596525

It did, see comments 10 and 11...

Comment 14

[Benjamin Smedberg]

dbaron, can tinderbox logs tell us what was being allocated? Leaking the XPT arenas is intentional, as noted, but I wouldn't expect it to be 1.1MB of data. Is it possible to inform trace-malloc that the XPT arenas are intentionally not freed because it would just harm shutdown time?

Comment 15

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

The very first tinderbox log with the increase should have a stack tree of the delta, but I'm not sure whether the symbols will be good.

Also:  regarding not freeing to save shutdown time, we should perhaps ifdef such things so that people (and debug tinderboxes) doing leak debugging don't see the leaks.  (It also seems sort of silly to not spend time freeing one thing when we spend time freeing everything else; maybe an appropriate ifdef-ing approach would fix that.)

Comment 16

[Jesse Ruderman]

I filed bug 573268 on the leak.