xpti (the typelib loader) shows up high on the spacetrace list of offenders. It eagerly creates xptiInterfaceInfo objects using 'new' for each interface in xpti.dat (currently over one thousand interfaces in a development build). It also uses the malloc intensive plhash. That many calls to new and malloc take time and with so many small long-lived heap allocation we are doubtless wasting space due to heap overhead. At some point we ought to come up with a better scheme to reduce the number of interfaces in xpt files that we actually ship. We know that we use a fairly small percentage of them. See bug 46707. The danger is that we'll fail to ship something that is *sometimes* needed. But, that's a different bug. I've been working on a patch that refactors this stuff so that xpti's per interface info is kept in tight arena allocated structs and the (now lightweight) refcounted xptInterfaceInfos that clients use are created on demand and live only as long as they are being used. I've also converted xpti to use the less allocation-hungry PLDHash tables. Unfortunately, this scheme has a small amount of new locking overhead. I don't think this will show up as significant at all. Sampling instrumentation in the scheme I have shows that we keep between about 70 and 100 of the possible 1074 interface infos alive at any one time. The most alive at one time is about 175. My change includes two small DOM changes: one place that was using an nsIInterfaceInfo pointer without holding a ref, and the use of a new method nsIInterfaceInfoManager that allows building an enumeration of interfaces containing only those interfaces whose names begin with a give prefix (e.g. "nsIDOM") - this allows us to avoid building one of each and every Info at startup for the DOM system. Anyway, I have this all working and will post a patch after I finish a round of code cleanup/review. I just wanted to get this filed to let people know it is on the way.
Created attachment 60968 [details] [diff] [review] big 'ol patch ready for review Most of this big patch is transfering identical logic from xptiInterfaceInfo to xptiInterfaceEntry. I was also able to move a lot of the misc objects from being new'd to being in the arena. Previously anything that referenced an xptiInterfaceInfo needed to be somewhere where a dtor would run at the right time to release the reference. Now that these things refer to the xptiInterfaceEntries that have a lifetime bounded by the lifetime of the arena, we can just move these things into the same arena and not worry about leaking references. (Note that these things were going to live as long as the arena in the old system anyway). A lot of the patch is the conversion to PLDHash. This should be good. Unfortunately, PLDHash table's entry array sizes grow by powers of 2 and our current interface count is about 1074. So these tables are making (long-lived) space for 2048 entries. We could save a few K here by shipping a couple hundred less interface infos in our xpt files.
*maybe* this will make it through review for the current milestone.
in xptiInterfaceInfo::Release what's to prevent AddRef from incrementing mRefCnt after the check to if(mRefCnt)?
> in xptiInterfaceInfo::Release what's to prevent AddRef from incrementing mRefCnt > after the check to if(mRefCnt)? Do you mean if it was found to be zero or found to be non-zero? If it was found to be non-zero, then I don't care. I return non-zero and I'm done. Returning an accurate value from Release is unimportant. Returning zero or non-zero has some use. If it was found to be zero then how could it go up from there? The only entry to AddRef would be through xptiInterfaceEntry::GetInterfaceInfo. And it calls AddRef from inside the monitor we've already acquired - and then only if our object has not deleted itself. We trust that no outside caller would make the mistake of calling AddRef on a pointer to which it is not already holding a reference.
Ok that sounds reasonable, I hadn't looked back to see that the AddRef call was wrapped by the lock in that case.
Comment on attachment 60968 [details] [diff] [review] big 'ol patch ready for review sr=jst
Comment on attachment 60968 [details] [diff] [review] big 'ol patch ready for review r=dbradley Looks pretty good. I've run with this for the past few days under Windows in debug, and updated and added new components and all was well with this code active. One thing I noticed, not sure if it's worth muddying up this patch with it, but the two methods, AddOnlyNewFilesFromFileList and DoFullValidationMergeFromFileList, in xptiInterfaceInfoManager are nearly identical, except for a few small places.
Thanks for the reviews. I'll land this when I get back from my xmas vacation.
*** Bug 119288 has been marked as a duplicate of this bug. ***