Closed
Bug 384490
Opened 17 years ago
Closed 15 years ago
Remove nsHashTable from mailnews
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b2
People
(Reporter: mscott, Unassigned)
References
Details
Attachments
(11 files, 7 obsolete files)
512 bytes,
patch
|
mscott
:
superreview+
|
Details | Diff | Splinter Review |
25.83 KB,
patch
|
mscott
:
superreview+
|
Details | Diff | Splinter Review |
3.59 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
22.23 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
4.79 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
7.63 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
2.20 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
14.30 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
9.15 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
7.69 KB,
patch
|
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
6.71 KB,
patch
|
standard8
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
It looks like nsHashTable.h is not supported by frozen linkage (it includes nsString.h). We'll need to convert the callers in mailnews to something else like an implementation of nsTHashTable or one of its derived classes. It looks like there are 16 implementations of nsHashTable in mailnews: http://mxr.mozilla.org/seamonkey/search?string=nsHashTable.h Maybe prasad and I can work on this while we wait for biesi's code review for the nsINetUtil escape patch.
Comment 1•17 years ago
|
||
I'm happy to do the ones I'm responsible for using, especially the nsMsgDatabase ones, but also the nsMsgGroupView one, and the imap ones. Do we need to do the one in ldap as well?
Comment 2•17 years ago
|
||
the other ones probably won't be as easy :-)
Attachment #268424 -
Flags: superreview?(mscott)
Reporter | ||
Comment 3•17 years ago
|
||
Comment on attachment 268424 [details] [diff] [review] fix mailnews/db :)
Attachment #268424 -
Flags: superreview?(mscott) → superreview+
Reporter | ||
Comment 4•17 years ago
|
||
I'm fixing nsMsgIncomingServer. David's working on the hash tables in the imap code.
Updated•17 years ago
|
OS: Windows Vista → All
Hardware: PC → All
Comment 5•17 years ago
|
||
looks like I have some of Nick's NS_free cleanup in my tree and thus in the diff - sorry about that.
Attachment #268527 -
Flags: superreview?(mscott)
Reporter | ||
Comment 6•17 years ago
|
||
Comment on attachment 268527 [details] [diff] [review] [checked in] fix for imap code I see you remembered to initialize these hash tables which I forgot to do at first when fixing nsMsgIncomingServer :)
Attachment #268527 -
Flags: superreview?(mscott) → superreview+
Reporter | ||
Comment 7•17 years ago
|
||
This patch should behave the same as with nsHashTable. But I still haven't fully understood the eviction policy and how that relates to kMaxDownloadTableSize which is defined at 500 in nsMsgIncomingServer. The old hash table was initialized with a size of 16 (see the default constructor in nsHashTable.h). I bumped that up to 50 here, but that still doesn't seem large enough with this value of 500 floating around... I'll keep digging.
Comment 8•17 years ago
|
||
This code removes nsHashtable from addrbook code. I could not test if nsAbOutlookDirectory would compile because I don't have Windows! One of the hash tables in nsAbOutlookDirectory is declared to be of type nsClassHashtableMT as it is expected to be thread safe.
Attachment #268532 -
Flags: review?(mscott)
Comment 9•17 years ago
|
||
Oops! The previous patch had parts from other patch too. Cleaned it up now.
Attachment #268532 -
Attachment is obsolete: true
Attachment #268533 -
Flags: review?(mscott)
Attachment #268532 -
Flags: review?(mscott)
Updated•17 years ago
|
Attachment #268527 -
Attachment description: fix for imap code → [checked in] fix for imap code
Reporter | ||
Comment 10•17 years ago
|
||
Reporter | ||
Comment 11•17 years ago
|
||
Comment on attachment 268533 [details] [diff] [review] Removes nsHashtable from addrbook. asking Mark for a peer review on the address book changes. Mark, we're removing nsHashtable which isn't available using frozen linkage.
Attachment #268533 -
Flags: superreview?(mscott)
Attachment #268533 -
Flags: review?(mscott)
Attachment #268533 -
Flags: review?(bugzilla)
Reporter | ||
Comment 12•17 years ago
|
||
The only thing I'm not sure about here is if we should use a large value for the hash table for folder cache elements. Currently the default size is 16. for my account, I end up having 65 folder cache elements stored in the hash table. Not that I'm a typical user...
Attachment #268845 -
Flags: superreview?(bienvenu)
Reporter | ||
Comment 13•17 years ago
|
||
Comment on attachment 268831 [details] [diff] [review] [checked in]fix nsMsgAccountManager.cpp I've been running and testing with this patch since Friday morning.
Attachment #268831 -
Flags: superreview?(bienvenu)
Comment 14•17 years ago
|
||
Comment on attachment 268845 [details] [diff] [review] [checked in]fix nsMsgFolderCache I'll try this out - I have dark suspicions about getting rid of the code to set the folderCacheEl to the passed in key, when it's not null. I think it breaks the case where the imap folder name differs from the name of the .msf file on disk, after some brief experiments in the debugger...
Updated•17 years ago
|
Attachment #268831 -
Flags: superreview?(bienvenu) → superreview+
Comment 15•17 years ago
|
||
Comment on attachment 268845 [details] [diff] [review] [checked in]fix nsMsgFolderCache sorry, false alarm - I wasn't applying the diff correctly in my head :-)
Attachment #268845 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•17 years ago
|
Attachment #268831 -
Attachment description: fix nsMsgAccountManager.cpp → [checked in]fix nsMsgAccountManager.cpp
Reporter | ||
Updated•17 years ago
|
Attachment #268845 -
Attachment description: fix nsMsgFolderCache → [checked in]fix nsMsgFolderCache
Reporter | ||
Comment 16•17 years ago
|
||
Reporter | ||
Comment 17•17 years ago
|
||
Reporter | ||
Comment 18•17 years ago
|
||
I don't have oexpress on vista to actually test this against but the change was very simple.
Attachment #268898 -
Flags: superreview?(bienvenu)
Reporter | ||
Comment 19•17 years ago
|
||
Comment on attachment 268876 [details] [diff] [review] [checked in]fix mapi I verified that nsClassHashtable destroys the mapi session object when the hash table is destroyed.
Attachment #268876 -
Flags: superreview?(bienvenu)
Reporter | ||
Comment 20•17 years ago
|
||
Comment on attachment 268533 [details] [diff] [review] Removes nsHashtable from addrbook. I don't think you need to use nsVoidPtrHashKey, the hash table template should build the key automatically: + nsVoidPtrHashKey key((void *)childDir); + mServers.Put (&key, aServer); can just be: mServers.Put ((void *) childDir, aServer); * GetDirectories_getDirectory (const void *aKey, Shouldn't aKey be defined as nsVoidPtrHashKey * aKey? * I would have changed closure to be aClosure but that's a minor nit. +PR_STATIC_CALLBACK(PLDHashOperator) +GetDirectories_getDirectory (const void *aKey, nsAutoPtr<DIR_Server> &aData, void* closure) * Is this line really necessary? Can't we just use aData here directly? + DIR_Server* server = (DIR_Server*) aData; * if the definition of aKey is changed, then this line can go away: nsVoidPtrHashKey* voidKey = (nsVoidPtrHashKey* )aKey; and you can probably just inline the nsIAbdirectory cast so we are left with something like: getDirectories->directories->AppendElement(NS_STATIC_CAST(nsIAbDirectory *, aKey->GetKey())); * Again, + nsVoidPtrHashKey key((void *)directory); + DIR_Server *server; + mServers.Get(&key, &server); no need for nsVoidPtrHashKey here, just pass in directory directly. * no need to cast to a void * for getDirectories argument: + mServers.Enumerate (GetDirectories_getDirectory, (void *)&getDirectories); * + nsVoidPtrHashKey k((void *)d); + mServers.Remove(&k); can just be mServers.Remove(d); (you may need to cast d to void *) * I didn't realize you could do this to test for existence: - *hasCard = mCache.Exists (&key); + *hasCard = mCache.Get(&key, NULL); I learn something every day! I was testing against existence of the hash data. * Should this: + nsInterfaceHashtable<nsVoidPtrHashKey, nsISupports> mCache; be nsInterfaceHashtable<nsVoidPtrHashKey, nsIAbCard> mCache; Same for this one: + nsInterfaceHashtable<nsVoidPtrHashKey, nsISupports> mSearchCache; that also changes the argument for enumerateSearchCache and simplifies some code there as well. * there are more explicit uses of nsVoidPtrHashKey and casts to void * that I don't think we need. * + nsUint32HashKey newKey(*aReturnValue) ; mQueryThreads.Put(&newKey, newThread) ; can just be mQueryThreads.Put(*aReturnValue, newThread) ; same optimization for: nsUint32HashKey contextKey(aContext) ; + nsUint32HashKey hashKey(aThreadId) ; * this can be nsIAbCard instead of nsISupports: + nsInterfaceHashtable<nsVoidPtrHashKey, nsISupports> mCardList; Other than those small things, the patch looks good to me!
Attachment #268533 -
Flags: superreview?(mscott) → superreview-
Updated•17 years ago
|
Attachment #268876 -
Flags: superreview?(bienvenu) → superreview+
Updated•17 years ago
|
Attachment #268898 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•17 years ago
|
Attachment #268876 -
Attachment description: fix mapi → [checked in]fix mapi
Reporter | ||
Updated•17 years ago
|
Attachment #268898 -
Attachment description: fix nsOEAddressIterator → [checked in]fix nsOEAddressIterator
Comment 21•17 years ago
|
||
Comment on attachment 268533 [details] [diff] [review] Removes nsHashtable from addrbook. r- per Scott's comments. A couple of my own comments though: - nsHashtable mServers; + nsClassHashtable<nsVoidPtrHashKey,DIR_Server> mServers; For both this and mCache, does it work if we make nsVoidPtrHashKey a nsISupportsHashKey instead? I think this would avoid some casts. Should we be calling Init() on mCache and some of the others? I've not seen documentation that says we must.
Attachment #268533 -
Flags: review?(bugzilla) → review-
Reporter | ||
Updated•17 years ago
|
Attachment #268529 -
Flags: superreview?(bienvenu)
Updated•17 years ago
|
Attachment #268529 -
Flags: superreview?(bienvenu) → superreview+
Comment 22•17 years ago
|
||
Scott: for enumeration functions as the one below, the first argument has to be the KeyType which will turn out to be "void *" for VoidPtrHashKeys. GetDirectories_getDirectory (const void *aKey, nsAutoPtr<DIR_Server> &aData, void* closure) Mark: I changed all the hashtables for which interfaces were casted to "void *" and changed them to use nsISupportsHashKey instead of nsVoidPtrHashKey.
Attachment #268533 -
Attachment is obsolete: true
Attachment #268968 -
Flags: review?(bugzilla)
Reporter | ||
Comment 23•17 years ago
|
||
Comment on attachment 268871 [details] [diff] [review] fix nsMsgGroupView The old code used to use a variety of different hash keys based on the sort type. I used a HashString method to convert all of the string based hash keys into PRUint32 keys so our hash table has only one hash key type.
Attachment #268871 -
Flags: superreview?(bienvenu)
Comment 24•17 years ago
|
||
But, if two subject strings, for example, hash to the same value, they'll get put in the same group, i.e., I think this approach ignores hash collisions, whereas the old code didn't have this problem, I don't think.
Reporter | ||
Comment 25•17 years ago
|
||
Comment on attachment 268968 [details] [diff] [review] Removes nsHashtable from addrbook - Updated as per comments from Scott and Mark. I like the nsISupports change for the hash key! * Is this line necessary, can we just use aData directly? + DIR_Server* server = (DIR_Server*) aData; * This doesn't look right to me: + nsISupportsHashKey* key = (nsISupportsHashKey* )aKey; + getDirectories->directories->AppendElement((nsIAbDirectory *) key->GetKey()); I would have expected aKey to be the nsISupports object we used as the key which is a nsIAbDirectory. i.e. nsCOMPtr<nsIAbDirectory> abDir = do_QueryInterface(aKey); getDirectories->directories->AppendElement(abDir); or maybe even: getDirectories->directories->AppendElement(aKey); * NIT: add a space after the comma: + nsClassHashtable<nsISupportsHashKey,DIR_Server> mServers; * I don't think Init() is being called on all the hash tables in this patch. We'll get an error at run time if the hash table isn't initialized before it is used. * In nsAbOutlookDirectory, can this: nsInterfaceHashtable<nsVoidPtrHashKey, nsIAbCard> mCardList; use nsISupports as the key? That would save several casts later on. I can help test the outlook directory code before checking these changes in for you. sr=mscott if you can address those things + anything Mark comes up with :)
Attachment #268968 -
Flags: superreview+
Reporter | ||
Updated•17 years ago
|
Attachment #268529 -
Attachment description: fix nsMsgIncomingServer → [checked in]fix nsMsgIncomingServer
Comment 26•17 years ago
|
||
- Changed nsVoidPtrHashKey to nsISupportsHashKey in nsAbOutlookDirectory - Called Init for the hashtables that I missed earlier and made other changes suggested by scott.
Attachment #268968 -
Attachment is obsolete: true
Attachment #269069 -
Flags: superreview?(mscott)
Attachment #269069 -
Flags: review?(bugzilla)
Attachment #268968 -
Flags: review?(bugzilla)
Reporter | ||
Comment 27•17 years ago
|
||
Comment on attachment 269069 [details] [diff] [review] Removes nsHashtable from addrbook - Version 3 Thanks Prasad. I can land this one Mark gives it his ok.
Attachment #269069 -
Flags: superreview?(mscott) → superreview+
Reporter | ||
Comment 28•17 years ago
|
||
Comment on attachment 269069 [details] [diff] [review] Removes nsHashtable from addrbook - Version 3 oops, actually I ran into a problem running with the patch. nsClassHashtable is destructive, it deletes the objects in it when it loses scope. When I click on a message that has remote content, we look up the sender in the address book to see if we should load the content. I get a crash in nsAbBSDirectory because nsAbBSDirectory::mServers lost scope, deleted the DIR_Server objects out from underneath the void array of DIR_Servers managed by nsDirPrefs.cpp
Attachment #269069 -
Flags: superreview+ → superreview-
Reporter | ||
Comment 29•17 years ago
|
||
(In reply to comment #24) > But, if two subject strings, for example, hash to the same value, they'll get > put in the same group, i.e., I think this approach ignores hash collisions, > whereas the old code didn't have this problem, I don't think. Good catch David. One possibility is to use nsStrings as the hash key. Converting integers into strings and converting nsCStrings to nsStrings before using them as hash keys.
Reporter | ||
Updated•17 years ago
|
Attachment #268871 -
Attachment is obsolete: true
Attachment #268871 -
Flags: superreview?(bienvenu)
Reporter | ||
Comment 30•17 years ago
|
||
this patch uses nsString as the hash key, converting integer values into strings.
Comment 31•17 years ago
|
||
Comment on attachment 269069 [details] [diff] [review] Removes nsHashtable from addrbook - Version 3 On the whole this looks much better, but per Scott's comments its an r-. I don't know if you've already looked at a way of getting round Scott's problems. One idea that I do have is that the list of servers contained within nsAbBSDirectory::mServers is basically the same as that within nsDirPrefs (dir_ServerList). Indeed, I think they should be exactly the same. So maybe the solution is to make nsAbBSDirectory use nsDirPrefs's version/copy. I've not looked at it in detail, but it'll probably be a better solution.
Attachment #269069 -
Flags: review?(bugzilla) → review-
Reporter | ||
Comment 32•17 years ago
|
||
Comment on attachment 269253 [details] [diff] [review] fix nsMsgGroupView This approach should fix the collision issue David caught in the previous hash table.
Attachment #269253 -
Flags: superreview?(bienvenu)
Comment 33•17 years ago
|
||
Comment on attachment 269253 [details] [diff] [review] fix nsMsgGroupView I think this one might be sticking raw int values into the string, and I'm not sure how well that will work with 0. But you could always do the val + '0' trick to turn them into strings.
Attachment #269253 -
Flags: superreview?(bienvenu) → superreview-
Reporter | ||
Comment 34•17 years ago
|
||
I've been running this patch for almost a week now. This uses AppendInt instead of assigning the integer into the string.
Attachment #269253 -
Attachment is obsolete: true
Attachment #270615 -
Flags: superreview?(bienvenu)
Updated•17 years ago
|
Attachment #270615 -
Flags: superreview?(bienvenu) → superreview+
Reporter | ||
Updated•17 years ago
|
Attachment #270615 -
Attachment description: updated patch for nsMsgGroupView → [checked in] updated patch for nsMsgGroupView
Comment 35•17 years ago
|
||
This is a part of the full addrbook patch. Mark, can you review it?
Attachment #269069 -
Attachment is obsolete: true
Attachment #291008 -
Flags: review?(bugzilla)
Comment 36•17 years ago
|
||
Comment on attachment 291008 [details] [diff] [review] [checked in]Patch for nsAbLDAPDirectory and nsAbMDBDirectory r=me, thanks prasad.
Attachment #291008 -
Flags: review?(bugzilla) → review+
Comment 37•17 years ago
|
||
Comment on attachment 291008 [details] [diff] [review] [checked in]Patch for nsAbLDAPDirectory and nsAbMDBDirectory David, can you super-review this patch?
Attachment #291008 -
Flags: superreview?(bienvenu)
Comment 38•17 years ago
|
||
Comment on attachment 291008 [details] [diff] [review] [checked in]Patch for nsAbLDAPDirectory and nsAbMDBDirectory thx, Prasad, sr=bienvenu, with the nit - can you use nsnull instead of NULL?
Attachment #291008 -
Flags: superreview?(bienvenu) → superreview+
Comment 39•17 years ago
|
||
Comment on attachment 291008 [details] [diff] [review] [checked in]Patch for nsAbLDAPDirectory and nsAbMDBDirectory I've checked this in on Prasad's behalf with NULL changed to nsnull as per David's comment.
Attachment #291008 -
Attachment description: Patch for nsAbLDAPDirectory and nsAbMDBDirectory → [checked in]Patch for nsAbLDAPDirectory and nsAbMDBDirectory
Comment 40•17 years ago
|
||
(In reply to comment #28) > (From update of attachment 269069 [details] [diff] [review]) > oops, actually I ran into a problem running with the patch. > nsClassHashtable is destructive, it deletes the objects in it when it loses > scope. > When I click on a message that has remote content, we look up the sender in the > address book to see if we should load the content. I get a crash in > nsAbBSDirectory because nsAbBSDirectory::mServers lost scope, deleted the > DIR_Server objects out from underneath the void array of DIR_Servers managed by > nsDirPrefs.cpp Prasad, I've just had another look at this patch on the nsAbBSDirectory. The ideal solution would be to rework nsDirPrefs/nsAbBSDirectory so this "sharing" wouldn't be necessary. I'm don't think we're ready to do that yet. So, my suggestion would be how about using: nsDataHashtable<nsISupportsHashKey, DIR_Server*> mServers; i.e. storing a data set of pointers - its essentially what we do at the moment. So I think it may get around the destruction issues. Want to give it a try?
Comment 41•17 years ago
|
||
Sure, I can do this. I will fix this using the DataHashtable for now. I have also been trying to rework nsAbBSDirectory - will do that after the frozen linkage migration.
Comment 42•17 years ago
|
||
This is the patch for nsAbBSDirectory. Used nsDataHashtable instead of nsClassHashtable and it seems to work fine (i.e the objects are not being destroyed as they were when using the nsClassHashtable.
Attachment #293632 -
Flags: review?(bugzilla)
Comment 43•17 years ago
|
||
Comment on attachment 293632 [details] [diff] [review] Patch for nsAbBSDirectory There's some instances where we have unnecessary spaces before items e.g. +GetDirectories_getDirectory (nsISupports *aKey, DIR_Server* &aData, void* aClosure) + mServers.Enumerate (GetDirectories_getDirectory, (void *)&getDirectories); + getDirectories.directories->GetElementAt (i, getter_AddRefs(d)); I think its mainly legacy code, but if you could tidy those up as you're touching these lines, that'd be great. r=me with the spaces fixed.
Attachment #293632 -
Flags: review?(bugzilla) → review+
Comment 44•17 years ago
|
||
Fixed the spaces as per Mark's comments. r=Mark
Attachment #293632 -
Attachment is obsolete: true
Attachment #293873 -
Flags: superreview?(bienvenu)
Comment 45•16 years ago
|
||
Comment on attachment 293873 [details] [diff] [review] [checked in] Patch for nsAbBSDirectory thx, Prasad.
Attachment #293873 -
Flags: superreview?(bienvenu) → superreview+
Updated•16 years ago
|
Attachment #293873 -
Attachment description: Patch for nsAbBSDirectory → [checked in] Patch for nsAbBSDirectory
Assignee | ||
Updated•16 years ago
|
Product: Core → MailNews Core
Updated•16 years ago
|
Assignee: mscott → nobody
Comment 46•15 years ago
|
||
How much more is needed for this bug? (I'm not sure if it can be marked fixed yet) Nominating wanted-thunderbird3? due to the abundance of patches that have already been checked in.
Flags: wanted-thunderbird3?
Comment 47•15 years ago
|
||
(In reply to comment #46) > How much more is needed for this bug? (I'm not sure if it can be marked fixed > yet) Easy, mxr knows: http://mxr.mozilla.org/comm-central/search?string=nsHashTable&find=/mailnews One instance remaining in the outlook address book code and a couple of unneeded includes. Maybe Neil might like to take a stab at removing it.
Comment 48•15 years ago
|
||
Attachment #359061 -
Flags: superreview?(bienvenu)
Attachment #359061 -
Flags: review?(bugzilla)
Updated•15 years ago
|
Attachment #359061 -
Flags: superreview?(bienvenu) → superreview+
Updated•15 years ago
|
Attachment #359061 -
Flags: review?(bugzilla) → review+
Comment 49•15 years ago
|
||
Comment on attachment 359061 [details] [diff] [review] Patch for nsAbOutlookDirectory nit: I noticed several whitespace (end of line) issues whilst looking at this patch. I've not tested it, I'm assuming you have. r=me with the nits fixed and that assumption being true.
Comment 50•15 years ago
|
||
Pushed changeset edb3e85438a6 to comm-central. I assume this means that this bug is now fixed, so resolving as such.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Updated•15 years ago
|
Flags: wanted-thunderbird3?
Target Milestone: --- → Thunderbird 3.0b2
You need to log in
before you can comment on or make changes to this bug.
Description
•