196210 - component/category managers are malloc happy

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Alec Flett]

In spacetrace I was seeing lots of noise around category manager and the
component manager. I discovered that we could clean up a whole lot:

- nsID::ToString() allocates its resulting buffer every time, and it is
guaranteed to be less than 39 bytes every time. So instead, I hand-rolled a
GetIDString() for use in the component manager
- nsCategoryManager was allocating tons of little strings for its hashtables for
the category/name/value stuff. In spacetrace, I saw 20 allocations for the
category names (avg of 22 bytes each), 373 allocations for each name (avg of 14
bytes each), 373 allocations for each nsString used to store the value (16 bytes
each), and 373 allocations for each value (also avg of 14 bytes each) this means
a total of 1139 allocations, most somewhere in the 14-22 byte range. I fixed
this with arenas.
- the component manager was using a 1k arena, and allocating upwords of 28k just
in an embedding build, so I've upped the arena size there to 8k
- we were strlen()'ing a bunch of strings (such as category names or
contractIDs) all over the component manager, even though we had gotten the
length from the nsManifestLineReader.. sometimes we'd strlen() the same string 2
or 3 times in the process of storing it... so now I'm keeping those lengths
around so that we can more quickly do arena allocations, string comparisons, and
so forth.

Patch forthcoming

Comment 1

[Alec Flett]

Attachment: Category/Component Manager optimizations

And here's the patch. 
You'll notice that much of this is just passing around an extra "length"
parameter with many of the strings. This does increase some of the entry sizes
by 4 bytes, so I had to change some of the alpha bounds stuff. Storing this
extra length may seem unnecessary but it actually allows for faster hashing
because we can compare key lengths before comparing keys (though I'm leaving
that up to the string library to do the right thing there)

Comment 2

[Alec Flett]

Comment on attachment 116449 [details] [diff] [review]
Category/Component Manager optimizations

requesting reviews.. doug/dan - mind taking a look?

Comment 3

[Alec Flett]

Ok, checked in (got r=dougt)

Comment 4

[Alec Flett]

oops, marking fixed.
We didn't see much of a startup performance decrease, but we did see the number
of heap allocations go down by 2000 or so...(on average over the last 12 hours
on tinderbox)

Comment 5

[Daniel Veditz [:dveditz]]

Comment on attachment 116449 [details] [diff] [review]
Category/Component Manager optimizations

You don't need sr= in xpcom anymore?

>+AutoRegEntry::AutoRegEntry(const nsACString& name, PRInt64* modDate) :
>+    mModDate(*modDate),
>+    mName(ToNewCString(name)),
>+    mNameLen(name.Length())
...
> AutoRegEntry::~AutoRegEntry()
> {
>     if (mName) PL_strfree(mName);

Should be nsMemory::Free(mName) to match the change to ToNewCString(). Yeah
we're currently using the same malloc/free nspr does, but we might not always.
Also here you add ToNewCString(), but in nsCategoryManager.cpp you rip out a
couple ToNewCStrings() and replace them with nsCRT::strdup() -- why? Should you
do the same here?

You changed some functions to require a passed length, but then everywhere you
call them you stick in a call to strlen(). Unless you're planning further
cleanups you might as well hide the strlen() inside the functions rather than
scatter them all over. In fact most of 'em seem to boil down to calling
ArenaStrndup() instead of ArenaStrdup, but the latter would do the strlen() for
them. In particular I don't see what the length args to FindFactory,
GetFactoryEntry, HashContractID and RegisterComponentCommon buys you except the
need to scatter a dozen strlens throughout.

The only length arg that seems to save you anything is the one in
nsFactoryEntry()

You said this is checked-in, should the bug be marked fixed?

I'd like the PL_strfree() changed to match the allocator; the length arg
comments were an expression of preference and you can do what seems right.

fwiw given this is checked in sr=dveditz