130381 - Use Arena for registry prepopulation

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Suresh Duddi (gone)]

[  956 ] HashContractID   -  44,325
[  856 ] newFactoryEntry  -  27,392
[  804 ] dlllocation      -  12,383
-----------------------------------
[ 2616 ] total allocation -  84,100

Using Arena:

saves us: 2616 * 8 = 20,928
(8 bytes is per malloc overhead)

wastage on average: 512 bytes (using 1k block size)
total alllocs : 83

Patch follows...

Comment 1

[Suresh Duddi (gone)]

Attachment: Arena for factoryentry, location and contractid strings

Arena uses a block size of 1024 to minimize wastage.

Comment 2

[Suresh Duddi (gone)]

dougt/alecf r=/sr= ?

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 73755 [details] [diff] [review]
Arena for factoryentry, location and contractid strings

I doubt that #include <new.h> will work xp.

Comment 4

[Suresh Duddi (gone)]

Comment on attachment 73755 [details] [diff] [review]
Arena for factoryentry, location and contractid strings

r=dougt (after talking to him over AIM). new.h is xp. We already use it.

Comment 5

[Alec Flett]

Comment on attachment 73755 [details] [diff] [review]
Arena for factoryentry, location and contractid strings

sr=alecf
I'll bet this is faster too.
thanks for the ~mFactoryEntry destructor comment!

Comment 6

[Mike Shaver (:shaver emeritus)]

Comment on attachment 73755 [details] [diff] [review]
Arena for factoryentry, location and contractid strings

> #include "prthread.h" /* XXX: only used for the NSPR initialization hack (rick) */

(While we're in here, do we still need that?)

>@@ -325,7 +326,10 @@
> {
>     nsFactoryTableEntry* entry = NS_STATIC_CAST(nsFactoryTableEntry*, aHdr);
> 
>-    delete entry->mFactoryEntry;
>+    // nsFactoryEntry is usually arena allocated including the strings it
>+    // holds. So we dont bother calling the destructor. If the destructor
>+    // does more than freeing memory, call it like this
>+    //     entry->~mFactoryEntry;
>     PL_DHashClearEntryStub(aTable, aHdr);
> }

We don't need factory_ClearEntry any more at all, just use
PL_DHashClearEntryStub
directly in factory_DHashTableOps

>   
>@@ -369,10 +373,13 @@
>         entry->mFactoryEntry->typeIndex == NS_COMPONENT_TYPE_SERVICE_ONLY && 
>         entry->mFactoryEntry->cid.Equals(kEmptyCID)) {
>         // this object is owned by the hash.  Time to delete it.
>-        delete entry->mFactoryEntry;
>+        // nsFactoryEntry is usually arena allocated including the strings it
>+        // holds. So we dont bother calling the destructor. If the destructor
>+        // does more than freeing memory, call it like this
>+        //     entry->~mFactoryEntry;
>     }
> 
>-    nsCRT::free(entry->mContractID);
>+    // mContractID is Arena allocated. So dont free it.
> 
>     PL_DHashClearEntryStub(aTable, aHdr);
> }

Ditto here, where we can save the CID equality checks and whatnot as
well.

>@@ -398,25 +405,34 @@
>                                int aType)
>     : cid(aClass), typeIndex(aType)
> {
>-    MOZ_COUNT_CTOR(nsFactoryEntry);
>-    location = nsCRT::strdup(aLocation);
>+#ifdef DEBUG_dp
>+    printf("newFactoryEntry %d\n", sizeof(nsFactoryEntry));
>+    printf("dlllocation %d %s\n",strlen(aLocation), aLocation);
>+#endif
>+    // Arena allocate the location string
>+    PLArenaPool *arena = &nsComponentManagerImpl::gComponentManager->mArena;
>+    void *mem = nsnull;
>+    PL_ARENA_ALLOCATE(mem, arena, (strlen(aLocation) + 1));
>+    if (mem)
>+        strcpy(NS_STATIC_CAST(char *, mem), aLocation);

While we're looking for speed, use memcpy here, where we already have the
length
to hand off.  (Don't forget to NUL-terminate explicitly, though.)

(Also below in ReInit.)

>+            void *mem = nsnull;
>+            PL_ARENA_ALLOCATE(mem, &mArena, (strlen(aContractID) + 1));
>+            if (mem)
>+                strcpy(NS_STATIC_CAST(char *, mem), aContractID);

Do we want a helper macro for this?  We repeat this verse many times, and
I fear maintenance mis-steps from missing one on a change.

>+// Arena used by component manager for storing contractid string, dll
>+// location strings and small objects
>+#define NS_CM_BLOCK_SIZE (1024)
>+#define PL_ARENA_CONST_ALIGN_MASK 7
>+#include "plarena.h"

Do we really want that in nsComponentManager.h?  We include that file in
a couple of places, and I fear what happens if one of those files (like
the registry, in a newer, braver world) wants to use an PLArena with a
different alignment mask.

Comment 7

[Brendan Eich [:brendan]]

and don't set mem twice in a row:

>+    void *mem = nsnull;
>+    PL_ARENA_ALLOCATE(mem, arena, (strlen(aLocation) + 1));

There's no point in initializing to nsnull.  Save another cycle or two.

Shaver's right, PL_ARENA_CONST_ALIGN_MASK was meant to be #defined early in a .c
file -- can we do that here, or will inlines need it and "plarena.h" in the CM
.h file?

/be

Comment 8

[Suresh Duddi (gone)]

All nice points. Will do and reattach patch.

Comment 9

[Suresh Duddi (gone)]

Attachment: Shaver's comments; fixes leak of factory

Has Shaver's comments:

- uses memcpy() instead of strcpy()
- inlined helper function ArenaStrdup()
- PL_ARENA_CONST_ALIGN_MASK defined in .cpp file instead of .h file

Fixes leak of factory:

- nsFactoryEntry destructor needs to be called so it can release its reference
on factory and serviceobject. The previous patch failed to call the destructor
and introduced the leak

So I tried to figure out why I didn't find the leak the previous time
eventhough I did the XPCOM_MEM_LEAK_LOG thing. I am guessing that in my local
tree while testing the previous patch I had the code calling the destructor.
Eventhough the destructor was empty, the objects in question were COMPtrs and
got released automatically. It is either that or I was looking at the wrong
leak log file. Anyway, sorry about the screw up. I have made doubly sure there
is no other changes and was looking at the right leaklog this time around.

Comment 10

[Suresh Duddi (gone)]

Ccing shaver and brendan.

Forgot to mention the current patch also has brendan's comments of not
initializing mem to nsnull unneccessarily.

Comment 11

[Doug Turner (:dougt)]

Comment on attachment 74100 [details] [diff] [review]
Shaver's comments; fixes leak of factory

even better.  r=dougt

Comment 12

[Alec Flett]

Comment on attachment 74100 [details] [diff] [review]
Shaver's comments; fixes leak of factory

sr=alecf

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 74100 [details] [diff] [review]
Shaver's comments; fixes leak of factory

>+++ nsComponentManager.cpp	14 Mar 2002 17:23:58 -0000
>@@ -34,6 +34,13 @@
> #include "nsCRT.h"
> #include "nspr.h"
> 
>+// Arena used by component manager for storing contractid string, dll
>+// location strings and small objects
>+// CAUTION: Arena align mask needs to be defined before including plarena.h
>+//          currently from nsComponentManager.h
>+#define PL_ARENA_CONST_ALIGN_MASK 7
>+#define NS_CM_BLOCK_SIZE (1024)

Uber-Nit: no need to parenthesize 1024.

>@@ -368,11 +391,13 @@
>     if (entry->mFactoryEntry != kNonExistentContractID && 
>         entry->mFactoryEntry->typeIndex == NS_COMPONENT_TYPE_SERVICE_ONLY && 
>         entry->mFactoryEntry->cid.Equals(kEmptyCID)) {
>-        // this object is owned by the hash.  Time to delete it.
>-        delete entry->mFactoryEntry;
>+        // this object is owned by the hash.

'Nother-Nit: Capitalize This at start of sentence?

Are components or services ever unregistered other than at shutdown?  Non-LIFO
allocatoin using an arena could hurt a lot, because you do not recycle via a
freelist (or freelists, one for factory entries, one for contract-id strings,
etc.).

/be

Comment 14

[Suresh Duddi (gone)]

Components and service are never unregistered even at shutdown time. Unregister
is a separate activity done by regxpcom.exe

Some component might override someother component. In that case the
nsFactoryEntry that got allocated earlier will be reused. The only wasted memory
would be the string location. This happens once in for mozilla now - need to
check why.

DEBUG: ReInit({dd1b8d10-1dd1-11b2-9852-e162b2c46000}, rel:gkplugin.dll)

Comment 15

[Asa Dotzler [:asa]]

Comment on attachment 74100 [details] [diff] [review]
Shaver's comments; fixes leak of factory

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 16

[Jaime Rodriguez, Jr.]

ADT1.0.0- as this is too risky compared to the benefit we would recieve. We
shold land this on the trunk after Mozilla branches for 1.0.

Comment 17

[Suresh Duddi (gone)]

Checked into trunk.