1669872 - GloDa indexing messages smashes the address book

Status: RESOLVED FIXED

(MailNews Core :: Database, defect, P1)

Description

[Geoff Lankow (:darktrojan)]

For every message GloDa indexes, and every email address in that message, it asks the address book to find a contact. In the worst case if there is no such contact, every directory must look through its entire database to find nothing.

In this bug I'm going to cache the results of the lookup so that when the same address is looked up a second time the address book is not hit. Given that this cache will collect every address ever seen (whether or not it's in the address book, and whether or not the address will ever be seen again) I'll clear the cache when GloDa stops indexing.

Comment 1

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1669872 - Try to avoid using the address book database when indexing messages. r?mkmelin

Comment 2

[Wayne Mery (:wsmwk)]

I didn't do a benchmark of try build vs beta, but results seem positive - tested with my luwsm account, 5k message inbox, very responsive, message bodies display quickly, even though activity manager shows indexing is still in progress. (but potentially my results skewed by using phone hotspot - a temporary situation)

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/6171c903abd1
Try to avoid using the address book database when indexing messages. r=mkmelin

Comment 4

[Wayne Mery (:wsmwk)]

I just fired up my "production" beta profile with the try build. It is definitely better.

What other locations likely use AB lookup that may see significant performance impact? Filters? Calendar? Any addons?

Comment 5

[Geoff Lankow (:darktrojan)]

Oh yuck, I suspect this code is pretty hot too, and it does almost exactly what the GloDa code does. Maybe I should generalise what I've done in this bug and put it somewhere more useful.

Comment 6

[Geoff Lankow (:darktrojan)]

Attachment: Bug 1669872 - Create a cached function which calls cardForEmailAddress on all directories. r?mkmelin

This is more general than the previous version so that other code can call it too. I also realised with the 60s time out, waiting for Gloda to idle was unnecessary.

Comment 7

[alta88]

Big improvement to open it up to other callers but why not make this cache permanent? Maintenance on a card change is very simple; just add those listeners and replace/add/delete the cache entry. That is the most performant and best practices way to do it. Doubt memory is an issue (but it could be modeled).

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2efdb12bede3
Create a cached function which calls cardForEmailAddress on all directories. r=mkmelin

Comment 9

[Geoff Lankow (:darktrojan)]

Comment on attachment 9180659 [details]
Bug 1669872 - Create a cached function which calls cardForEmailAddress on all directories. r?mkmelin

[Approval Request Comment]
Regression caused by (bug #):
User impact if declined: Lots of slow things
Testing completed (on c-c, etc.): On c-c since last week
Risk to taking this patch (and alternatives if risky): Not much, this is just a cache, and it's only used in places where 100% accuracy isn't critical.

The other patch in this bug will need to land first, but it's almost entirely wiped out by this patch, so it'd probably be tidier if they were rolled together.

Comment 10

[Wayne Mery (:wsmwk)]

Comment on attachment 9180659 [details]
Bug 1669872 - Create a cached function which calls cardForEmailAddress on all directories. r?mkmelin

[Triage Comment]
Approved for beta

Comment 11

[Rob Lemley [:rjl]]

Attachment: 1669872-gloda_cache_functions_COMBINED.patch

As suggested, this is the combined version of the two patches that landed on comm-central, intended for uplift.

[Triage Comment]
Previous un-combined version approved by wsmwk.

Comment 12

[Rob Lemley [:rjl]]

Thunderbird 82.0b3:
https://hg.mozilla.org/releases/comm-beta/rev/a3e7a88b857f

Comment 13

[Wayne Mery (:wsmwk)]

Comment on attachment 9181038 [details] [diff] [review]
1669872-gloda_cache_functions_COMBINED.patch

[Triage Comment]
Approved for esr78 (I assume combined is also what is wanted for esr)

Comment 14

[Rob Lemley [:rjl]]

Thunderbird 78.4.0:
https://hg.mozilla.org/releases/comm-esr78/rev/24f92319c043

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9180659 [details]
Bug 1669872 - Create a cached function which calls cardForEmailAddress on all directories. r?mkmelin

[Triage Comment]
for 78 we used attachment 9181038 [details] [diff] [review] 1669872-gloda_cache_functions_COMBINED.patch