138208 - IMAP: we calculate custom headers from filters every time we download headers in every folder, even if you have no custom headers

Status: NEW

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

Every time you download headers in an imap folder, we iterate over all your
filters to see if any of them have custom headers defined. If you have no custom
headers, we don't ever set the m_arbitraryHeaders string to a non-empty string,
so we iterate over all the filters again. If you have  a lot of filters, this
could add up - also, we should probably only be doing this for the inbox since
it's the only folder we run filters against.

Comment 1

[Navin Gupta]

hey, feel free to file custom headers bug on me. 

Comment 2

[Wayne Mery (:wsmwk)]

(In reply to comment #0)
> Every time you download headers in an imap folder, we iterate over all your
> filters to see if any of them have custom headers defined. If you have no custom
> headers, we don't ever set the m_arbitraryHeaders string to a non-empty string,
> so we iterate over all the filters again. If you have  a lot of filters, this
> could add up - also, we should probably only be doing this for the inbox since
> it's the only folder we run filters against.

David, do you know if this is still true?  And in terms of visible performance impact, what might be considered "a lot of filters"?

Comment 3

[David :Bienvenu]

I doubt there's a visible performance impact.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #3)
> I doubt there's a visible performance impact.

It would be interesting to test this. Some users have dozens, even hundreds of filters (which I could remember what's the max I've encountered in user stories).

And, would this not impact users who set the "not a performance feature" hidden preferences mail.check_all_imap_folders_for_new and mail.server.default.check_all_folders_for_new ?

Comment 5

[Wayne Mery (:wsmwk)]

I have one account with 130 filters. I've encountered a few users who have many more - don't remember who.

Danny, Mark, what's the largest number of filters you have on an account?

Comment 6

[Dan Pernokis]

Sorry to disappoint on this, Wayne, but I don't have a lot of filters, nor do we do much IMAP -- all but one are POP3.

The single IMAP account is a contingency and rarely receives mail. In any case, it is polled once every half hour and has just one simple filter to TAG ALL with a unique colour tag and star.

The largest account has 38 filters on file, 26 of which are actively used and search/filter for very specific items. This account is polled every five minutes and receives maybe one or two dozen emails per day.

The second-largest is LOCAL FOLDERS which has 23 on file and 9 actively used, but of course it applies to all polls.

FYI, I have 18 accounts in total (+ Local Folders) which are pollable, of which 14 are part of GET-MAIL. (The other 4 are polled manually on demand. One is actually just a template so is never polled.) A couple of important accounts are polled every ten minutes, the rest anywhere from half-hour to over an hour.

We don't usually get a lot of email traffic, so the filters are not exercised to the point where there are significant delays. (I see much more delay -- average 30-40 seconds or more sometimes, per account, on manual polls or manual GET-MAILs -- caused by timing at our ISP (3 accounts) or our web host's mail servers (most others). The single IMAP account is G-Mail, and it rarely ever has delays in polling. In all these polling delays, it makes no noticeable difference whether there are messages or not -- a complete manual GET-MAIL cycle (14 accounts) can finish in a few seconds at certain times, and take the 30-40 seconds or more in heavy traffic. Any delay by filtering is masked by the jitter in polling.

[ I know you are well aware of problems with pop-up windows when certain collisions occur (Bug 707933, for example). Perhaps the multiple/redundant filter iterations cause or contribute to this issue... ]

Comment 7

[u328884]

Sorry, Wayne, I don't use IMAP. Also, I probably have fewer than a half-dozen filters.

Comment 8

[Dan Pernokis]

Wayne: Perhaps speak to STEVE in Bug #707933 -- I think he has a sizable bunch of accounts, and I think they're IMAPs.

In his Bug 707933 Comment #46, he happens to mention "I can click twice on the GET MAIL button and get 17 modal windows to dismiss." In my 14 accounts on GET-MAIL (ALL), I usually get 2 or 3 popups, sometimes more -- so if he gets 17, he probably has dozens or more accounts being polled. (He spoke hypothetically in his next comment about "...if I tell it to check 100 email accounts...", and he had said specifically in Bug 707933 Comment 30 "...if the system is processing 27 mail accounts, like me...")

Comment 9

[:aceman]

This still exists, we treat empty string in m_arbitraryHeaders to mean 'uninitialized' and run the determination of headers again. We reset the string to empty on each change of filters (save of filter file) to trigger update of the list.

The determination of the headers is at
https://searchfox.org/comm-central/source/mailnews/base/search/src/nsMsgFilterList.cpp#1106 .
It is a small C++ loop so it shouldn't be too slow. But a profile would help.

There are a few ways to fix this:

1. use a special value in the string to mean it is initialized, but empty.
2. just call ComputeArbitraryHeaders() from LoadTextFilters() and SaveTextFilters() and not from GetArbitraryHeaders().

Solution 2 is marginally simpler than 1.

The fix also wouldn't be to complicated so we could just do it without meditating if it will be a visible perf gain or not.

The current state is a slight optimization in that it only runs ComputeArbitraryHeaders() if the filters are to be applied on IMAP, so non-IMAP users never call it. This would be regressed by solution 2 as it would be called unconditionally at startup or filters change (but that's not a frequent scenario).

Comment 10

[Magnus Melin [:mkmelin]]

I think option #2 sounds slightly better.

Comment 11

[:aceman]

I tried that but it is not fully correct. Our tests (e.g. imap/test/unit/test_filterCustomHeaders.js) just create a filter list and add a filter to it. There is no load and save of the filter file. That looks like a valid scenario addons could do too, so I need to rethink where to put the update of the custom header list.

Comment 12

[:aceman]

That would mean that even in the original case if there already would be custom headers in the variable, adding new filters by script that would need more custom headers (without saving the filter file), the change wouldn't be picked up as ComputeArbitraryHeaders() would never be called again.

Comment 13

[Wayne Mery (:wsmwk)]

(In reply to David :Bienvenu from comment #3)

I doubt there's a visible performance impact.

=> S4