562965 - folderPane.js ftv_getRowProperties/ftv_getCellProperties disproportionately slow

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect)

Description

[Andrew Sutherland [:asuth] (he/him)]

Performance tooling reveals that we are spending ~1ms on each call to ftv_getRowProperties/ftv_getCellProperties.  In comparison, ftv_getCellText which dispatches and does some work is 0.08-0.11ms and ftv_getImageSrc which is a JS-implemented no-op takes 0.012-0.014ms.

ftvItem_getProperties which is the thing that gets dispatched to by ftv_getRowProperties/ftv_getCellProperties does a number of things that could easily be cached...

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v1 folder pane caching

With the patch, the cost of ftv_getRowProperties/ftv_getCellProperties drops from ~1ms to ~0.16-0.19ms (in the cached case).  It's that slow because the nsITreeView interface passes in an nsISupportsArray and the batch addition methods for nsISupportsArray are [notxpcom] so script can't use them.  Likewise, the clone method is useless since the array is being passed in.

Although this should do nice performance things for scrolling the folder pane, I'm honestly more interested in the likely performance benefits to our mozmill tests.

The major concern with the changes in the patch are cases where the current implementation is not properly invalidating rows but lucks out because the tree view feels the need to repaint almost everything almost all the time.  The one case I know of is marking a folder whose subfolders have unread messages appropriately.  I skipped addressing this immediately because the implementation of getParentIndex in the folder pane is rather naive to the tune of O(n) which could result in an accumulated cost of O(mn) without resolution.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 442769 [details] [diff] [review]
v1 folder pane caching

er, there's obviously at least one invalidation bug already present.  I'll fix that and address the parent index efficiency thing for the other case before requesting review.

Comment 3

[David :Bienvenu]

Thx, yeah, there's an existing bug about parent invalidation when a child gets unread messages. It would be great to fix that while you're mucking with the code.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Attachment: v2 folder pane caching

this corrects the invalidation problems and I think corrects the other mentioned bug.  to wit, when a new message was added to collapsed folder B, its parent folder A (which was visible) got bolded.

This is pretty pleasant to run with right now but I need to try out the mozmill tests and probably augment them to cover the new logic.

Comment 5

[David :Bienvenu]

asuth, any chance of picking this up? We might want it for 3.2...

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I gave up on this when it seemed probable that the current paint-happy implementation was likely hiding a multitude of sins and would likely lead to a number of regressions in core and in extensions unless a lot of effort was put into it.  (Also, writing mozmill tests to verify the correct behaviour is more involved than average...)

If someone wants to pick up the patch and run with it, they should feel free.

Comment 7

[Wayne Mery (:wsmwk)]

(In reply to comment #6)
> I gave up on this when it seemed probable that the current paint-happy
> implementation was likely hiding a multitude of sins and would likely lead to a
> number of regressions in core and in extensions unless a lot of effort was put
> into it. 

Andrew, thanks for the update. What do you mean by "core" - do you mean this will expose issues in Thunderbird cpp code?  And do any specific extensions spring to mind?  I ask because I'd think population of extensions that operate on folder pane is small. (but I suppose they might big extensions, like nostagly, mnheny, etc)

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

By core I just mean everything Thunderbird ships with.  I don't keep up with the set of extensions that augment the folder pane, so I couldn't speculate.  It's certainly a tractable problem and I suspect I may have dealt with the issues in core that I knew about; I don't mean to make it seem impossible.  It's just likely to take more time and have more fallout that needs to be handled than I can budget for the given payoff.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 443909 [details] [diff] [review]
v2 folder pane caching

fwiw, patch has obsoleted.

$ patch -p1 --dry-run < ~/Desktop/562965folder-pane-caching-v2.patch 
patching file mail/base/content/folderPane.js
Hunk #5 FAILED at 1482.
Hunk #6 FAILED at 1857.
Hunk #7 succeeded at 1947 (offset 11 lines).
Hunk #8 FAILED at 1980.
3 out of 8 hunks FAILED -- saving rejects to file mail/base/content/folderPane.js.rej
patching file mailnews/base/util/folderUtils.jsm

Comment 10

[alta88]

In Bug 1308727 and Bug 1312813 caching for folderpane was generically implemented, although currently is only used for feed specific |properties| and |favicon|. (The cache shouldn't be on ftvItem, as the initial favicon implementation discovered, as the rowMap is torn down when switching folder views, and even on updates in certain folder views). Atoms aren't used and are surely theoretically faster (and nsITreeView has long since removed atoms, Bug 407956) but unknown if user detectable faster.

Anyway, in doing all this, the true culprit for serious jank in folderpane was noticed to seemingly be getSmartFolderName().  This thing (Bug 553747) does a getStringProperty on every folder, meaning it has to go to panacea.dat then (iirc) to the folder msgDatabase. And it will never find the string "smartFolderName" as the implementation is *only* for extensions. 

It can be observed that Tb memory will more than double instantly after merely mousing over a fullscreen length folderpane (after is has settled for a few minutes after startup and gc on dbs has run), as the msgDatabase has to be reloaded for this (and only this -- all other properties are derived from the folder itself) access in folderpane. I used a screenful of feed folders, to test favicons etc. perf behavior.

It can also be observed that a simple getSmartFolderName = function() {return null;} in the console command line will eliminate the problem.

It would be interesting to see if there is even any extension which calls addSmartFolderType() because the penalty for everyone else is very high. If someone can disprove this theory, please do so. Btw, Bug 553747 was checked in 10 days before asuth filed this..

Comment 12

[alta88]

Attachment: smart.patch

Bug 795038 is correct, there can be no msgdb access from folderpane tree methods. Perhaps someone can scan amo, but to me it's of very little importance given how egregious this flaw is for everyone else.

Comment 13

[:aceman]

(In reply to alta88 from comment #10)
> It would be interesting to see if there is even any extension which calls
> addSmartFolderType() because the penalty for everyone else is very high. If
> someone can disprove this theory, please do so.

There are 2 addons on AMO using this call and one of our tests.

The addons do this:
1.-----------------------
try {
    /* this depends on a patch landing for folderPane.js,
       thunderbird bug 553747 */
    let folderTypes = {};
    for each(let [name, data] in Iterator(bulklists)) {
        folderTypes[data.type]=1;
    }
    for each(let [folderType,] in Iterator(folderTypes)) {
        gFolderTreeView.getFolderTreeMode("smart")
            .addSmartFolderType(folderType, false, false);
    }
} catch(e) {
    /* smartfolder addon patch not applied */
}

2.------------------------
/**
 * insert our folder type into the unified folders view, use the actual flag
 * value in case we change the name later.
 */
function addSmartFolderType(aFolderName) {
  gFolderTreeView.getFolderTreeMode("smart")
        .addSmartFolderType(aFolderName, false, false);
}

try {
  var gMailingListBundle = Components.classes["@mozilla.org/intl/stringbundle;1"]
          .getService(Components.interfaces.nsIStringBundleService)
          .createBundle("chrome://maillistfilter/locale/list.properties");
  var mlFolderName = gMailingListBundle.GetStringFromName("maillistfolder.label");
  var fFolderName = gMailingListBundle.GetStringFromName("filteredlistfolder.label");
  if (MLFUtils.prefs.getBoolPref("enable_smartfolders", false)) {
    addSmartFolderType(mlFolderName);
    addSmartFolderType(fFolderName);
  }
} catch(e) {/*ignore*/}

Comment 14

[:aceman]

Comment on attachment 8834521 [details] [diff] [review]
smart.patch

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

Thanks, the change seems simple enough. I do not see any addon calling getSmartFolderName().
But the two mentioned addons use folder.setStringProperty("smartFolderName", name). So I assume this will still pass after your patch but it will not have any effect as getSmartFolderName() will not pick up this set name.

Please add the author and commit message to the HG header in the patch.

::: mail/base/content/folderPane.js
@@ +2391,5 @@
>        properties += FeedUtils.getFolderProperties(this._folder, null);
>        gFolderTreeView.setFolderCacheProperty(this._folder, "properties", properties);
>      }
>  
> +    this["properties"] = properties;

Is this change related to the bug here?

@@ +2870,5 @@
> + *
> + * Example:
> + *   gFolderTreeView.setFolderCacheProperty(folder, // nsIMsgFolder
> + *                                          "smartFolderName",
> + *                                          "My Smart Folder");

Can you make the setSmartFolderName() function with this code and then update the test test-custom-smart-folder.js to use it? The test fails now with this patch.

Comment 15

[:aceman]

(In reply to alta88 from comment #10)
> In Bug 1308727 and Bug 1312813 caching for folderpane was generically
> implemented, although currently is only used for feed specific |properties|

Does the generic implementation cover all the features in asuth's patch here (like the invalidation stuff)? Or is that no longer needed?

Comment 16

[alta88]

Attachment: smart.patch

(In reply to :aceman from comment #14)
> Comment on attachment 8834521 [details] [diff] [review]
> smart.patch
> 
> Review of attachment 8834521 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, the change seems simple enough. I do not see any addon calling
> getSmartFolderName().
> But the two mentioned addons use folder.setStringProperty("smartFolderName",
> name). So I assume this will still pass after your patch but it will not
> have any effect as getSmartFolderName() will not pick up this set name.

the addons will need to be updated, could you please list them here.

> 
> Please add the author and commit message to the HG header in the patch.
> 

sure.

> ::: mail/base/content/folderPane.js
> @@ +2391,5 @@
> >        properties += FeedUtils.getFolderProperties(this._folder, null);
> >        gFolderTreeView.setFolderCacheProperty(this._folder, "properties", properties);
> >      }
> >  
> > +    this["properties"] = properties;
> 
> Is this change related to the bug here?
> 

removed; only way to do sane debug in treeview..

> @@ +2870,5 @@
> > + *
> > + * Example:
> > + *   gFolderTreeView.setFolderCacheProperty(folder, // nsIMsgFolder
> > + *                                          "smartFolderName",
> > + *                                          "My Smart Folder");
> 
> Can you make the setSmartFolderName() function with this code and then
> update the test test-custom-smart-folder.js to use it? The test fails now
> with this patch.

done.  the tests pass locally.

Comment 17

[alta88]

(In reply to :aceman from comment #15)
> (In reply to alta88 from comment #10)
> > In Bug 1308727 and Bug 1312813 caching for folderpane was generically
> > implemented, although currently is only used for feed specific |properties|
> 
> Does the generic implementation cover all the features in asuth's patch here
> (like the invalidation stuff)? Or is that no longer needed?

It's similar, but much simpler. It also uses the folder uri as key. Atoms are not used (I would really like to see some numbers on whether atoms are *measurably* and then *practically* faster). As for row invalidation, for feeds I implemented that two callers up from setFolderCacheProperty() as it really depended there on some specific logic.  I suppose it could be brought into setFolderCacheProperty as an extra arg, similar to here:
https://dxr.mozilla.org/comm-central/rev/d28d4429f6faa8bc5a1e48278e530efa05b1d086/mailnews/extensions/newsblog/content/FeedUtils.jsm

For folder change events, this might be useful if indeed the backend is over invalidating when only the single row is changing. It gets tricky if only a range is needed, but in that case it's simpler to just invalidateAll().

Comment 18

[alta88]

meh, dxr.  here:
https://dxr.mozilla.org/comm-central/source/mailnews/extensions/newsblog/content/FeedUtils.jsm#754

Comment 19

[alta88]

Attachment: smart.patch

fix hg headers.

Comment 20

[Andrew Sutherland [:asuth] (he/him)]

Working with JS strings is almost certainly going to be faster than atoms unless the underlying C++ code converts things to atoms, which it sounds like is not the case.  Skimming my patch, I think the goal was just to do expensive things once and cache them rather than every time; atoms only entered into it because atoms were what the existing code used.

Comment 21

[alta88]

(In reply to Andrew Sutherland [:asuth] from comment #20)
> Working with JS strings is almost certainly going to be faster than atoms
> unless the underlying C++ code converts things to atoms, which it sounds
> like is not the case.  Skimming my patch, I think the goal was just to do
> expensive things once and cache them rather than every time; atoms only
> entered into it because atoms were what the existing code used.

That's good to know; the reference is to an atoms debate in another bug..

Comment 22

[:aceman]

In case of doubt, I'd avoid atoms. Some interfaces in m-c already removed them, now they are giving us problems in the other bug. They look like on the way to depreciation.

Comment 23

[:aceman]

Comment on attachment 8834674 [details] [diff] [review]
smart.patch

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

Thanks, the test passes now.

::: mail/test/mozmill/folder-tree-modes/test-custom-smart-folder.js
@@ +72,5 @@
> +function test_cache_property() {
> +  if (mc.window.getSmartFolderName(subfolderA) != smartParentNameA)
> +    throw new Error("smartFolderName A cache property not set");
> +  if (mc.window.getSmartFolderName(subfolderB) != smartParentNameB)
> +    throw new Error("smartFolderName B cache property not set");

I'm not sure about the addition of the A and B. the property name is still "smartFolderName". Just that it is a cache property now, not string property. The error message meant the property name, not the expected value. But you could reword, modify it to include it.

Comment 24

[alta88]

(In reply to :aceman from comment #23)
> Comment on attachment 8834674 [details] [diff] [review]
> smart.patch
> 
> Review of attachment 8834674 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks, the test passes now.
> 
> ::: mail/test/mozmill/folder-tree-modes/test-custom-smart-folder.js
> @@ +72,5 @@
> > +function test_cache_property() {
> > +  if (mc.window.getSmartFolderName(subfolderA) != smartParentNameA)
> > +    throw new Error("smartFolderName A cache property not set");
> > +  if (mc.window.getSmartFolderName(subfolderB) != smartParentNameB)
> > +    throw new Error("smartFolderName B cache property not set");
> 
> I'm not sure about the addition of the A and B. the property name is still
> "smartFolderName". Just that it is a cache property now, not string
> property. The error message meant the property name, not the expected value.
> But you could reword, modify it to include it.

It doesn't make sense to have an identical error msg for both when 2 different names are used, that's why..  For error log readability.

Comment 25

[alta88]

Comment on attachment 8834674 [details] [diff] [review]
smart.patch

[Approval Request Comment]
Regression caused by (bug #):  Bug 562965 
User impact if declined: serious ui freeze, folderpane jank
Testing completed (on c-c, etc.): local tests.
Risk to taking this patch (and alternatives if risky): none, not even for the unused unupdated extensions for which this was erroneously allowed into the codebase.

Comment 26

[alta88]

@wsmwk - i believe all bugs after 562965, complaining of ui freeze, freeze on tab change into 3pane tab, simple focus on the Tb window, and even the unresponsive script dialog at startup, can be reviewed and likely summarily closed. the problem is more noticeable with a greater number of large folders.

Comment 27

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/450c53f428f5a590658ab880b8cf28d590c59ae9

Comment 28

[Jorg K (CEST = GMT+2)]

Sorry, I backed this out for now while investigating additional Mozmill failures which may be unrelated:
https://hg.mozilla.org/comm-central/rev/9a82837cc5fb575f5559cb31c3573dbe26a16ff9

Comment 29

[Jorg K (CEST = GMT+2)]

Sorry for backing this out. I recorded the new test failure not caused by this in bug 1338138.

However, with this patch in place I saw more failures which have gone away after backing it out:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=450c53f428f5a590658ab880b8cf28d590c59ae9&selectedJob=75850136

TEST-UNEXPECTED-FAIL | C:\slave\test\build\tests\mozmill\folder-display\test-deletion-from-virtual-folders.js | test-deletion-from-virtual-folders.js::test_delete_last_message_from_virtual_folder_closes_message_displays

Also seen here:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=873c200a502ed469d4262a4e97f679135e3dfc56&selectedJob=75860038

You might want to investigate this a bit before relanding it. Sorry, I'm playing sheriff here, I haven't looked whether the test failure could be related to the patch here.

Comment 30

[alta88]

This patch is not the cause of the error in comment 29. You will notice that failure in this try run (before this patch) on win7 opt:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=679a8fcd3a3ce8c23c36c90fde3a98b66493d63e

And you don't see it in this try run, which includes this patch:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dad7a38398492dcc537117d3f2e38c1d26fd2c7b

Comment 31

[Jorg K (CEST = GMT+2)]

OK, sorry.

Comment 32

[Wayne Mery (:wsmwk)]

(In reply to alta88 from comment #26)
> @wsmwk - i believe all bugs after 562965, complaining of ui freeze, freeze
> on tab change into 3pane tab, simple focus on the Tb window, and even the
> unresponsive script dialog at startup, can be reviewed and likely summarily
> closed. the problem is more noticeable with a greater number of large
> folders.

Thanks for the heads up.  Look forward to taking this in b3, assuming all the kinks get worked out

Comment 33

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/8290de02b1381a49d669121fe1bcf4e362521e4e

Comment 34

[Jorg K (CEST = GMT+2)]

Aurora (TB 53):
https://hg.mozilla.org/releases/comm-aurora/rev/93a9ae7c47b862ad48f9e951555f72f4cf1da9b0

Comment 35

[Jorg K (CEST = GMT+2)]

Beta (TB 52):
https://hg.mozilla.org/releases/comm-beta/rev/fe3301a68e98c555ea25c304c20cd156e48e6b5f