Starting TB 78 on a profile with many folders gives unresponsive script error
Categories
(Thunderbird :: Folder and Message Lists, defect, P1)
Tracking
(thunderbird_esr78 fixed, thunderbird79 fixed)
People
(Reporter: jorgk-bmo, Assigned: aleca)
References
(Regression)
Details
(Keywords: perf, regression)
Attachments
(4 files, 1 obsolete file)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mkmelin
:
review+
wsmwk
:
approval-comm-esr78+
|
Details | Diff | Splinter Review |
(deleted),
image/png
|
Details | |
(deleted),
image/png
|
Details |
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?
Reporter | ||
Comment 1•4 years ago
|
||
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 2•4 years ago
|
||
I suspect it might be the very slow insertRule()
API.
Jorg, are you able to load and try this patch?
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 3•4 years ago
|
||
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.
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.
Assignee | ||
Comment 5•4 years ago
|
||
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.
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•4 years ago
|
||
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?
Reporter | ||
Comment 8•4 years ago
|
||
(I guess, see bug 1637668 comment #51, right?)
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
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 bysheet.insertRule()
.
Comment 10•4 years ago
|
||
Comment 11•4 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/06793905fcd8
Fix unresponsive script for Folder color customization. r=mkmelin
Assignee | ||
Updated•4 years ago
|
Updated•4 years ago
|
Updated•4 years ago
|
Assignee | ||
Comment 12•4 years ago
|
||
We should uplift this to esr78, not beta, right?
Assignee | ||
Comment 14•4 years ago
|
||
Comment 15•4 years ago
|
||
Comment 16•4 years ago
|
||
bugherder uplift |
Thunderbird 78.0.0 (build2):
https://hg.mozilla.org/releases/comm-esr78/rev/8a5ead85ecd8
Updated•4 years ago
|
Reporter | ||
Updated•4 years ago
|
Updated•2 years ago
|
Description
•