Run filters periodically
Categories
(MailNews Core :: Filters, enhancement)
Tracking
(Not tracked)
People
(Reporter: rkent, Assigned: aceman)
References
(Blocks 2 open bugs)
Details
Attachments
(5 files, 10 obsolete files)
Reporter | ||
Comment 1•12 years ago
|
||
Reporter | ||
Comment 2•12 years ago
|
||
Comment 4•12 years ago
|
||
Comment 6•12 years ago
|
||
Comment 7•12 years ago
|
||
Reporter | ||
Comment 8•12 years ago
|
||
Reporter | ||
Comment 10•12 years ago
|
||
Comment 11•12 years ago
|
||
Reporter | ||
Comment 12•12 years ago
|
||
Reporter | ||
Comment 13•12 years ago
|
||
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Comment 14•12 years ago
|
||
Comment 15•12 years ago
|
||
Reporter | ||
Comment 16•12 years ago
|
||
Comment 17•12 years ago
|
||
Reporter | ||
Comment 18•12 years ago
|
||
Reporter | ||
Comment 19•12 years ago
|
||
Comment 20•12 years ago
|
||
Comment 21•12 years ago
|
||
Updated•11 years ago
|
Reporter | ||
Comment 22•11 years ago
|
||
Reporter | ||
Comment 23•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Comment 24•11 years ago
|
||
Reporter | ||
Comment 25•11 years ago
|
||
Reporter | ||
Comment 26•11 years ago
|
||
Reporter | ||
Comment 27•11 years ago
|
||
Reporter | ||
Updated•11 years ago
|
Assignee | ||
Comment 28•11 years ago
|
||
Comment 29•11 years ago
|
||
Comment 30•11 years ago
|
||
Comment 31•11 years ago
|
||
Comment 32•11 years ago
|
||
Comment 33•11 years ago
|
||
Comment 34•11 years ago
|
||
Comment 35•11 years ago
|
||
Comment 36•11 years ago
|
||
Reporter | ||
Comment 37•11 years ago
|
||
Comment 38•11 years ago
|
||
Updated•10 years ago
|
Assignee | ||
Comment 39•6 years ago
|
||
Assignee | ||
Comment 40•6 years ago
|
||
Assignee | ||
Comment 42•6 years ago
|
||
Assignee | ||
Comment 43•6 years ago
|
||
Guys, can we think about this again? It may not be perfect and does not cover all use cases (like Blake describes), but it should cover some users, e.g. running periodically on Inbox and potentially some other chosen folders is doable with this (which should be the main use of this). Each account has its own set of periodic filters. Sent folder will be excluded by default.
Will there be any harm in landing this feature?
I'm willing to revive the patch, unbitrot it, address review comments and polish it some more.
Comment 45•6 years ago
|
||
I'm also in favor of this incremental improvement.
Comment 46•6 years ago
|
||
See comments 31-34.
Assignee | ||
Comment 47•6 years ago
|
||
Assignee | ||
Comment 48•6 years ago
|
||
This is a refreshed patch (almost all hunks didn't apply). Addressed the review comments from Magnus and Ian. Updated to current syntax and the new .jsm file even passes eslint.
I fixed the logic problem with running all filters, now it runs only enabled filters of the Periodic type. Another problem was that nsIMsgIncomingServer.setIntValue (getIntValue) only handle int32 values, thus storing Date.now() overflowed the value, causing the nextFilterTime always be in the past, thus filters were run at each check (1-minute timer), instead of the 10 minutes set in mail.server.default.periodicFilterRateSeconds . So the '/ 1000' in the new patch is to store seconds instead of milliseconds, which do fit into int32. It will overflow in year 2038. If that is a problem we can make it / 10000, thus counting in 10-second granularity which would still be enough for this use case (we do not want the filters run too often and with seconds precision). We could even store minutes (/ 60000) because the check still happens in one minute intervals so setting e.g. 90 seconds will not be honored as precisely. We could then switch the pref to also contain minutes.
Another change I made is I stripped the folder flag to set which folders the periodic filters apply to, as this is subject to further discussion and possibly different implementation. I hard-coded that the periodic filters only run on Inbox and it is clearly displayed to the user. In this way it does not yet cover all user use-cases, but one clearly communicated one, where the user can choose if he wants this or not without surprises.
I suggest we could land this confined to Inbox to also see the perf impact for users actually enabling the periodic filters.
I can look at per-filter selector of folders to apply to in a followup bug.
Comments welcome :)
Comment 49•6 years ago
|
||
Assignee | ||
Comment 50•6 years ago
|
||
Yes, that hunk applies onto bug 697522, but you can also ignore it (not apply that part), it is not important for seeing the periodic filters feature.
Comment 51•6 years ago
|
||
Assignee | ||
Comment 52•6 years ago
|
||
Better? :)
I also reworked the sanitization of the run period so that user can't set a period below 60 seconds (via prefs for now) and bring TB to its knees.
Comment 53•6 years ago
|
||
Assignee | ||
Comment 54•6 years ago
|
||
OK, fixed the interval display.
Please still note comment 48 as it needs deciding.
Assignee | ||
Comment 55•6 years ago
|
||
This patch contains strings, can we get it finished in time for TB68?
Comment 56•6 years ago
|
||
Assignee | ||
Comment 57•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #56)
Comment on attachment 9061183 [details] [diff] [review]
why is this (all) inside the PopupNOoifications lazy getter? Are we sure the
notification it's going to listen to didn't already happen?
May that be a false impression from the patch? This is called from onLoadMessenger() which does not seem related to PopupNotifications. See https://searchfox.org/comm-central/source/mail/base/content/msgMail3PaneWindow.js#524 .
::: mail/locales/en-US/chrome/messenger/FilterEditor.dtd
+<!ENTITY contextPeriodic.label "Periodically (on Inbox)">
we can't have it for other folders?
You would need to read the other 55 comments ;)
In short: yes we want this for other folders too, but nobody could agree on a way how to choose those other folders. To just run them on all folders by default wasn't deemed safe as there may be different filters needed for e.g. Inbox vs. Sent. Kent's version had a method to choose the folders (but globally for all filters) via "folder properties" (right click a folder), which is a bit obscure and maybe does not scale too (does not automatically apply to subfolders when they are created).
So that's where the patch stalled and of course Kent can't play with it any longer.
So my proposal to solve this dead-lock is to only run the filters on Inbox as first stage.
Then in a followup bug we can flesh out a method and UI about how the user can specify the folders for each filter.
I still need you to decide comment 48 (the year 2038 problem).
@@ +95,5 @@
newFilterIndex++;
}
}
log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
if (newFilterIndex > 0)
what's this check for?
A micro-optimization to not even call MailServices.filters.applyFiltersToFolders if we have no filters to run. Of course MailServices.filters.applyFiltersToFolders can bail out if there are no filters, so I can drop this redundant check if you want.
Comment 58•6 years ago
|
||
I think the label should't still say anything about inbox...
For comment 48, let's just store minutes which will anyway be the usable unit there.
For the micro-optimization, yeah maybe not do that - we don't elsewhere in the similar situation either.
Updated•6 years ago
|
Comment 59•6 years ago
|
||
So my proposal to solve this dead-lock is to only run the filters on Inbox as first stage.
I think that's reasonable.
It's on Inbox where we need it, because new mail arrives there by definition.
Mail arrives in other folders only
a) with server-side filters, but then we combine server-side filtering with TB client-side filtering? That's really an edge case.
b) after manually moving it, after which filters should probably not apply.
So, while you can certainly construct cases where one might want it on other folders, they are really edge cases.
And, like aceman said, it makes the UI really complicated.
And I think this is a fair first step.
Please just remove the brackets in the label: "Periodically on Inbox"
Comment 60•6 years ago
|
||
(Or remove " on Inbox" entirely, because I think it's implied - normal filters also apply on Inbox, actually.)
Assignee | ||
Comment 61•6 years ago
|
||
(In reply to Ben Bucksch (:BenB) from comment #59)
b) after manually moving it, after which filters should probably not apply.
That's questionable. We have manually run filters so user may want to cleanup his folders once in a while. Running them automatically would be a help for him. Also I have heard cases where user would move a message into a folder (e.g. Inbox) and then expect the message is filtered (even to another folder).
(In reply to Ben Bucksch (:BenB) from comment #60)
(Or remove " on Inbox" entirely, because I think it's implied - normal filters also apply on Inbox, actually.)
Some apply on Sent when you check the right box.
Comment 62•6 years ago
|
||
Some apply on Sent when you check the right box.
Right, but if you don't, they apply only on Inbox. So, "Inbox only" is actually implied for most existing filters, without explicitly saying so in the UI.
I'm just saying that it's nothing new or outstanding.
Assignee | ||
Comment 63•6 years ago
|
||
Refreshed the patch and addressed the comments.
It seems now after the filter editor lost the <grid> elements, the second column of items does not align as good vertically to the items in the first column.
Comment 64•6 years ago
|
||
Updated•6 years ago
|
Assignee | ||
Comment 65•6 years ago
|
||
(In reply to Magnus Melin [:mkmelin] from comment #64)
+periodLength=Interval: #1 minute;Interval: #1 minutes
I think instead of having this label separately it should just be put into
the Periodically main label, as[ x ] Periodically, every 10 minutes
Note the checkboxes are still in a tabular grid (when though without a real <grid>) thus this would shift the second column, with the "before/after classification" menulist, which would then be more separated from the "getting new mail" label with which it kinda composes a sentence.
We could remove the grid layout, it does not seem very needed here.
Comment 66•6 years ago
|
||
Comment 67•6 years ago
|
||
This patch simplifys the header part. The two columns aren't needed (also the grid was never needed here).
Comment 68•6 years ago
|
||
Aceman's patch updated to apply on top of my patch. No string changes.
Updated•6 years ago
|
Assignee | ||
Comment 69•6 years ago
|
||
Thanks for the update. The layout works nicely.
Comment 70•6 years ago
|
||
Assignee | ||
Comment 71•6 years ago
|
||
Nice, thanks.
Assignee | ||
Comment 72•6 years ago
|
||
Fixed commit message.
Comment 73•6 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0c6818af6df7
Remove the unneeded second vbox in the first groupbox of the filter editor. r=mkmelin
https://hg.mozilla.org/comm-central/rev/686c2fa1e7cb
Add possibility to run filters periodically (most of code by rkent, tweaked by aceman). r=mkmelin,ui-r=Paenglab DONTBUILD
Comment 74•6 years ago
|
||
That somehow missed TB 24, see comment #23 ;-)
Comment 75•6 years ago
|
||
aceman - thanks for implementing this - I will add a UI to my Add-on quickFilters, if that helps! :)
Assignee | ||
Comment 76•6 years ago
|
||
I have tested the performance of this on a 4GB folder with 1 million of messages. I tried a filter that had a rule that matched various numbers of messages (tried 800000 and also 50000) with having a single action of "Mark as unread" (while all messages were read before).
The filter has to check every message if it matches at every run and then applies the action to all of the matching ones.
The new filter logging has visualized that the filter engine was applying the action to all those 50000 messages at each run (whether the action had any real effect or not). The log grew to hundreds of MBs :)
The results are that the time it takes to process a folder (run a filter on all the messages in it) is strictly dependent on the number of messages that are changed by the actions, NOT by the number of messages neding to be checked.
So if those messages were Read and it really had to change the flag to Unread and store that to the mbox file and the msf database, that took like 20 minutes (which is even longer than the 10 minute interval between the periodic filters, but TB was stuck unresponsive in that time and it does not seem that a new filter run was started while the first one was running). But TB did NOT take any noticeable amounts of additional RAM while executing the filters.
If those matched messages were already Unread so that the filter action didn't really do any change to them, the execution was very fast, within seconds and mostly unnoticeable in process monitor. A user actively using TB in that time could maybe spot the hang for a moment, but such a big folder causes small hangs all over the TB UI so it is not making things worse.
So it is great the c++ filter engine is so efficient and the actions are actually checked if they result in any change to msg headers (at least the Read/Unread transition, and we could optimize others if it is found some action does not do this) and the case of not hitting the database is very quick. As that is the expected common case for these periodic filters. They will run on a folder often, but rarely will they change any message, even if they match more of them. They are to catch some one odd message that was added to the folder between runs (maybe manually) and hadn't had the actions applied to it yet. Or to move messages older than some time away from the folder. And similar use cases.
Only problem was, the test was done on a machine with heaps of RAM and Ryzen CPU so the whole 4GB folder and it's msf (300MB) and the filter log file (300MB) could be kept in memory and only written to disk later by the OS when TB was already back to responding to user. On the other hand, it was a debug build, which is much slower than a release one.
Results on a slower machine may vary. Hopefully, users with ~4GB of RAM will have smaller folders and less messages in them as they will already face problems elsewhere.
Assignee | ||
Comment 77•6 years ago
|
||
(In reply to :aceman from comment #57)
log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
if (newFilterIndex > 0)
what's this check for?
A micro-optimization to not even call MailServices.filters.applyFiltersToFolders if we have no filters to run. Of course >MailServices.filters.applyFiltersToFolders can bail out if there are no filters, so I can drop this redundant check if you want.
Sadly, this optimization wasn't aggressive enough.
Before we enumerate the periodic filters to run (if any) we first lookup the Inbox folder of each server with the line:
let foldersToFilter = server.rootFolder.getFoldersWithFlags(Ci.nsMsgFolderFlags.Inbox);
This adds those Inbox folders to the folder cache, which is quite small (30 folders by default). If you have many accounts (like I have 20 in test profile), even some "dead" ones, for which no mail is fetched or no filters defined, this may push out useful folders out of the cache.
I have filed bug 1554424 for this.
Comment 78•6 years ago
|
||
(In reply to :aceman from comment #77)
(In reply to :aceman from comment #57)
log.debug("PeriodicFilterManager apply periodic filters to server " + server.prettyName);
if (newFilterIndex > 0)
what's this check for?
A micro-optimization to not even call MailServices.filters.applyFiltersToFolders if we have no filters to run. Of course >MailServices.filters.applyFiltersToFolders can bail out if there are no filters, so I can drop this redundant check if you want.Sadly, this optimization wasn't aggressive enough.
Before we enumerate the periodic filters to run (if any) we first lookup the Inbox folder of each server with the line:
let foldersToFilter = server.rootFolder.getFoldersWithFlags(Ci.nsMsgFolderFlags.Inbox);
This adds those Inbox folders to the folder cache, which is quite small (30 folders by default). If you have many accounts (like I have 20 in test profile), even some "dead" ones, for which no mail is fetched or no filters defined, this may push out useful folders out of the cache.
I have filed bug 1554424 for this.
We have to be careful if we are planning to exclude inactive accounts. It is possible to use inactive accounts (as I do) as "Local Folders" - meaning I have a number of old accounts with "dead" email addresses that are configured to not get mail from the servers - but we can set up filters to redirect mail from "active" Inboxes to these inactive ones, and then have cascading filters within the "dead" account.
So my question would be - how does the algorithm distinguish between accounts to determine they are "dead"? On the other hand, I don't know whether periodic running is necessary on a dead account when we use "cascaded" filters that react to (potentially read) emails that were moved over from "live" folders?
Assignee | ||
Comment 79•6 years ago
|
||
(In reply to Axel Grude [:realRaven] from comment #78)
We have to be careful if we are planning to exclude inactive accounts. It is possible to use inactive accounts (as I do) as "Local Folders" - meaning I have a number of old accounts with "dead" email addresses that are configured to not get mail from the servers - but we can set up filters to redirect mail from "active" Inboxes to these inactive ones, and then have cascading filters within the "dead" account.
No worries, we have no concept of 'inactive'/'dead' accounts. I only meant practically dead (which do not download from server and user only occasionally visits them), but we do not detect those in any way. Loading the Inbox of those every few minutes IF there are no periodic filters is useless and may affect other useful folders. Further comments in bug 1554424 please :)
Comment 80•5 years ago
|
||
NI: I have a filter that I use on certain local folders but not on others. It deletes items that are more than 2 days old. If I say that that filter is periodic every 10 minutes, how do I limit it to a folder? What would be good is that the folder name be made available to the filter definition then I can create folder-specific periodic filters.
Assignee | ||
Comment 81•5 years ago
|
||
Currently the periodic filters only run on Inbox folder so it is not possible to run them on other folder.
Exactly because there is no way to define the folders to run on.
What you propose is one method and there are also others so one has to be chosen and actually implemented in a new bug.
Description
•