Closed Bug 1597163 Opened 3 months ago Closed 3 months ago

Cache address book lists and cards

Categories

(MailNews Core :: Address Book, task)

task
Not set

Tracking

(thunderbird71 fixed, thunderbird72 fixed)

RESOLVED FIXED
Thunderbird 72.0
Tracking Status
thunderbird71 --- fixed
thunderbird72 --- fixed

People

(Reporter: darktrojan, Assigned: darktrojan)

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

A lot of repeated main thread I/O can be prevented if we remember the cards in a directory and their properties. It also saves scanning the entire properties table for each card. The downside is using a bit more memory but I think it is worth it.

Attached patch 1597163-addrbook-cache-1.diff (obsolete) — Splinter Review
Attachment #9109341 - Flags: review?(mkmelin+mozilla)
Attached patch 1597163-addrbook-cache-1.diff (obsolete) — Splinter Review

Now without the code I used to check this was actually doing something worthwhile.

Attachment #9109341 - Attachment is obsolete: true
Attachment #9109341 - Flags: review?(mkmelin+mozilla)
Attachment #9109342 - Flags: review?(mkmelin+mozilla)
Attached patch 1597163-addrbook-cache-2.diff (obsolete) — Splinter Review

Now with the right file. This is going well. :-/

Attachment #9109342 - Attachment is obsolete: true
Attachment #9109342 - Flags: review?(mkmelin+mozilla)
Attachment #9109344 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9109344 [details] [diff] [review]
1597163-addrbook-cache-2.diff

Review of attachment 9109344 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/addrbook/jsaddrbook/AddrBookDirectory.jsm
@@ +322,5 @@
>      return card.QueryInterface(Ci.nsIAbCard);
>    },
>    _loadCardProperties(uid) {
> +    if (this._inner.hasOwnProperty("_cards")) {
> +      let cachedCard = this._cards.get(uid);

should this (preferably) be this._inner._cards? same below
Attachment #9109344 - Flags: review?(mkmelin+mozilla) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/d5118af449e7
Cache address book lists and cards; r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 72.0

Assuming there are no regressions…

Attachment #9109891 - Flags: review+
Attachment #9109891 - Flags: approval-comm-beta?
Comment on attachment 9109891 [details] [diff] [review]
1597163-addrbook-cache-3.diff

That's the beta patch?
Attachment #9109891 - Flags: approval-comm-beta? → approval-comm-beta+

That's the patch I landed with the review comments addressed. I assume it works on beta.

Attachment #9109344 - Attachment is obsolete: true
Keywords: perf
You need to log in before you can comment on or make changes to this bug.