Closed Bug 1755493 Opened 3 years ago Closed 3 years ago

Poor scrolling performance on Google Slides when there's a border-radius on the <browser> element

Categories

(Core :: Graphics: WebRender, defect)

defect

Tracking

()

RESOLVED FIXED
99 Branch
Performance Impact ?
Tracking Status
firefox-esr91 --- unaffected
firefox97 --- unaffected
firefox98 --- unaffected
firefox99 --- fixed

People

(Reporter: mconley, Unassigned)

References

(Regression)

Details

(Keywords: regression)

We've noticed a recent performance regression while doing some experiments with different browser styles. In particular, our QA team has noticed that if we apply a border-radius to the <browser> element, scrolling on Google Slides gets way jankier.

Here are some performance profiles:

Good build: https://share.firefox.dev/3uWo25j

Bad build: https://share.firefox.dev/3sLjlsl

and here's the patch that can be applied to get the border-radius: https://hg.mozilla.org/try/rev/02c0143a641ff0e3773fb80f8a0674d38f98aade

Hey gw, we noticed a somewhat related issue a few days back where the border radius would cause the content area to get really blurry when it was scrollable. This was tracked down to bug 1749380 (https://hg.mozilla.org/mozilla-central/rev/b7f88e5c537bb7d64ba9d4cb58b052650d510638), but then the blurryness was fixed soon after... but we noticed this performance issue remained. Could it be related to the work in bug 1749380?

Flags: needinfo?(gwatson)

This problem is apparently more noticeable on Windows.

I've just noticed that the patches associated with bug 1749380 have been very recently backed out. We'll test to see if the performance issue has also been resolved.

Hey Rares, I have another try build for you to attempt to reproduce the performance issue on: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdd342684c918749ead9dce43067a1682803e7ad

If you're unable to reproduce the issue with that build, I think that strengthens the case that this was related to bug 1749380.

Flags: needinfo?(rares.doghi)

The bad build profile has broken symbols.

Also adding border-radius around the browser element will likely require some kind of masking or clipping of the scrollable layer which isn't supported by our native compositor integration (and perhaps not possible with DirectComposition/CoreAnimation) which means we lose native scrolling support which comes at a noticeable performance cost.

The bad build profile has broken symbols.

Argh. Do we not publish symbols for artifact builds?

isn't supported by our native compositor integration (and perhaps not possible with DirectComposition/CoreAnimation) which means we lose native scrolling support which comes at a noticeable performance cost.

Ah, so why are we only starting to hit this now? Is it because we're in the midst of transitioning to native compositor integration?

Hi Mike, I cant reproduce the issue in the latest Try build, I tried with multiple projects as well as the one where I kept seeing the issue and it doesnt occur anymore.

Flags: needinfo?(rares.doghi)

No, we've had native compositor integration for a while.

The problem should be showing up as soon as you add border-radius. This particular case is probably something extreme going wrong, unrelated to the compositor integration. Perhaps the general performance regression caused by adding border-radius just wasn't noticed?

Thanks, Rares. So I think that's a pretty strong case that it was regressed by bug 1749380.

(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)

Perhaps the general performance regression caused by adding border-radius just wasn't noticed?

Yes, that's certainly possible.

Yeah, see also bug 1745894, where I added a border-radius around the <body> element and it caused a similar perf regression.

It does sound like there's two potential issues here:

  1. The performance problem introduced by the bounding rect patches. I understand the root cause of these quite well (though getting the correct fix is tricky, which is why I've backed them out for now). Specifically, adding a border radius to an element that has a large local size, and a transform element that scales that element down by a large amount to device pixels is one way to trigger the performance problem - it causes render targets to be much larger than needed. This pattern seems to occur often when Gecko encounters SVGs with large view boxes.

  2. Adding a border radius to a top-level window (as Emilio mentioned above) will not allow a top-level surface to be handed off to native compositors. This can cause performance issues (though not as significant as 1), but also has a significant effect on power consumption, since the surface now needs to be masked and composited by WR on GPU before hand-off to the native OS compositor.

I can't say for sure from this whether both are relevant here, or only (1).

Flags: needinfo?(gwatson)

Even if this specific regression is addressed, adding border-radius to the browser element will remain to be a large performance and power usage regression overall. I would like to strongly recommend that no border-radius is used; it breaks basically all of WebRender's scroll optimizations and video playback optimizations.
It is possible to add support for border radius in our native compositors (at least it is on macOS, I'm not sure about Windows), but the graphics team would need to make an assessment of the amount of work that this involves.

As an alternative, the front-end code can achieve the same visual effect without applying border-radius to the browser element: If you want to round the top left corner and display a rounded border and a solid background color outside the rounded corner, you can achieve this effect by overlaying a small square element in the top left corner which overlaps the browser.
An overlapping element would not cause WebRender to fall down the slow path and should address the performance issue.

Demo of the overlapping corner approach: https://codepen.io/mstange/pen/GROMVba?editors=1100

Has Regression Range: --- → yes

Fixed by backout. I will be re-landing this patch series next week, with changes that fix this issue (for [1] from comment 11). Please re-open if it occurs again.

Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch
Performance Impact: --- → ?
Whiteboard: [qf]
You need to log in before you can comment on or make changes to this bug.