Investigate why HTMLScrollFrame regresses browser.xhtml
Categories
(Core :: Layout, enhancement)
Tracking
()
People
(Reporter: bdahl, Unassigned)
References
(Blocks 1 open bug)
Details
When migrating browser.xhtml from a XUL DOM structure to an HTML DOM structure there were a few talos regressions (tart, tresize, and tscrollx). The regressions were fixed by preventing the HTMLScrollFrame from being created for browser.xhtml. Controlling if the scroll frame is created can be toggled removing the attribute scrolling="false" on the root html element.
Profile with NO scroll frame
Profile with scroll frame and overflow: hidden
|
Reporter | |
Updated•5 years ago
|
|
||
Comment 1•5 years ago
|
||
So, there's a bunch of regressions, but there's also a bunch of comparable improvements in ts_paint.
Presumably the ts_paint progression was also related to the scrollframe? That's quite odd...
|
|
Reporter | |
Comment 2•5 years ago
|
||
(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)
So, there's a bunch of regressions, but there's also a bunch of comparable improvements in ts_paint.
Presumably the ts_paint progression was also related to the scrollframe? That's quite odd...
Ignore the ts_paint improvement. That was a bug in how XULPersist was restoring window sizes. The bug caused XULPersist to handle the width/height of the window instead of AppWindow(previously named nsXULWIndow). We should actually file a bug to look into those improvements. However, with that bug you also run into bug 1590044.
|
|
||
Comment 3•5 years ago
|
||
So, ok, I don't see anything terrible in those profiles (the tart ones). I'd be curious about tresize.
In tart, the initial <browser> creation is a bit different because somehow we're hitting the eager frame construction, and that may cause various stuff to run.
Are those patches rebased enough to compare apples-to-apples, without the tpaint noise?
|
|
Reporter | |
Comment 4•5 years ago
|
||
They both are from MC on Tue, 05 Nov 2019 which includes the XULPersist fixes so the ts_paint issue shouldn't be there. I triggered the talos-chrome jobs on the above try runs, so we should have tresize profiles soon.
|
|
||
Comment 5•5 years ago
|
||
So I thought tresize was going to be the easiest thing to profile, since we should be able to see the difference consistently.
One of the things that stick out is that we still spend time updating scrollbars even in the overflow: hidden case, because we always create scrollbars even for overflow: hidden on the root... Not 100% sure that will account for all the regression though, scrollbars seem like a fairly small part of the page... Bug 1590247 would probably make this unnecessary, as the only reason we always create scrollbars on the root is to handle overflow: auto -> overflow: hidden more nicely, and bug 1590247 would make the optimization much more generic.
cc'ing some folks which may want to take a look at the profile and may have more insights.
|
|
Reporter | |
Comment 6•5 years ago
|
||
Here's another more recent talos run with current mozilla/central vs scroll="false" removed. All the same regressions are still there.
I noticed in one of the tresize profiles that ScrollFrameHelper::LayoutScrollbars was showing up, so I tried disabling that when scrollbar-width: none, but there were no changes on talos.
|
|
||
Comment 7•5 years ago
|
||
So the patch above sets overflow: hidden on both the root and the <body>... Could you only set it on one of them (on the :root) instead?
|
|
Reporter | |
Comment 8•5 years ago
|
||
Results are still coming in but:
- mozilla/central vs scroll=false removed and overflow:hidden only on :root - same regressions as overflow:hidden on :root and body
- overflow:hidden :root and body vs just :root - no difference
|
|
Reporter | |
Comment 9•5 years ago
|
||
I enabled display list dumping and diffed with the scrollframe enabled/disabled and noticed the #fullscreen-and-pointerlock-wrapper div showing up with scrollframes enabled, but not scrollframes disabled. I changed the wrapper to display:none and that seemed to help tresize performance locally, unfortunately it only seems to help tart and tresize (linux only) on talos. Compared to the current mozilla/central (no scrollframe) performance is still worse though.
- scroll frame enabled vs scroll frame enabled WITH hiding fullscreen-and-pointerlock-wrapper - some tart and tresize improvements
- mozilla/central vs scroll frame enabled WITH hiding fullscreen-and-pointerlock-wrapper - still lots of regressions
|
||
Comment 10•5 years ago
|
||
(In reply to Brendan Dahl [:bdahl] from comment #9)
I enabled display list dumping and diffed with the scrollframe enabled/disabled and noticed the
#fullscreen-and-pointerlock-wrapperdiv showing up with scrollframes enabled, but not scrollframes disabled. I changed the wrapper todisplay:noneand that seemed to help tresize performance locally, unfortunately it only seems to help tart and tresize (linux only) on talos. Compared to the current mozilla/central (no scrollframe) performance is still worse though.
- scroll frame enabled vs scroll frame enabled WITH hiding fullscreen-and-pointerlock-wrapper - some tart and tresize improvements
- mozilla/central vs scroll frame enabled WITH hiding fullscreen-and-pointerlock-wrapper - still lots of regressions
That's still a good find (safe change and nice relative improvement). We should land along with any platform optimizations.
|
||
Comment 11•4 years ago
|
||
Hi Brendan!
I was looking into getting rid of the XUL codepath in DevTools highlighters.
I just saw that this scrollframe issue is blocking it:
https://searchfox.org/mozilla-central/rev/027893497316897b8f292bde48dbb6da2391a331/devtools/server/actors/highlighters/utils/markup.js#113-120
I was wondering if the regression was still occuring. In 7months, may be something automagically fixed this regression?
If not, is there still a group or individuals spending times on removing XUL bits?
Otherwise, I did not follow the dexulification project... Do you know if we still use XUL documents somewhere?
May be not anymore in Firefox, but still on Thunderbird? KaiOS?
|
|
Reporter | |
Comment 12•4 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #11)
I was wondering if the regression was still occuring. In 7months, may be something automagically fixed this regression?
If not, is there still a group or individuals spending times on removing XUL bits?
I'm not aware of anything that has changed recently to fix the regressions, but I don't really watch the layout code. You could try rebasing the above patches and doing some new talos runs. There's no longer a dedicated team for XUL removal.
Otherwise, I did not follow the dexulification project... Do you know if we still use XUL documents somewhere?
May be not anymore in Firefox, but still on Thunderbird? KaiOS?
There are no more .xul files, but some of the XHTML files still use a "XUL like" structure e.g. <window>.....
|
|
||
Comment 13•3 years ago
|
||
Note that in bug 1442600 we will enable highlighters against browser.xhtml which used to be disabled since https://phabricator.services.mozilla.com/D52104 landed.
So it looks like things have been fixed and we can now use insertAnonymousContent just fine.
|
||
Updated•2 years ago
|
Description
•