95685 - BLOAT: Reduce bloat by closing the db if the folder is unselected

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[Navin Gupta]

If the folder is selected and the user goes on to the next folder, we should 
close the previous folder's db. We should leave inbox as the special folder 
because of biff. This may help us to cut down on bloat drastically but there
may be a small price to pay in performance. 

Another appoach is to have at any time 5 open db(s).

Comment 1

[Håkan Waara]

Would be helpful to maybe check just how small that pay would be.

Comment 2

[Navin Gupta]

Attachment: proposed fix

Comment 3

[Navin Gupta]

So I was trying the first approach but since we depend upon the refcount to go
zero and it never looks like it does, this patch will not help. Any suggestions 
?

Comment 4

[Håkan Waara]

Set the comptr to nsnull maybe.

Comment 5

[David :Bienvenu]

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

Comment 6

[David :Bienvenu]

I'll take this for now - I know part of the problem, which is that the db keeps
a cache of the most recently used headers so it doesn't have to recreate them.
This code needs to ask the db to clear that cache in order to get the refs on
the db to go to 0. Re hwaara's comments, I think that's what this patch already
does. However, I don't think the code in SetMsgDatabase is doing the right thing
with listeners in the case of a null nsMsgDatabase, but I'll look at that more
closely as well.

Comment 7

[David :Bienvenu]

Attachment: new db interface to release cached headers

Comment 8

[David :Bienvenu]

Attachment: slight mod to Navin's patch (call my new db method)

Comment 9

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

sr=sspitzer

Comment 10

[David :Bienvenu]

this is pretty much fixed - there are some cases where we don't close the db
(e.g., an imap folder with headers to download doesn't get closed when you click
on another folder). I'm working on the exceptions but the db gets closed for the
most part.

Comment 11

[[e-mail] (gone - use [e-mail] instead)]

From bug 93013:

------- Additional Comments From Stephen Donner 2001-10-10 12:29 -------

Build        |Mem Start   |Mem Done    |Mem Shutdown
-----------------------------------------------------
0.9.2        |106056k     |155532k     |86676k
0.9.4        |103212k     |151416k     |81018k
Trunk        |133416k     |159112k     |111712k

Okay, you're right, David ;-) 

With 0.9.2 and 0.9.4, we had twice as much memory being allocated when going
from folder to folder:

49476k - 0.9.2 (difference from mem done minus mem start)
48204k - 0.9.4                     " 
25696k - Trunk                     "

Also of interest is that 0.9.2 doesn't have the outliner widget in the folder
pane, whereas 0.9.4 and the trunk of course do (there is around a meg of
difference between those two builds' memory usage). 

Verified FIXED

Comment 12

[Navin Gupta]

I am not seeing the ~nsMsgDatabase() get called on switching the folders
someone is still holding onto the db and this happens for pop/imap by just
switching folders. There is also a strong circular reference between db and 
folder which needs to be broken. We can do that by setting to null or weak 
ptr. 

Reopening for futher investigation..  

Comment 13

[David :Bienvenu]

Navin, it's js that's holding onto the db. Try selecting a message in the second
folder and waiting - you'll see the db closed then (or maybe after waiting two
seconds since they've made some changes in when gc is run since I last fixed
this). Also, the link is broken when you select the second folder, so there
isn't a cycle.

Comment 14

[David :Bienvenu]

my mistake - this has indeed been broken, I believe because of the single thread
cache. I'll attach a fix shortly.

Comment 15

[David :Bienvenu]

Attachment: clear cached thread when folder tries to clear db.

Comment 16

[David :Bienvenu]

Navin, thanks for catching this. I don't think it'll help your search view
problem since in theory search won't be displaying any threads, but it'll help
other things.

Comment 17

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

Comment on attachment 59939 [details] [diff] [review]
clear cached thread when folder tries to clear db.

sr=sspitzer

Comment 18

[Navin Gupta]

r=naving, this is a major item for bloat reduction, thanks for fixing it. 

Comment 19

[David :Bienvenu]

fix checked in.

Comment 20

[[e-mail] (gone - use [e-mail] instead)]

11-30 and 12-06 comparisons:

11-30 build:

Message Count | Memory Usage | Offset
-------------------------------------
Initial State | 32,088K      | 0
1,000         | 33,452K      | +1364
2,192         | 34,516K      | +1064
2,192         | 35,816K      | +300
1,000         | 35,816K      | 0

12-06 build:

Message Count | Memory Usage | Offset
-------------------------------------
Initial State | 42,912K      | 0
1,000         | 43,528K      | +616
2,192         | 44,300K      | +772
2,192         | 45,136K      | +836
1,000         | 45,140K      | +4

I know we leak in other places, but clearly, this seems to have cut our bloat
nearly in 1/2 (in some places, lower in others).  Guys, is this safe to be
verified?  Should I/we just spin individual leak bugs off?  Do you think we got
all the bloat?  Thanks...

Comment 21

[Navin Gupta]

I still see the gc calling the destructor only 1 out of 10 times when I switch 
folder but ya we can spin of a new bug. 

Comment 22

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED.  Navin, if you wouldn't mind opening a new bug on the garbage
collection issue, I'd appreciate it (I wouldn't know exactly how to file it). 
Thanks.

Comment 23

[David :Bienvenu]

OK, I'm going to spin off a new bug for the remaining issue, which is that when
messages are moved into target folders (definitely imap, haven't tried local),
then the target db's ref-count doesn't go to 0. But what I've noticed is that
the db's for folders you move/copy messages into stay open. See bug 114487

Comment 24

[Michael Buckland]

Adding to status whiteboard AOLTW and plussing bugs I know are required for
6.2.2.  Adding AOLTW to status whiteboard, but with no plus to bugs that are on
the risk analysis list, but have been questioned for a public release.

Comment 25

[Michael Buckland]

Oops should be 6.2.1 in my comment above. Sorry for typo.

Comment 26

[Michael Buckland]

Removed AOLTW