Closed Bug 1686144 Opened 3 years ago Closed 3 years ago

Implement a local cache for the folder properties

Categories

(Thunderbird :: Folder and Message Lists, enhancement)

enhancement

Tracking

(thunderbird_esr78 unaffected, thunderbird85 unaffected)

RESOLVED FIXED
86 Branch
Tracking Status
thunderbird_esr78 --- unaffected
thunderbird85 --- unaffected

People

(Reporter: aleca, Assigned: aleca)

References

Details

Attachments

(1 file, 1 obsolete file)

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.

Attachment #9196694 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9196694 [details] [diff] [review]
1686144-folder-properties-cache.diff

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

Thx, r=mkmelin
Attachment #9196694 - Flags: review?(mkmelin+mozilla) → review+
Target Milestone: --- → 86 Branch

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?

Attachment #9196694 - Flags: feedback-

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.

(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?

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:

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?

Flags: needinfo?(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).

Flags: needinfo?(alta88)

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.

(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.

Attachment #9196694 - Flags: review+

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.

Attachment #9197843 - Flags: review?(mkmelin+mozilla)
Attachment #9197843 - Flags: feedback?(alta88)
Comment on attachment 9197843 [details] [diff] [review]
1686144-folder-properties-cache-object.diff

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

Seems reasonable
Attachment #9197843 - Flags: review?(mkmelin+mozilla) → review+
Attachment #9196694 - Attachment is obsolete: true

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/45dffbabfe2d
Implement local cache for the Folder properties. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Attachment #9197843 - Flags: feedback?(alta88) → feedback+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: