Use Arena for registry prepopulation

RESOLVED FIXED in mozilla1.0.1

Status

()

P3
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: dp, Assigned: dp)

Tracking

({memory-footprint})

Trunk
mozilla1.0.1
x86
Windows 2000
memory-footprint
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Updated

17 years ago
Status: NEW → ASSIGNED
Keywords: footprint, nsbeta1+
Priority: -- → P3
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 1

17 years ago
Created attachment 73755 [details] [diff] [review]
Arena for factoryentry, location and contractid strings

Arena uses a block size of 1024 to minimize wastage.
(Assignee)

Comment 2

17 years ago
dougt/alecf r=/sr= ?

Comment 3

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

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

Comment 4

17 years ago
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.
Attachment #73755 - Flags: review+

Comment 5

17 years ago
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!
Attachment #73755 - Flags: superreview+
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.
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
(Assignee)

Comment 8

17 years ago
All nice points. Will do and reattach patch.
(Assignee)

Comment 9

17 years ago
Created attachment 74100 [details] [diff] [review]
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.
Attachment #73755 - Attachment is obsolete: true
(Assignee)

Comment 10

17 years ago
Ccing shaver and brendan.

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

Comment 11

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

even better.  r=dougt
Attachment #74100 - Flags: review+

Comment 12

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

sr=alecf
Attachment #74100 - Flags: superreview+
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
(Assignee)

Comment 14

17 years ago
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

17 years ago
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
Attachment #74100 - Flags: approval+
(Assignee)

Updated

17 years ago
Keywords: adt1.0.0
Whiteboard: [adt3]

Comment 16

17 years ago
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.
Keywords: adt1.0.0, nsbeta1+ → adt1.0.0-, nsbeta1-
Whiteboard: [adt3]
(Assignee)

Updated

17 years ago
Target Milestone: mozilla1.0 → mozilla1.0.1
(Assignee)

Comment 17

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