437657 - increase autocompact threshold "mail.purge_threshhold"

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Tuukka Tolvanen (sp3000)]

http://mxr.mozilla.org/mozilla/search?string=pref("mail.purge_threshhold
/penelope/base/prefs/all-penelope.js (View change log or blame annotations)
    * line 4 -- pref("mail.purge_threshhold", 10000);
/mailnews/mailnews.js (View change log or blame annotations)
    * line 282 -- pref("mail.purge_threshhold", 100);

Come on, who today wants to free 100kB of disk space ;) ...writing what would tend to be tens if not hundreds of MB each time. I tend to use 10M like penelope, but 1M might be sane enough for now.

Comment 1

[Magnus Melin [:mkmelin]]

Agreed 100KB is way too small. Even 1MB sounds a quite small. On the other hand, 10MB might be too much - know a lot of ISPs offer only 10MB storage. 5MB perhaps? (And make the label MB of course.) 

Comment 2

[Daniel Veditz [:dveditz]]

Remember this is mail people think they've deleted. Unless you've got graphic attachments 1Mb of text mail is a lot. 10MB is definitely too much.

Comment 3

[Steve Simms]

xref bug 379103

Comment 4

[klonos]

Some think 100k is too small ,others say 10M is too much... then why not make it a user configurable option (perhaps with a default value set)? This way all should be happy.

Comment 5

[Tuukka Tolvanen (sp3000)]

it is a pref; it even has ui. mnyromyr was volunteered to render a seamonkey opinion on whether this could be tweaked up in the shared conf. I guess to something conservative like 1000 kB.

Comment 6

[Karsten Düsterloh]

"Getting volunteered" is new to me, but who am I to disagree with the natives... ;-)

(In reply to comment #1)
> (And make the label MB of course.) 

That's not that easy, you'd need to decouple the value shown in the UI from the value stored in the prefs then. But using MB in the UI seems more useful these days, agreed.
(We can't just switch the meaning of a pref number by factor 1024!)

I agree that 1000 KB (in the current UI) or even 1024 KB (when shown as 1 MB) would a reasonable new value.

Comment 7

[Phil Ringnalda (:philor)]

The only problem I see with doing the UI in MB is that we're using a numberbox, so if someone nudged up our 100KB to 200KB in Tb2, in Tb3 we'll be telling them in the UI that they have it set to 1MB when they actually have it set to 0.1953125MB. I guess we could be slightly obnoxious, and have the copy-paste of the readCacheSize function (which currently has the same problem) just set the pref value when it reads a value that's not an even number of MB.

Comment 10

[David :Bienvenu]

We could use a new pref in MB, migrate the old pref, default the new pref to 10MB, and change the default to have autocompact on. A new pref avoids problems going back and forth between versions.

Comment 11

[Andrei Boros]

I suggest make the default limit larger even. Something like 50 or 100MB. (AFAIK 50MB is the default cache size of Firefox). 

Anyway, in today's world, single emails (with some attachments will) easily go larger then 10MB, so such a default limit could trigger a compact after deleting even just every 2-3 messages... That would be annoying too.

Comment 12

[David :Bienvenu]

Attachment: turn on autocompact by default, switch to MB

This patch does several things:

Turn on autocompact by default
For TB, migrate the pref to MB, and migrate old pref
Fix the options UI to use the new pref in MB
switch the default to 20MB, if the user hasn't changed the threshold.

This leaves SM using the old pref, but if they fix their preferences UI to use MB's, we can turn on the backend code for them.

Comment 13

[Wayne Mery (:wsmwk)]

nice plan! in the absence of hard data, 20MB sounds as good as anything and 5MB is a reasonable minimum. 


(In reply to comment #11)
> I suggest make the default limit larger even. Something like 50 or 100MB.
> (AFAIK 50MB is the default cache size of Firefox). 
> 
> Anyway, in today's world, single emails (with some attachments will) easily go
> larger then 10MB, so such a default limit could trigger a compact after
> deleting even just every 2-3 messages... That would be annoying too.

Larger may indeed be better, but we have no data/studies on which to base a number.  Bienvenu, we can change the default value at any time if a better number is devised, correct?

As for frequency, auto compact limits itself to one compact per hour.

Comment 14

[David :Bienvenu]

(In reply to comment #13)
> nice plan! in the absence of hard data, 20MB sounds as good as anything and 5MB
> is a reasonable minimum. 
1MB is the minimum
> 
> Larger may indeed be better, but we have no data/studies on which to base a
> number.  Bienvenu, we can change the default value at any time if a better
> number is devised, correct?
Yes, we can change it.

> As for frequency, auto compact limits itself to one compact per hour.
Right, that's an important point (once per hour, but the clock restarts whenever you start the app up)

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 503351 [details] [diff] [review]
turn on autocompact by default, switch to MB

>diff --git a/mailnews/mailnews.js b/mailnews/mailnews.js

>-pref("mail.purge_threshhold",              100);
>-pref("mail.prompt_purge_threshhold",       false);
>+pref("mail.purge_threshhold",              20480);

I'm not we need really need to keep mail.purge_threshold for Thunderbird now. If the preference doesn't exist, then we can detect that and just set the default to 20480. If the preference does exist then we can use its value appropriately. Hence we can just ifdef MOZ_SUITE on it.

> nsMsgDBFolder::GetPurgeThreshold(PRInt32 *aThreshold)

So in removing the old pref you'll need to rearrange this function a bit as rv won't be success if the pref doesn't exist.

It'd also be nice if we could avoid attempting to get the old value each time if we've already migrated.

Comment 16

[David :Bienvenu]

Attachment: fix addressing comments

Comment 17

[Mark Banner (:standard8)]

Comment on attachment 509585 [details] [diff] [review]
fix addressing comments

I'm having a bit of trouble testing this. How often should the auto-compact check kick in? I tried changing one value that I thought was relevant from 480 to 10, but that didn't seem to help even after a few hours of running.

This also makes me think that we should probably have a dummy call to get the pref (and hence migrate it) when we open up the preferences dialog. This way the user will get the right value even if auto compact hasn't happened yet.

+#define PREF_MAIL_PURGE_MIGRATED "mail.purge_pref_migrated"
...
+pref("mail.purge_threshold_migrated", false);

Note: these names are different.

Comment 18

[David :Bienvenu]

clicking on a newsgroup or local folder will kick in the autocompact (or clicking an imap folder while offline). We probably need some way to kick off autocompact for imap folder offline stores that doesn't involve being offline.

I'll fix the pref name and look at forcing a migration of the pref.

Comment 19

[David :Bienvenu]

Attachment: fix pref name

I fixed the pref name. I looked into making the options UI migrate the pref, but it's not easy since the global purge threshold isn't an attribute of nsIMsgFolder (and I don't want to make it one). Currently, only calling AutoCompact makes the pref get migrated. I tried making the options UI do its own migration but I couldn't get the code to work (I'm not even sure it was getting called). If I figure out a way to more easily get offline stores compacted for IMAP, that'll ameliorate the options UI issue since the pref will be much more likely to get migrated before the user brings up the options UI. I could hook it up to the auto sync manager IDLE notification code, since it's most likely people who have autosync turned out that will care about compaction of offline imap stores. I'd need to try to make it not get in the way of autosync, though given that it only does something at most once an hour, it wouldn't get in the way often.

Comment 20

[Mark Banner (:standard8)]

Comment on attachment 510402 [details] [diff] [review]
fix pref name

So code-wise I think this patch looks fine.

There's two things I'm concerned about:

- IMAP doesn't seem to auto compact unless we're in offline mode, so you'd have to be doing something in local folders for this to kick in. I'm guessing for many of our users they may not be doing that at the moment (unless send in background was working and that would trigger it), which makes turning on autocompact a bit useless. We can deal with this in a follow-up bug I think.

- The big modal dialog prompting you to compact.

I've got a couple of concerns with this dialog:

1) I'm assuming that if you run the IMAP delete model then auto-compact will purge your deleted emails? If so, I think the current dialog doesn't make that clear:

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#90

"Do you wish to compact all local and offline folders to save disk space?"

2) Should we be offering to turn off auto compact in the dialog? I can see a lot of users upgrading to 3.3 and hitting this dialog and getting annoyed by it (although they can at least choose to always do it without prompting).


Whilst I think these issues probably aren't major, I think I'd like clarkbw to chime in on them in case he wants follow-ups or something different.

In the meantime, I'd be happy for you to land the migration parts of this patch, with the actual auto-compact change waiting on Bryan's comments.

Comment 21

[David :Bienvenu]

(In reply to comment #20)

> - IMAP doesn't seem to auto compact unless we're in offline mode, so you'd have
> to be doing something in local folders for this to kick in. I'm guessing for
> many of our users they may not be doing that at the moment (unless send in
> background was working and that would trigger it), which makes turning on
> autocompact a bit useless. We can deal with this in a follow-up bug I think.

Yes, in an earlier comment I suggested that autosync might be a convenient time to do autocompaction for imap offline folders. But, this bug is a lot more important for pop3 users, because if they never compact, they could run into the 4GB limit, whereas imap has no effective limit for the size of offine stores.
> 
> 
> 1) I'm assuming that if you run the IMAP delete model then auto-compact will
> purge your deleted emails? 

No, it doesn't. For imap folders, it only compacts the offline store.

> 2) Should we be offering to turn off auto compact in the dialog? I can see a
> lot of users upgrading to 3.3 and hitting this dialog and getting annoyed by it
> (although they can at least choose to always do it without prompting).

That sounds reasonable - or give them a way to get from the dialog to the account settings so they can change the threshold, or turn off autocompact.

Comment 22

[David :Bienvenu]

(In reply to comment #20)

> 
> In the meantime, I'd be happy for you to land the migration parts of this
> patch, with the actual auto-compact change waiting on Bryan's comments.

Does that mean everything but the change to default to compacting automatically? I can't land the migration without the corresponding UI change, since the UI needs to use the migrated pref.

Comment 23

[David :Bienvenu]

Attachment: patch to turn on autocompact

Comment 24

[David :Bienvenu]

everything but autocompact by default checked in - http://hg.mozilla.org/comm-central/rev/bb7b2e55f9c2

Comment 25

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #20)
> I've got a couple of concerns with this dialog:
> 
> 1) I'm assuming that if you run the IMAP delete model then auto-compact will
> purge your deleted emails? If so, I think the current dialog doesn't make that
> clear:
> 
> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/messenger.properties#90
> 
> "Do you wish to compact all local and offline folders to save disk space?"

I'm immediately wondering if it's possible to avoid the purge of deleted mails or if we shouldn't default to this being on for IMAP delete model people.  It seems a bit risky for them and I'm under the assumption that most people don't use that model so changing the text to include it makes it awkward for most people.


> 2) Should we be offering to turn off auto compact in the dialog? I can see a
> lot of users upgrading to 3.3 and hitting this dialog and getting annoyed by it
> (although they can at least choose to always do it without prompting).

I like David's suggestion of offering a settings button that would take you to the compaction settings page.  It will reduce the amount of UI we need in the dialog to ( OK ) or ( Compaction Settings ) where people can turn it off or change the values.

Comment 26

[David :Bienvenu]

(In reply to comment #25)
> I'm immediately wondering if it's possible to avoid the purge of deleted mails
As I said earlier, I don't think autocompact purges deleted mails. It just compacts local stores.

Comment 27

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 514679 [details] [diff] [review]
patch to turn on autocompact

I think we can move forward with this.  I need to work on the first time compact dialog that is shown, but otherwise I think we're good.

Comment 28

[Mark Banner (:standard8)]

Ok, lets get this in for a3 and see what feedback we get.

Comment 29

[Mark Banner (:standard8)]

The patch for the preference change landed here:

http://hg.mozilla.org/comm-central/rev/3b4fbc1a34b8

Comment 30

[[e-mail]]

Comment on attachment 510402 [details] [diff] [review]
fix pref name

>+        thresholdMB = NS_MAX(1, *aThreshold/1024);
I didn't notice this because it was #ifndef MOZ_SUITE before bug 636370 landed but now that it's turned it on for my experimental external API build it is now failing to compile for me because nsAlgorithm.h isn't getting included.

Comment 33

[Eric Moore]

"But using MB in the UI seems more useful these days, agreed."

I updated to the 20110522000045 build of 3.3a4pre and noticed that the GUI claims the threshold is 0MB though mail.purge_threshhold seems to still have the correct value. I have it set to 100KB.  I've tried larger values such as 1MB and found it eventually caused corruption in a remote folder, because most of my messages are about 5KB.

We've been recommending setting the threshold around 100KB to 500KB for years in the MozillaZine forums and our KB articles because so many users have corrupted inbox's. You may disagree with that advice but please consider the effect displaying 100KB as 0MB will have on existing users. If they notice that many will not understand that the displayed value is wrong.

Comment 34

[David :Bienvenu]

message size is fairly irrelevant to the purge threshold in terms of corruption - lower thresholds just mean compaction happens more frequently.