Open Bug 1594587 Opened 5 years ago Updated 2 years ago

Investigate why HTMLScrollFrame regresses browser.xhtml

Categories

(Core :: Layout, enhancement)

Product:
Core
Component:
Layout
Type:
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.

Talos Regressions

Profile with NO scroll frame
Profile with scroll frame and overflow: hidden

Component: XUL Widgets → Layout
Product: Toolkit → Core

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

Flags: needinfo?(bdahl)

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

Flags: needinfo?(bdahl)

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?

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.

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.

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.

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?

Flags: needinfo?(bdahl)

Results are still coming in but:

Flags: needinfo?(bdahl)

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.

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

That's still a good find (safe change and nice relative improvement). We should land along with any platform optimizations.

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?

Flags: needinfo?(bdahl)

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

Flags: needinfo?(bdahl)

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.

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.