96461 - ComponentManager could PLHash or PLDHash

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P2)

Description

[Suresh Duddi (gone)]

Instead of using nsHashtable, there could be wins using PLHash straight.

From Brendan:
nsHashtable |new|s every key that's Put in the table, and the underlying
plhash.c code malloc's each entry structure, so for E entries, you have 2E words
of malloc overhead.  Then there is the PLHashEntry linkage, at one word per
entry.  Then there is the bucket overhead, T words for a table of size T.  If
you use pldhash, you save all those, and potentially more if you can coalesce
key and entry allocations -- but you waste (T-E)*sizeof(YourEntrySubtype) bytes
on unused/removed entries.  See
http://lxr.mozilla.org/mozilla/source/xpcom/ds/pldhash.h#99 for the trade-off
formula.

Comment 1

[Mike Shaver (:shaver emeritus)]

I did some work on this in bug 46832, but I've lost the patch-in-progress from
it.  PLDHash was a win for me, based on the numbers brendan and I ran.

Comment 2

[Suresh Duddi (gone)]

That is good to know shaver. Upping priority.

Comment 3

[Suresh Duddi (gone)]

Doug, the new brave owner of xpcom has accepted to do these. Yeah! thanks doug.

Comment 4

[Doug Turner (:dougt)]

neeti is working on this.

Comment 5

[neeti]

Attachment: patch using pldhash

Comment 6

[neeti]

I have converted mFactories and mContractIDs tables to use pldhash. The 
enumeration code is based on shaver's patch from bug 46832. I am in process of 
evaluating the change in the performance numbers.

Comment 7

[Brendan Eich [:brendan]]

I didn't get through the entire patch, but I found some problems worth reporting
quickly.  Please fix these before testing too much, and I'll be happy to review
a revised patch.

Comments, important ones first, followed by nit-picks.

- contractID_GetKey can't be right -- it should return entry->mContractID, not
  &entry->mContractID (the latter is char **, but the callers to getKey want
  the same type that contractID_MatchEntry expects to be able to cast its aKey
  parameter to: char * or const char *).

- contractID_ClearEntry is buggy:
  - it leaks mContractID, which should be freed with nsCRT::free() to match
    how it was allocated.
  - it does not need to null-test entry -- get rid of that outer if statement.

- 2048 entries in the initial tables seem like quite a few -- do we believe
  that the table always ends up near this size, steady state -- and that this
  saves significant cycles compared to growing the table by powers of 2 from
  256?  If not, let the table start out smaller, and let it grow and shrink
  according to its load factor

- Return NS_ERROR_OUT_OF_MEMORY, not NS_ERROR_NULL_POINTER, if a PL_DHASH_ADD
  PL_DHashTableOperate on the mFactories table returns null.  See the correct
  return code for the mContractIDs table case.

- Nit: test PL_DHASH_ENTRY_IS_BUSY(e) rather than !PL_DHASH_ENTRY_IS_FREE(e)
  for an entry pointer e.

- Nit: indentation of underhanging parts of multi-line NS_STATIC_CAST wrapped
  calls to PL_DHashTableOperate do not line up as nicely as they might.  For
  example, compare:

+        nsFactoryTableEntry* factoryTableEntry =
+            NS_STATIC_CAST(nsFactoryTableEntry*,
+                       PL_DHashTableOperate(&mFactories, &aClass,
+                                            PL_DHASH_ADD));

  or my revision of it:

+        nsFactoryTableEntry* factoryTableEntry =
+            NS_STATIC_CAST(nsFactoryTableEntry*,
+                           PL_DHashTableOperate(&mFactories, &aClass,
+                                                PL_DHASH_ADD));

  Or again:

+    nsContractIDTableEntry* contractIDTableEntry =
+                    NS_STATIC_CAST(nsContractIDTableEntry*,
+                       PL_DHashTableOperate(&mContractIDs, aContractID,
+                                            PL_DHASH_ADD));

  (which is not consistent in formatting even compared to the first example)
  compared to this:

+    nsContractIDTableEntry* contractIDTableEntry =
+        NS_STATIC_CAST(nsContractIDTableEntry*,
+                       PL_DHashTableOperate(&mContractIDs, aContractID,
+                                            PL_DHASH_ADD));

  Oh -- I bet the problem in this second example is just the NS_STATIC_CAST
  being overindented -- if you pull that back to underhang by four spaces, it
  looks great.  Same problem afflicts

+    nsContractIDTableEntry* contractIDTableEntry =
+            NS_STATIC_CAST(nsContractIDTableEntry*,
+                       PL_DHashTableOperate(&mContractIDs, aContractID,
+                                            PL_DHASH_LOOKUP));

  soon after.  And so on.

- No need to set entry to nsnull just to return nsnull:

+                nsFactoryTableEntry* factoryTableEntry =
+                    NS_STATIC_CAST(nsFactoryTableEntry*,
+                       PL_DHashTableOperate(&mFactories, &cidKey,
+                                            PL_DHASH_ADD));
+                if (!factoryTableEntry) {
+                    entry = nsnull;
+                    return entry;
+                }

  Instead, just do this (I also fixed the underhanging indentation):

+                nsFactoryTableEntry* factoryTableEntry =
+                    NS_STATIC_CAST(nsFactoryTableEntry*,
+                                   PL_DHashTableOperate(&mFactories, &cidKey,
+                                                        PL_DHASH_ADD));
+                if (!factoryTableEntry) {
+                    return nsnull;
+                }

/be

Comment 8

[Mike Shaver (:shaver emeritus)]

When last I looked (April) we had about 700 contract IDs in a Mozilla build, so
I was sizing the table at 1024.  See the comment in pldhash.h and run the math
to see if pldhash or plhash will be cheaper, based on your structure size and
expected loading.

I'll take a look at this patch more after I get done with my 0.9.5 duties.

Comment 9

[neeti]

Attachment: revised patch

Comment 10

[neeti]

I have attached a revised patch based on all the changes Brendan mentioned, 
except for the table size. 

Currently we have around 785 entries in the mFactories table, and around 870 
entries in the mContractIDs table.  If we keep the initial table size to 1024, 
then we grow once to 2048, when the table reaches 75% of it capacity. What would 
be a good way to handle 785 and 870 entries?

Comment 11

[neeti]

If we start of the table size at 256 and grow by powers of 2, we still end up 
with a table of 2048 entry capacity, with only 785 or 870 entries.

Comment 12

[lidl]

Maybe I'm missing something obvious here -- it could be, I haven't looked
into how the PL{,D}Hash code works.

If it auto-expands at 75% of full, why not just take a slightly
rounded up number near 870 (call it 900):

((900*100)/75)/4) = 300

So, start at 300, it will auto-expand twice, and end up at 1200
entries in the table.

Comment 13

[Mike Shaver (:shaver emeritus)]

PLDHash always uses the least power of two greater than or equal to the provided
size for its allocations, so 300 is the same as 512 for purposes of that call.

Brendan's going to tweak pldhash for us, likely to require an alpha of >= 0.875
before expanding.  That should hold us to 1024 unless we add a few dozen more
contractIDs.  At that point, we'll be < 0.5 alpha, and then it might make sense
to look at chaining instead of double-hashing, depending on the entry size.

Comment 14

[Brendan Eich [:brendan]]

[e-mail], double hashing requires a power-of-two sized table.

shaver, see the formula at

http://lxr.mozilla.org/mozilla/source/js/src/jsdhash.h#152

and do the math (then shop :-).  For nsFactoryTableEntry, whose k=2 and whose
esize is six words, alpha must be >= (6-1)/(6+2) or .625 for double hashing to
win.  But, I just noticed that nsFactoryEntry (the pointed-at "value" of
nsFactoryTableEntry) contains the CID.  So there is absolutely no need to store
the CID redundantly in nsFactoryTableEntry.

IOW, we have k=3 if we use mFactoryEntry->cid as the key, and do away with mCID
from the nsFactoryTableEntry struct.  Now, with k=3 and esize=2, we need alpha
>= (2-1)/(2+3) or .2, and the current double hashing implementation guarantees
alpha >= .25, so double hashing always wins.  Yeah!

Having thought this through, however, the question arises: why allocate
nsFactoryEntry separately?  Why not make it an extension of PLDHashEntryHdr, for
k=4?  Then esize would be a horrendous 4+16+24+4+4+4 bytes or 14 words, mainly
due to the nsCString location (24 bytes in an nsCString directly, not counting
the malloc'd buffer stuff!).  We really should make location a char*, I think. 
Say we do that.  esize drops to 9 words and alpha must be >= (9-1)/(9+4) or
.6923 for double hashing to beat chaining.

I think that's acceptable as an alpha lower bound -- however, if we derive the
nsFactoryEntry class from PLDHashEntryHdr, then the nsFactoryEntry address is
not invariant: table shrinks, compresses, and grows will move entry addresses,
making life hard for nsContractIDTableEntry::mFactoryEntry.

Ok, back to k=3 (use mFactoryEntry->cid, do not store mCID redundantly in
nsFactoryTableEntry).  That's guaranteed to win, because .2 < .25.

The math for nsContractIDTableEntry is easier: k=2 (we must have key and value
pointers separately, and again mFactoryEntry must not vary during an entry's
lifetime), esize=3, alpha >= (3-1)/(3+2) or .4.

If we find that the number of contract-id to factory mappings is such that .25
<= alpha < .4, then we might consider switching the mContractIDs table from
PLDHashTable to PLHashTable, but I rather think we would do better to add a way
to "advise" the pldhash.c code about minimum and maximum alpha, so a client can
tune its table growth and shrinkage.

Comments?

I'll look at the latest patch later today, but neeti: feel free to cook up a new
patch that gets rid of nsFactoryTableEntry::mCID.

dp, anyone: please file a bug on the nsCString location member of nsFactoryEntry
being pure, evil bloat.

/be

Comment 15

[Brendan Eich [:brendan]]

Attachment: raise maximum alpha to .875 from .75 in {js,pl}dhash.[ch]

Comment 16

[Brendan Eich [:brendan]]

Attachment: better {js,pl}dhash.[ch] patch, max/min alpha defaults are 0.875/.25, and you can PL_DHashTableSetAlphaBounds to tweak 'em

Comment 17

[Brendan Eich [:brendan]]

Attachment: best {js,pl}dhash.[ch] patch: get minAlpha clamping right, provide PL_DHASH_MIN_ALPHA macro for users

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 51938 [details] [diff] [review]
better {js,pl}dhash.[ch] patch, max/min alpha defaults are 0.875/.25, and you can PL_DHashTableSetAlphaBounds to tweak 'em

best trumps better.

Comment 19

[Brendan Eich [:brendan]]

Neeti, please try applying attachment 59182 [details] [diff] [review] from your mozilla directory, after
reverting js/src/jsdhash.[ch] and xpcom/ds/pldhash.[ch] to top of trunk.  Then, call

    PL_DHashTableSetAlphaBounds(&mContractIDs,
                                0.875,
                                PL_DHASH_MIN_ALPHA(&mContractIDs, 2));

right after you PL_DHashTableInit(&mContractIDs, ...).

We can even tweak the 0.875 a bit if you like, based on number of
contract-ID-to-factory mappings.  There may be more contract-IDs than factory
objects, right?  What's the current entryCount with your last patch for both
mFactories and mContractIDs, once all components have registered?

/be

Comment 20

[Brendan Eich [:brendan]]

Comment on attachment 51880 [details] [diff] [review]
revised patch

No need for the cast in the delete operator expression -- entry is already of
the cast-to type:

+PR_STATIC_CALLBACK(void)
+factory_ClearEntry(PLDHashTable *aTable, PLDHashEntryHdr *aHdr)
+{
+    nsFactoryTableEntry* entry = NS_STATIC_CAST(nsFactoryTableEntry*, aHdr);
+
+    delete ((nsFactoryTableEntry *)entry)->mFactoryEntry;
+    PL_DHashClearEntryStub(aTable, aHdr);
+}

Nit: extra newline after local variable declaration:

+PR_STATIC_CALLBACK(void)
+contractID_ClearEntry(PLDHashTable *aTable, PLDHashEntryHdr *aHdr)
+{
+    nsContractIDTableEntry* entry = NS_STATIC_CAST(nsContractIDTableEntry*, aHdr);
+
+    

Don't free and re-allocate a contractID that must by definition be the same
in the existing entry (what you pass to free) and in the new entry:

@@ -1251,8 +1334,19 @@
     if(!aContractID)
         return NS_ERROR_NULL_POINTER;
     
-    nsCStringKey key(aContractID);
-    mContractIDs->Put(&key, fe);
+    nsContractIDTableEntry* contractIDTableEntry =
+        NS_STATIC_CAST(nsContractIDTableEntry*,
+                       PL_DHashTableOperate(&mContractIDs, aContractID,
+                                            PL_DHASH_ADD));
+    if (!contractIDTableEntry)
+        return NS_ERROR_OUT_OF_MEMORY;
+
+    if (contractIDTableEntry->mContractID)
+        nsMemory::Free(contractIDTableEntry->mContractID);
+
+    contractIDTableEntry->mContractID = nsCRT::strdup(aContractID);
+    contractIDTableEntry->mFactoryEntry = fe;

Instead, strdup and set the key only if (!contractIDTableEntry->mContractID).

But always set mFactoryEntry, it may be changing.

You could do a similar kind of optimization for nsFactoryTableEntry, but since
in this patch, its key is mCID, you can't test if (!factoryTableEntry->mCID)
or (!factoryTableEntry->mCID.m0) -- but what you can null-test to decide
whether the PL_DHASH_ADD that just succeeded did indeed create a new entry, or
merely find an existing one, is this: if (!factoryTableEntry->mFactoryEntry).
Then and only then do you need to copy aClass into mCID.  IOW, for example:

+                nsFactoryTableEntry* factoryTableEntry =
+                    NS_STATIC_CAST(nsFactoryTableEntry*,
+                                   PL_DHashTableOperate(&mFactories, &cidKey,
+                                                        PL_DHASH_ADD));
+                if (!factoryTableEntry) {
+                    return nsnull;
+                }
+
+                // Don't copy the CID key unless we just created this entry.
+                if (!factoryTableEntry->mFactoryEntry)
+                    factoryTableEntry->mCID = aClass;
+                factoryTableEntry->mFactoryEntry = entry;

Nit: misindented closing brace below:


@@ -1967,8 +2169,15 @@
 #endif
     {
         nsAutoMonitor mon(mMon);
-        nsCStringKey key(aContractID);
-        nsFactoryEntry* entry = (nsFactoryEntry*)mContractIDs->Get(&key);
+        nsFactoryEntry *entry = nsnull;
+        nsContractIDTableEntry* contractIDTableEntry =
+            NS_STATIC_CAST(nsContractIDTableEntry*,
+                           PL_DHashTableOperate(&mContractIDs, aContractID,
+                                                PL_DHASH_LOOKUP));
+  
+        if (PL_DHASH_ENTRY_IS_BUSY(contractIDTableEntry)) {
+            entry = contractIDTableEntry->mFactoryEntry;
+    }

Nit: enumerator is misspelled as "emumerator" in existing code, can you fix?

@@ -2886,8 +3286,9 @@
             return rv;
     }
 
-    return NS_NewHashtableEnumerator(mFactories, ConvertFactoryEntryToCID,
-                                     this, aEmumerator);
+    return PL_NewDHashTableEnumerator(&mFactories,
+                                      ConvertFactoryEntryToCID,
+                                      nsnull, aEmumerator);

More comments on the Enumerator stuff when I have time.

/be

Comment 21

[neeti]

Applied (js,pl)dhash attachment 51982 [details] [diff] [review], and made all the changes Brendan 
mentioned above. With my latest patch we have 789 entries in the mFactories 
table, and 875 entries in the mContractIDs table.

Next, I will work on a patch that gets rid of nsFactoryTableEntry::mCID

Comment 22

[neeti]

Attachment: revised patch using PL_DHashTableSetAlphaBounds(..)

Comment 23

[Brendan Eich [:brendan]]

Neeti, thanks -- one slight improvement: don't PL_DHashTableSetAlphaBounds on
every call to nsComponentManagerImpl::Init, if that method can be called more
than once for a given instance of nsComponentManagerImpl.  Do SetAlphaBounds in
the if (!mContractIDs.ops) {...} statement's "then" block.  If Init is called
exactly once for a given cm, then why have those if (!....ops) statements?

/be

Comment 24

[neeti]

Attaching a revised patch. Removed nsFactoryTableEntry::mCID. Also moved 
SetAlphaBounds within the (!mContractIDs.ops) {...} statement, since the Init() 
method could be called more than once.

Comment 25

[neeti]

Attachment: removed mCID from nsFactoryTableEntry

Comment 26

[Brendan Eich [:brendan]]

neeti: great!  But now k=3 for nsContractIDTableEntry (it saves 3 words wasted
with chaining [bucket, key, next]), so you should change

        PL_DHashTableSetAlphaBounds(&mContractIDs,
                                    0.875,
                                    PL_DHASH_MIN_ALPHA(&mContractIDs, 2));

to

        // Minimum alpha uses k=3 because nsContractIDTableEntry saves three
        // words compared to what a chained hash table requires.
        PL_DHashTableSetAlphaBounds(&mContractIDs,
                                    0.875,
                                    PL_DHASH_MIN_ALPHA(&mContractIDs, 3));

Note the comment.

/be

Comment 27

[neeti]

Brendan: could you explain how k=3 for the nsContractIDTableEntry? We have the 
key and value pointers separately for the nsContractIDTableEntry. 

The k value for the nsFactoryTableEntry changed from k=2 to k=3, because we got 
rid of the key nsFactoryTableEntry::mCID.

Comment 28

[Brendan Eich [:brendan]]

Neeti: right you are, I confused the two!  Sorry.  But my confusion stemmed from
the lack of a PL_DHashTableSetAlphaBounds call for the mFactories table.  Would
you add one, with k=3?  The k=2 for mContractIDs in your last patch should
stand, of course.  Thanks,

/be

Comment 29

[neeti]

Attachment: added PL_DHashTableSetAlphaBounds(..) for mFactories table with k=3

Comment 30

[Brendan Eich [:brendan]]

While reviewing the enumerator changes, I noticed that we've lost thread safety
by switching from nsObjectHashtable (constructed with the threadSafe parameter
set to PR_TRUE) to PLDHashTable.

Neeti, it should suffice to use a lock around calls into PL_DHashTableOperate
and PL_DHashTableEnumerate (Init, SetAlphaBounds, and Finish should run on the
main thread only).  We could use mMon for now, but I think a sequel bug should
be filed asking that nsComponentManager.cpp use at most a PRLock (with
nsAutoLock wrapping as appropriate), not a PRMonitor.

/be

Comment 31

[Mike Shaver (:shaver emeritus)]

*** Bug 46832 has been marked as a duplicate of this bug. ***

Comment 32

[neeti]

Added locks around calls to into PL_DHashTableOperate and 
PL_DHashTableEnumerate.

Comment 33

[neeti]

Attachment: Added locks around calls to into PL_DHashTableOperate and PL_DHashTableEnumerate

Comment 34

[neeti]

brendan: bug 13009 deals with too many locks in the service manager, now merged 
with the component manager.

Comment 35

[Brendan Eich [:brendan]]

Comment on attachment 52531 [details] [diff] [review]
Added locks around calls to into PL_DHashTableOperate and PL_DHashTableEnumerate

You must hold the monitor or lock until you're done inspecting and updating
the entry addressed by the returned PLDHashEntryHdr pointer.  So, for instance,
this is unsafe:

+    nsFactoryEntry *entry = nsnull;
+    PR_EnterMonitor(mMon);
+    nsContractIDTableEntry* contractIDTableEntry =
+        NS_STATIC_CAST(nsContractIDTableEntry*,
+                       PL_DHashTableOperate(&mContractIDs, aContractID,
+                                            PL_DHASH_LOOKUP));
+    PR_ExitMonitor(mMon);
+  
+    if (PL_DHASH_ENTRY_IS_BUSY(contractIDTableEntry)) {
+        entry = contractIDTableEntry->mFactoryEntry;
+    }
+

You need:

+    nsFactoryEntry *entry = nsnull;
+    PR_EnterMonitor(mMon);
+    nsContractIDTableEntry* contractIDTableEntry =
+        NS_STATIC_CAST(nsContractIDTableEntry*,
+                       PL_DHashTableOperate(&mContractIDs, aContractID,
+                                            PL_DHASH_LOOKUP));
+  
+    if (PL_DHASH_ENTRY_IS_BUSY(contractIDTableEntry)) {
+        entry = contractIDTableEntry->mFactoryEntry;
+    }
+
+    PR_ExitMonitor(mMon);

In general, if there are early returns or other unbalanced control flow, use an
nsAutoMonitor.  To keep the code and its rules simplest, you might always use
an nsAutoMonitor, but be sure to enclose it with a block scope to keep it from
holding the monitor too long, if a case arises where the automonitor would be
in scope all the way till end of function or method, and there are non-critical
sections, calls out of module, etc. that don't want or need this locking.

The following enumerates mFactories twice -- shouldn't the second Enumerate be
on mContractIDs?  But then it needs a different enumerator callback function,
as the entries in mContractIDs are not of type nsFactoryTableEntry, they are
of type nsContractIDTableEntry.

 nsresult 
 nsComponentManagerImpl::FreeServices()
 {
-    if (mFactories)
-        mFactories->Enumerate(FreeServiceEntry);
+    nsAutoMonitor mon(mMon);
+
+    if (mFactories.ops)
+        PL_DHashTableEnumerate(&mFactories, FreeServiceEntryEnumerate, nsnull);
 
-    if (mContractIDs)
-        mContractIDs->Enumerate(FreeServiceEntry);
+    if (mContractIDs.ops)
+        PL_DHashTableEnumerate(&mFactories, FreeServiceEntryEnumerate, nsnull);
 
     return NS_OK;
 }

/be

Comment 36

[neeti]

Attachment: revised patch using nsAutoMonitor

Comment 37

[neeti]

Attached a new patch with all the changes mentioned above.

Comment 38

[neeti]

Attachment: changed location member of nsFactoryEntry from nsCString to char*

Comment 39

[Brendan Eich [:brendan]]

Comment on attachment 51982 [details] [diff] [review]
best {js,pl}dhash.[ch] patch: get minAlpha clamping right, provide PL_DHASH_MIN_ALPHA macro for users

Patch has moved, see bug 103990.

/be

Comment 40

[neeti]

Attachment: updated patch to the current tree

Comment 41

[neeti]

Attachment: updated patch with the current tree

Comment 42

[Suresh Duddi (gone)]

Not a huge perf win for startup per neeti. Probably a memory win. Taking it off
startup perf tracking bug.

Comment 43

[Brendan Eich [:brendan]]

Comment on attachment 53943 [details] [diff] [review]
updated patch with the current tree

Nit: underhanging indentation is off here:

+PLDHashOperator PR_CALLBACK
+FreeServiceFactoryEntryEnumerate(PLDHashTable *aTable,
+                          PLDHashEntryHdr *aHdr,
+                          PRUint32 aNumber,
+                          void *aData)

I would put entry->mFactoryEntry in a local variable, for best perf and readability:

+    if (entry->mFactoryEntry->mServiceEntry) {
+        delete entry->mFactoryEntry->mServiceEntry;
+        entry->mFactoryEntry->mServiceEntry = nsnull;
+    }

Same comment for the next function.
Note for future reference: nesting monitor in RegisterService/GetFactoryEntry
will require a monitor, not merely a lock.
Fix by hand-inlining GetFactoryEntry?

The enumerator stuff looks ok to me, although I wonder why mCount is needed.
If it is, it seems to me it ought not be set to -1 on enumeration failure, as later code tests only !mCount.
Set it to 0, at the least.

/be

Comment 44

[neeti]

Attachment: updated patch with changes mentioned by Brendan

Comment 45

[Brendan Eich [:brendan]]

Comment on attachment 54006 [details] [diff] [review]
updated patch with changes mentioned by Brendan

Tabs have been added -- did your editor lose an expandtab option setting?

My comment about nested monitors was not meant to cause us to lose serialized registration.
Do we need to interlock RegisterFactory, etc., calls to avoid racing duplicates?
If so, hold the monitor across the GetFactoryEntry until the successful Put and initialization.
When we switch to a lock, we'll have to add a "placeholder" entry early, if GetFactoryEntry
doesn't find an entry.

Nit: make converter line up with the other members of its struct; ditto mContractID.

Instead of if (!aEnumerator) { NS_ASSERTION(0, ...); return NS_ERROR_NULL_POINTER; }
how about doing the assertion first, testing aEnumerator != nsnull.

Space nit: don't add spaces to an empty line:

-        
+    
     PR_ExitMonitor(mMon);
-        
+

/be

Comment 46

[neeti]

Yes, my editor did lose an expandtab option setting. Made the above changes, 
except for the nested monitor case across RegisterFactory/GetService calls.

Currently, we need a monitor in RegisterFactory, and we hold it across 
GetFactoryEntry until the successful Put and initialization. Also, we also need 
a monitor in GetFactoryEntry(), when we do a lookup in the hashtable lookup.
So, is the current code ok, until we switch to locks? Could you clarify?

thanks,
neeti

Comment 47

[neeti]

Attachment: revised patch

Comment 48

[Brendan Eich [:brendan]]

I just applied the patch, and don't see where, in RegisterFactory or
RegisterComponentCommon, the monitor is acquired and held in one critical
section that contains both the GetFactoryEntry and the conditional PL_DHASH_ADD.
 So it seems to me two Register calls could race, both decide that the entry was
not there, and both add -- and the second one to add would overwrite
factoryTableEntry->mFactoryEntry, leaking the entry created by the first one.

Otherwise looks ok.

/be

Comment 49

[neeti]

Attachment: patch updated with changes to RegisterFactory and RegisterCommonComponents

Comment 50

[Mike Shaver (:shaver emeritus)]

Comment on attachment 54185 [details] [diff] [review]
patch updated with changes to RegisterFactory and RegisterCommonComponents

>+    if (mContractIDs.ops)
>+        PL_DHashTableFinish(&mContractIDs);
>+    if (mFactories.ops)
>+        PL_DHashTableFinish(&mFactories);

For symmetry with Init, you should set .ops to nsnull after destruction.

With that, r=shaver.  Thanks for all the hard work!

Comment 51

[Brendan Eich [:brendan]]

Comment on attachment 54185 [details] [diff] [review]
patch updated with changes to RegisterFactory and RegisterCommonComponents

r=[e-mail]

The pre-existing code (not neeti's mods) is still slack (too much redundancy among the
::Register* methods, e.g.; lack of PRBool for checkRegistry [should be aCheckRegistry];
etc.), but that can wait for a later bug.

/be

Comment 52

[Doug Turner (:dougt)]

great work.  Lets get it into the trunk!

Comment 53

[Brendan Eich [:brendan]]

Comment on attachment 54185 [details] [diff] [review]
patch updated with changes to RegisterFactory and RegisterCommonComponents

Ah, shaver r='d.  I'll upgrade my r= to an sr= if you make the change shaver asked for.
No need for another patch attachment.

/be

Comment 54

[neeti]

fix checked in with the change shaver mentioned. Thanks brendan, shaver for your 
 reviews.

Comment 55

[kill this account]

I've been getting an abort when running the debugger on my Redhat 6.2 system:
nsComponentManagerImpl::Init calls PL_DHashTableSetAlphaBounds with
minAlpha of NaN.

#0  0x405bad41 in __kill () from /lib/libc.so.6
#1  0x4033a217 in raise (sig=6) at signals.c:65
#2  0x405bc0d8 in abort () at ../sysdeps/generic/abort.c:88
#3  0x402f8eb9 in PR_Assert (s=0x40273ce0 "0.5 <= maxAlpha && maxAlpha < 1 && 0 
<= minAlpha", file=0x40273ccc "pldhash.c", ln=227) at prlog.c:495
#4  0x4018c9cf in PL_DHashTableSetAlphaBounds (table=0x8072000, maxAlpha=0.875, 
minAlpha=-NaN(0x400000)) at pldhash.c:227
#5  0x401e2d88 in nsComponentManagerImpl::Init (this=0x8071fe0) at 
nsComponentManager.cpp:555
#6  0x4018bafc in NS_InitXPCOM2 (result=0x0, binDirectory=0x0, 
appFileLocationProvider=0x8072080) at nsXPComInit.cpp:340
#7  0x0805b15a in main (argc=1, argv=0xbffff814) at nsAppRunner.cpp:1612
#8  0x405b49cb in __libc_start_main (main=0x805afdc <main>, argc=1, 
argv=0xbffff814, init=0x8054220 <_init>, fini=0x8066a5c <_fini>, 
rtld_fini=0x4000ae60 <_dl_fini>, stack_end=0xbffff80c) at 
../sysdeps/generic/libc-start.c:92

I asked on #mozilla but other people are not having this problem.