Closed
Bug 96461
Opened 23 years ago
Closed 23 years ago
ComponentManager could PLHash or PLDHash
Categories
(Core :: XPCOM, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla0.9.6
People
(Reporter: dp, Assigned: neeti)
References
Details
(Keywords: perf)
Attachments
(13 files, 3 obsolete files)
32.77 KB,
patch
|
Details | Diff | Splinter Review | |
33.06 KB,
patch
|
Details | Diff | Splinter Review | |
36.21 KB,
patch
|
Details | Diff | Splinter Review | |
35.38 KB,
patch
|
Details | Diff | Splinter Review | |
35.83 KB,
patch
|
Details | Diff | Splinter Review | |
36.59 KB,
patch
|
Details | Diff | Splinter Review | |
38.53 KB,
patch
|
Details | Diff | Splinter Review | |
39.69 KB,
patch
|
Details | Diff | Splinter Review | |
39.22 KB,
patch
|
Details | Diff | Splinter Review | |
37.87 KB,
patch
|
Details | Diff | Splinter Review | |
37.83 KB,
patch
|
Details | Diff | Splinter Review | |
38.05 KB,
patch
|
Details | Diff | Splinter Review | |
38.27 KB,
patch
|
shaver
:
review+
brendan
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•23 years ago
|
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.
Reporter | ||
Updated•23 years ago
|
Priority: -- → P3
Reporter | ||
Comment 3•23 years ago
|
||
Doug, the new brave owner of xpcom has accepted to do these. Yeah! thanks doug.
Assignee: dp → dougt
Status: ASSIGNED → NEW
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•23 years ago
|
||
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
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.
Assignee | ||
Comment 10•23 years ago
|
||
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?
Assignee | ||
Comment 11•23 years ago
|
||
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•23 years ago
|
||
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.
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•23 years ago
|
||
lidl@pix.net, 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•23 years ago
|
||
Comment 16•23 years ago
|
||
Updated•23 years ago
|
Attachment #51923 -
Attachment is obsolete: true
Comment 17•23 years ago
|
||
Comment 18•23 years ago
|
||
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.
Attachment #51938 -
Attachment is obsolete: true
Comment 19•23 years ago
|
||
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
Status: NEW → ASSIGNED
Comment 20•23 years ago
|
||
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
Attachment #51880 -
Flags: needs-work+
Assignee | ||
Comment 21•23 years ago
|
||
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
Assignee | ||
Comment 22•23 years ago
|
||
Comment 23•23 years ago
|
||
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
Assignee | ||
Comment 24•23 years ago
|
||
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.
Assignee | ||
Comment 25•23 years ago
|
||
Comment 26•23 years ago
|
||
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
Assignee | ||
Comment 27•23 years ago
|
||
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•23 years ago
|
||
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
Assignee | ||
Comment 29•23 years ago
|
||
Comment 30•23 years ago
|
||
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
*** Bug 46832 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 32•23 years ago
|
||
Added locks around calls to into PL_DHashTableOperate and PL_DHashTableEnumerate.
Assignee | ||
Comment 33•23 years ago
|
||
Assignee | ||
Comment 34•23 years ago
|
||
brendan: bug 13009 deals with too many locks in the service manager, now merged with the component manager.
Comment 35•23 years ago
|
||
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
Assignee | ||
Comment 36•23 years ago
|
||
Assignee | ||
Comment 37•23 years ago
|
||
Attached a new patch with all the changes mentioned above.
Assignee | ||
Comment 38•23 years ago
|
||
Comment 39•23 years ago
|
||
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
Attachment #51982 -
Attachment is obsolete: true
Assignee | ||
Comment 40•23 years ago
|
||
Assignee | ||
Comment 41•23 years ago
|
||
Reporter | ||
Comment 42•23 years ago
|
||
Not a huge perf win for startup per neeti. Probably a memory win. Taking it off startup perf tracking bug.
No longer blocks: 7251
Comment 43•23 years ago
|
||
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
Attachment #53943 -
Flags: needs-work+
Assignee | ||
Comment 44•23 years ago
|
||
Comment 45•23 years ago
|
||
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
Attachment #54006 -
Flags: needs-work+
Assignee | ||
Comment 46•23 years ago
|
||
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
Assignee | ||
Comment 47•23 years ago
|
||
Comment 48•23 years ago
|
||
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
Assignee | ||
Comment 49•23 years ago
|
||
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!
Attachment #54185 -
Flags: review+
Comment 51•23 years ago
|
||
Comment on attachment 54185 [details] [diff] [review] patch updated with changes to RegisterFactory and RegisterCommonComponents r=brendan@mozilla.org 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•23 years ago
|
||
great work. Lets get it into the trunk!
Comment 53•23 years ago
|
||
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
Attachment #54185 -
Flags: superreview+
Assignee | ||
Comment 54•23 years ago
|
||
fix checked in with the change shaver mentioned. Thanks brendan, shaver for your reviews.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 55•23 years ago
|
||
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.
You need to log in
before you can comment on or make changes to this bug.
Description
•