1647251 - Starting TB 78 on a profile with many folders gives unresponsive script error

Status: RESOLVED FIXED

(Thunderbird :: Folder and Message Lists, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

A script on this page may be busy, or it may have stopped responding. You can stop the script now, or you can continue to see if the script will complete.
Script: chrome://messenger/content/folderPane.js:1730

This could affect may users. Related to bug 1637668?

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 2020-06-22 12_33_05-Warning_ Unresponsive script.png

Comment 2

[Alessandro Castellani [:aleca]]

Attachment: 1647251-folder-props.diff

I suspect it might be the very slow insertRule() API.
Jorg, are you able to load and try this patch?

Comment 3

[Alessandro Castellani [:aleca]]

Attachment: 1647251-folder-props.diff

All right, I did some profiling and found the issue.

Indeed, the sheet.inertRule() API is super slow.
I did a test in loading only 30 folders, which 10 of them have custom colors, and the difference was already glaring:

• sheet.inertRule(): 212ms to populate the stylesheet, FPS drop to 6, and a visible UI freeze.
• textContent +=: 108ms to populate the stylesheet, FPS drop to 16, and NO visible UI freeze.

I also improved the code to not loop through collapsed subfolder, and apply the color only when the parent folder gets opened, if any custom color was set, and of course only once.

The appendColor method doesn't run at all if no custom color is present in the folder database, or if the folder already has a custom color set.

Comment 4

[alta88]

Attachment: perf.png

Attached is a controlled, reproducible experiment that comes to a diametrically opposite conclusion as to which method is slower. The first half uses 1000 iterations, the second uses 100. Claiming insertRule() is 'super slow' seems super wrong.

Comment 5

[Alessandro Castellani [:aleca]]

Hey alta88, thanks for checking this as well.

I did a similar experiment with a simple HTML file, and I arrived to your same conclusions.
It's very strange as insertRule() is faster in a web page, but slower and causing UI freeze and drop in FPS in Thunderbird.

I'll attach the screenshots from Thunderbird profiling in a bit.
This is very strange.

Comment 6

[alta88]

It's demonstrably not insertRule() that is causing any of that. So let's put that meme to rest. The experiment is in the Tb developer toolbox, not a web page. I've already commented in the regressing bug on the problem being the design.

Comment 7

[Magnus Melin [:mkmelin]]

alta88, so what is the suggestion? To check if the row is outside the viewport and stop iterating then + keep a listener to retrigger setting as the viewport changes?

Comment 8

[Jorg K (CEST = GMT+2)]

(I guess, see bug 1637668 comment #51, right?)

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: performance-TB-UI.png

Here's a performance recording I did to compare how TB handles applying custom colors to 50 folders.

As you can see, using the sheet.insertRule() API causes a bottleneck in the Recalculate Style process.
This doesn't happen when injecting new rules in the stylesheet with textContent +=.

This happens because, whenever we add a new rule, the Recalculate Style process re-evaluates every selector and looks for matches in the DOM.

Indeed, my statement was not correct. The sheet.insertRule() API is not slower, but it's rather the Recalculate Style process getting called every time we add a new rule.

(I guess, see bug 1637668 comment #51, right?)

This design issue is solved in the patch I uploaded, which does the following:

• Don't loop through all the folders, only trigger the appendColor() when visible folders are generated on startup.
• Ignore subfolders if the parent is collapsed. Only trigger the appendColor() for subfolders when those become visible.
• Don't interact with the folder cache property if no custom colour was defined for that folder.
• Use textContent to avoid the Recalculate Style bottleneck caused by sheet.insertRule().

Comment 10

[Magnus Melin [:mkmelin]]

Comment on attachment 9159451 [details] [diff] [review]
1647251-folder-props.diff

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

Ok, let's land this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/06793905fcd8
Fix unresponsive script for Folder color customization. r=mkmelin

Comment 12

[Alessandro Castellani [:aleca]]

We should uplift this to esr78, not beta, right?

Comment 13

[Magnus Melin [:mkmelin]]

Yes, we should

Comment 14

[Alessandro Castellani [:aleca]]

Comment on attachment 9159451 [details] [diff] [review]
1647251-folder-props.diff

[Approval Request Comment]
Regression caused by (bug #): bug 1637668
User impact if declined: Startup performance issues with multiple folders
Testing completed (on c-c, etc.): c-c
Risk to taking this patch (and alternatives if risky): low as it improves the performance and prevents unnecessary interactions with hidden folders

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9159451 [details] [diff] [review]
1647251-folder-props.diff

Approved for ESR

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 78.0.0 (build2):
https://hg.mozilla.org/releases/comm-esr78/rev/8a5ead85ecd8