105192 - creating (and freeing) 512 item hashtable (about 6k) on every message copy (including copy to sent folder)

Status: NEW

(MailNews Core :: Backend, defect)

Description

[(not reading, please use [e-mail] instead)]

creating (and freeing) 512 item hashtable (about 6k) on every message copy 
(including copy to sent folder)

cavin and I found this while working on folder loading perf.

CreateNewHdr() calls CreateMsgHdr(), which will call AddHdrToCache(), which 
will create the hash table.

if we add a flag to CreateMsgHdr() to decide if we want to cache it or not, we 
can avoid the allocating and freeing (and hash table code) in the new hdr case.

Comment 1

[(not reading, please use [e-mail] instead)]

we'd need to fix this before checking in cavin's fix to use the total number of 
message in the folder as the value for the cache header size.

Comment 2

[(not reading, please use [e-mail] instead)]

according to cavin, this doesn't happen on every copy, just the first one.

after the first one, the hash table of cached headers is created, so we don't
create it again (for that destination folder, unless the hash table is removed,
which may happen if you open and close the folder, I'm not sure.)

I still think that we should consider not caching the hdr that we copy, to avoid
this code all together.

Comment 3

[Cavin Song]

> I still think that we should consider not caching the hdr that we copy, to avoid
> this code all together.
>
I agree and it will avoid wasting time and memory in the case where you copy a
msg to a few folders which contain a lot of msgs (500+) and you never open any
of  these folders. Msg filtering will fall into this category.

To fix this problem, in nsMsgDatabase::CopyHdrFromExistingHdr() we can set
'm_bCacheHeaders' to false before calling CreateNewHdr() and then set
'm_bCacheHeaders' back to true after CreateNewHdr() returns. This is because
'm_bCacheHeaders' controls whether or not msg headers are to be added to the
hash table in nsMsgDatabase::AddHdrToCache().

Comment 4

[David :Bienvenu]

I don't think we need to fix this for Cavin's change to work - he's only going
to call his routine to increase the size of the cache in the view code, which
means copying won't get involved. And since that's the case, I'd rather not
complicate this anymore by messing with a "addtocache" flag.

Comment 5

[scottputterman]

moving to future based on bienvenu's comments.  If we decide this is more
important, let's move it back.

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to comment #4)
> I don't think we need to fix this for Cavin's change to work - he's only going
> to call his routine to increase the size of the cache in the view code, which
> means copying won't get involved. And since that's the case, I'd rather not
> complicate this anymore by messing with a "addtocache" flag.

David, was Cavin's cache change completed?  And is this "addtocache" flag idea a  keeper?

Comment 7

[David :Bienvenu]

We're still creating the 512 element cache when you move/copy a message to a folder who's db is not open, because we're opening the db. But it seems like a drop in the bucket compared to all the other things that happen when we open a db so that the bigger win might be to figure out a way to avoid opening and closing the same db on multiple move/copies to the same target (perhaps lazily closing db's based on an LRU scheme as Wayne suggests in bug 347837.)