Closed Bug 384490 Opened 17 years ago Closed 15 years ago

Remove nsHashTable from mailnews

Categories

(MailNews Core :: Backend, defect)

defect
Not set
normal

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.
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?
Attached patch fix mailnews/dbSplinter Review
the other ones probably won't be as easy :-)
Attachment #268424 - Flags: superreview?(mscott)
Comment on attachment 268424 [details] [diff] [review]
fix mailnews/db

:)
Attachment #268424 - Flags: superreview?(mscott) → superreview+
I'm fixing nsMsgIncomingServer.

David's working on the hash tables in the imap code.
OS: Windows Vista → All
Hardware: PC → All
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)
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+
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.
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)
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)
Attachment #268527 - Attachment description: fix for imap code → [checked in] fix for imap code
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)
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)
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 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...
Attachment #268831 - Flags: superreview?(bienvenu) → superreview+
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+
Attachment #268831 - Attachment description: fix nsMsgAccountManager.cpp → [checked in]fix nsMsgAccountManager.cpp
Attachment #268845 - Attachment description: fix nsMsgFolderCache → [checked in]fix nsMsgFolderCache
Attached patch fix nsMsgGroupView (obsolete) — Splinter Review
I don't have oexpress on vista to actually test this against but the change was very simple.
Attachment #268898 - Flags: superreview?(bienvenu)
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)
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-
Attachment #268876 - Flags: superreview?(bienvenu) → superreview+
Attachment #268898 - Flags: superreview?(bienvenu) → superreview+
Attachment #268876 - Attachment description: fix mapi → [checked in]fix mapi
Attachment #268898 - Attachment description: fix nsOEAddressIterator → [checked in]fix nsOEAddressIterator
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-
Attachment #268529 - Flags: superreview?(bienvenu)
Attachment #268529 - Flags: superreview?(bienvenu) → superreview+
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)
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)
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. 
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+
Attachment #268529 - Attachment description: fix nsMsgIncomingServer → [checked in]fix nsMsgIncomingServer
- 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)
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+
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-
(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. 


Attachment #268871 - Attachment is obsolete: true
Attachment #268871 - Flags: superreview?(bienvenu)
Attached patch fix nsMsgGroupView (obsolete) — Splinter Review
this patch uses nsString as the hash key, converting integer values into strings.
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-
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 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-
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)
Attachment #270615 - Flags: superreview?(bienvenu) → superreview+
Attachment #270615 - Attachment description: updated patch for nsMsgGroupView → [checked in] updated patch for nsMsgGroupView
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 on attachment 291008 [details] [diff] [review]
[checked in]Patch for nsAbLDAPDirectory and nsAbMDBDirectory

r=me, thanks prasad.
Attachment #291008 - Flags: review?(bugzilla) → review+
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 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 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
(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?
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.
Attached patch Patch for nsAbBSDirectory (obsolete) — Splinter Review
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 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+
Fixed the spaces as per Mark's comments.
r=Mark
Attachment #293632 - Attachment is obsolete: true
Attachment #293873 - Flags: superreview?(bienvenu)
Comment on attachment 293873 [details] [diff] [review]
[checked in] Patch for nsAbBSDirectory

thx, Prasad.
Attachment #293873 - Flags: superreview?(bienvenu) → superreview+
Attachment #293873 - Attachment description: Patch for nsAbBSDirectory → [checked in] Patch for nsAbBSDirectory
Product: Core → MailNews Core
Assignee: mscott → nobody
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?
(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.
Attachment #359061 - Flags: superreview?(bienvenu)
Attachment #359061 - Flags: review?(bugzilla)
Attachment #359061 - Flags: superreview?(bienvenu) → superreview+
Attachment #359061 - Flags: review?(bugzilla) → review+
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.
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
Flags: wanted-thunderbird3?
Target Milestone: --- → Thunderbird 3.0b2
You need to log in before you can comment on or make changes to this bug.