1686144 - Implement a local cache for the folder properties

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, enhancement)

Description

[Alessandro Castellani [:aleca]]

While working on the folderPane.js, I noticed that the getProperties() method of each folder gets called for every mouse interaction, even on a simple hover.
This seems to slightly affect the CPU, especially since the implementation of custom colors.

I'd like to propose the implementation of a very simple local cache Map() that will store the properties of each folder when first called.
We can then clear the specific entry in case the row is invalidated (which means something was changed by the user or the code).

We already use a similar approach here: https://searchfox.org/comm-central/rev/426e057d39873c932641e3c3e2da3ede8acb524d/mail/base/content/toolbarIconColor.js#59

This would allow us to store the full list of properties and prevent repeated calls to the same methods.

Comment 1

[Alessandro Castellani [:aleca]]

Attachment: 1686144-folder-properties-cache.diff

Comment 2

[Magnus Melin [:mkmelin]]

Comment on attachment 9196694 [details] [diff] [review]
1686144-folder-properties-cache.diff

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

Thx, r=mkmelin

Comment 3

[Alessandro Castellani [:aleca]]

Try run to see if this implementation broke something: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=9f97ffaea89b0c7a502f280da0d5704d32ab30ba

Comment 4

[alta88]

Comment on attachment 9196694 [details] [diff] [review]
1686144-folder-properties-cache.diff

Why does this duplicate an existing folder properties cache mechanism [g|s]etFolderCacheProperty() and _cache? Why is Map used instead of Object (_cache) - and has any investigation been done as to whether one is more performant than the other?

Comment 5

[Alessandro Castellani [:aleca]]

Mh, there's a bct4 failure on Linux: https://treeherder.mozilla.org/jobs?repo=try-comm-central&selectedTaskRun=It5hhokQRgGURhYSysEVdw.0

I'm not able to reproduce it locally, and this patch doesn't seem to touch anything related to that test.

Comment 6

[Alessandro Castellani [:aleca]]

(In reply to alta88 from comment #4)

Hey, thanks for checking the patch.

Why does this duplicate an existing folder properties cache mechanism [g|s]etFolderCacheProperty() and _cache?

Because the [g|s]etFolderCacheProperty() and _cache are used for single properties like "smartFoldername", "favicon", "folderIconColor", etc., and the string with all the properties is saved only if the folder is from an RSS server (which honestly I don't know why).

Why is Map used instead of Object (_cache) - and has any investigation been done as to whether one is more performant than the other?

I proposed to implement a Map because it comes with quick and easy methods to set, get, delete, and clear the whole array.
We're storing the full properties string and not separated single properties, so we can clear the full Map at once without worrying.
This data doesn't need to be parsed to/from a JSON, so we don't need an Object.

From what I learned and what I was thought, Map() is faster in handling larger amount of data, and even if there's a small cost during the initialization compared to the Object, the difference is in fractions of milliseconds.

Do you have any insight on the reasons behind the _cache Object being populated and used in that way?

Comment 7

[Alessandro Castellani [:aleca]]

So, let's step back a second and analyze this situation because it seems like we have multiple caching systems here and we should simplify things.

First of all, the XUL Tree requests the properties at every mouse interaction for each row, and I think we can't change that (right?).
In doing so, the FtvItem.getProperties() method is called a "million" times: https://searchfox.org/comm-central/rev/75aaa96983fe0c594aba966ba1b2f172c927d831/mail/base/content/folderPane.js#3090

In this method, we call the following methods:

• folderUtils.jsm:getFolderProperties()
• this._folder.getFlag()
• getSmartFolderName()
• getFolderCacheProperty() for the folder color
• FeedUtils.getFolderProperties() if is a Feed (RSS) folder

All of this, to return a "properties" string.

These are my doubts and questions:

• Why all those extra conditions aren't done directly in the folderUtils.jsm:getFolderProperties()?
• Why do we store the finalized "properties" string in the _cache object only if the folder is a Feed?
• Wouldn't a Map() be easier to manage with has() instead of handling a getter and setter: https://searchfox.org/comm-central/rev/75aaa96983fe0c594aba966ba1b2f172c927d831/mail/base/content/folderPane.js#1926-1958, and using an extra condition to check for the returned value? https://searchfox.org/comm-central/rev/75aaa96983fe0c594aba966ba1b2f172c927d831/mail/base/content/folderPane.js#1159-1162

jcranmer also suggested that the folder properties are supposed to go to the regular (mork-based) folder cached, which is supposed to be fast, managed in the nsMsgFolderCache{Element} code.
Any idea why we're not using that?

Comment 8

[alta88]

Yes, the objection is to the addition of yet another caching system, using a potentially sub performant method. If Map() is shown to be not subperformant, ie Object[folder.URI] is not faster than Map.get(folder.URI), then _cache should become a Map. Sure anything with management apis would be easier to manage, always is not free perf-wise, and is fine in a code path not called a "million" times. I suggest you get definitive advice from a javascript peer, given the context of the nsITree use case.

The issue at hand is that nsITree.getCellProperties() must return a space delimited string of properties, and this is what needs to be cached, as it certainly does not change over the course of a "million" accesses. The worst offender to perf here is folderUtils.getFolderProperties(), which creates an array, pushes properties into it, then returns a joined string - every single time. The _cache was implemented for feed folders because they have the highest feature/complexity (favicons and various runtime states) for use in nsITree.getImageSrc(), also called a "million" times along with getCellProperties(). Using getFolderCacheProperty() there and for folder colors is the correct way to go. That wasn't implemented for feeds or other folders in getCellProperties() -> FtvItem.getProperties().

Note that properties (to decorate the tree rows) are a combination of persisted properties (in mork, now in JSONFile with colors) and runtime properties like favicon and state. These need to be melded into the cached string and updated elsewhere as needed, using invalidate() to update the tree. This is what feeds does in setFolderPaneProperty(). But persisted properties and their store/cache aren't the issue in this bug.

(I don't know why anyone would encourage use of nsMsgFolderCache or other cross boundary methods, isn't the strategy to get out of mork/xpcom where non perf harmful).

Comment 9

[Alessandro Castellani [:aleca]]

Thank you so much for the explanation and overview.
I'll do some profiling in order to identify what is affecting perf the most, and compare the use of Map() against Object for _cache.

The end goal here, and I think you'd agree, is to always return the cached space delimited string of properties without generating a new one every time the nsITree.getCellProperties() is called.
And only generate a new string after a row has been invalidated, and of course store the new string in the _cache.

Comment 10

[alta88]

(In reply to Alessandro Castellani (:aleca) from comment #9)

The end goal here, and I think you'd agree, is to always return the cached space delimited string of properties without generating a new one every time the nsITree.getCellProperties() is called.

Correct.

And only generate a new string after a row has been invalidated, and of course store the new string in the _cache.

There are two strategies: update the cached string, then invalidate(), or delete the folder.URI entry and have getCellProperties() initiate the update. The first is better for post startup, but the second may be the only way at startup. The startup situation is critical; for favicon the very expensive network call is async, but something must be returned immediately on first paint. Same with folder name and other basic first paint properties.

Comment 11

[Alessandro Castellani [:aleca]]

Attachment: 1686144-folder-properties-cache-object.diff

I've been doing some profiling between using an Object and a Map and I wasn't able to discern any clear winner between the 2 as the performance was pretty on par.

I decided to stick the _cache Object as it's well integrated in side the Folder Pane and it already comes with everything we need.
The only thing I did was storing the full string of properties in the Object cache and returning that if present with a new folder.getProperties() it's called (which is at every mouse movement due to how the XUL Tree works).

These are the recorded profiles:
Profile 1: Current implementation: https://share.firefox.dev/2XOOgVw
Profile 2: Patch applied with properties string saved in _cache Object: https://share.firefox.dev/38VHunh
Comparison: https://share.firefox.dev/3quCJa4

It seems that profiling in Thunderbird doesn't support recording memory usage, too bad.
The most noticeable difference between the 2 profiles is that the current implementation caused many more Jank markers.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9197843 [details] [diff] [review]
1686144-folder-properties-cache-object.diff

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

Seems reasonable

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/45dffbabfe2d
Implement local cache for the Folder properties. r=mkmelin