xpti calls 'new' too much

RESOLVED FIXED in mozilla0.9.8

Status

()

Core
XPCOM
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: John Bandhauer, Assigned: John Bandhauer)

Tracking

({footprint, perf})

Trunk
mozilla0.9.8
footprint, perf
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

16 years ago
Keywords: footprint, perf
(Assignee)

Comment 1

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

Comment 2

16 years ago
*maybe* this will make it through review for the current milestone.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9.7
(Assignee)

Comment 3

16 years ago
->0.9.8
Target Milestone: mozilla0.9.7 → mozilla0.9.8

Comment 4

16 years ago
in xptiInterfaceInfo::Release what's to prevent AddRef from incrementing mRefCnt
after the check to if(mRefCnt)?
(Assignee)

Comment 5

16 years ago
> 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.

Comment 6

16 years ago
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
Attachment #60968 - Flags: superreview+

Comment 8

16 years ago
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.
Attachment #60968 - Flags: review+
(Assignee)

Comment 9

16 years ago
Thanks for the reviews. I'll land this when I get back from my xmas vacation.
(Assignee)

Comment 10

16 years ago
*** Bug 119288 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 11

16 years ago
checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.