Closed Bug 570488 Opened 10 years ago Closed 10 years ago

Remove unneeded xpti.dat cache file complexity

Categories

(Core :: XPCOM, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: benjamin, Assigned: benjamin)

References

Details

Attachments

(2 files)

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.
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.
Attachment #449652 - Flags: review?(dtownsend)
This fixes and tests the XPT-in-JAR loading to skip XPCOM (which isn't registered yet) and use C++ directly.
Attachment #449653 - Flags: review?(mwu)
Blocks: omnijar
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!
Attachment #449653 - Flags: review?(mwu) → review+
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 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
Attachment #449652 - Flags: review?(dtownsend) → review-
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).
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.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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.
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.
(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.
(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...
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?
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.)
Depends on: 573243
Depends on: 573268
I filed bug 573268 on the leak.
Blocks: 573652
You need to log in before you can comment on or make changes to this bug.