Closed Bug 437657 Opened 12 years ago Closed 9 years ago

increase autocompact threshold "mail.purge_threshhold"

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.3a3

People

(Reporter: tuukka.tolvanen, Assigned: Bienvenu)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
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.) 
Keywords: helpwanted
OS: Linux → All
Hardware: PC → All
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.
xref bug 379103
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.
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.
"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.
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.
Duplicate of this bug: 613316
Duplicate of this bug: 379103
Assignee: nobody → bienvenu
Blocks: 341206
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.
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.
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.
Attachment #503351 - Flags: review?(bugzilla)
blocking-thunderbird5.0: --- → ?
Whiteboard: [has patch for review]
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.
(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 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.
Attachment #503351 - Flags: review?(bugzilla) → review-
Attached patch fix addressing comments (obsolete) — Splinter Review
Attachment #503351 - Attachment is obsolete: true
Attachment #509585 - Flags: review?(bugzilla)
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.
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.
Attached patch fix pref nameSplinter Review
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.
Attachment #509585 - Attachment is obsolete: true
Attachment #510402 - Flags: review?(bugzilla)
Attachment #509585 - Flags: review?(bugzilla)
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.
Attachment #510402 - Flags: ui-review?(clarkbw)
Attachment #510402 - Flags: review?(bugzilla)
Attachment #510402 - Flags: review+
(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.
(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.
Attachment #514679 - Flags: review?(bugzilla)
everything but autocompact by default checked in - http://hg.mozilla.org/comm-central/rev/bb7b2e55f9c2
No longer blocks: TB2SM
Attachment #510402 - Flags: ui-review?(clarkbw)
Attachment #514679 - Flags: ui-review?(clarkbw)
Attachment #514679 - Flags: review?(bugzilla)
Attachment #514679 - Flags: review+
(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.
(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 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.
Attachment #514679 - Flags: ui-review?(clarkbw) → ui-review+
Ok, lets get this in for a3 and see what feedback we get.
The patch for the preference change landed here:

http://hg.mozilla.org/comm-central/rev/3b4fbc1a34b8
Status: NEW → RESOLVED
blocking-thunderbird5.0: ? → ---
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch for review]
Target Milestone: --- → Thunderbird 3.3a3
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.
Duplicate of this bug: 341206
Duplicate of this bug: 205756
"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.
message size is fairly irrelevant to the purge threshold in terms of corruption - lower thresholds just mean compaction happens more frequently.
Blocks: 1462666
You need to log in before you can comment on or make changes to this bug.