1930507 - gfx performance regression caused by sidebar transform

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, defect)

Description

[David Turner]

Steps to reproduce:

I have an automated test which plays back a 720p60 video on a Raspberry Pi 4 (Raspberry Pi OS bookworm, aarch64) and records the framerate. This test is GPU-limited on a Pi4.

Actual results:

In Firefox 131.0 I see an average framerate of around 50-51fps. In Firefox 132.0 I see an average framerate of around 43-44fps. I git bisected the issue down to git commit 18dc02a0, which is https://phabricator.services.mozilla.com/D220846 , Bug 1915230

The difference is very clear:

• Running 18dc02a0^ gives FPS measurements: 50.4 49.8 50.0 50.0 49.8
• Running 18dc02a0 gives FPS measurements: 43.3 44.1 43.4 43.6 42.7

I've confirmed the performance is still degraded in the latest nightly build.

I'm very confused why this change has caused this performance reduction given I don't even have the sidebar shown. But I've gone back and forth before/after this commit several times and the change is consistent.

Expected results:

No performance regression.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Graphics: WebRender' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

:emilio, since you are the author of the regressor, bug 1915230, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 3

[David Turner]

Attachment: video_player.html

Attached a simple reproducer (it's just a video player with an FPS display...), insert any 720p60 H.264 video. The main thing is the regression will only be visible on a platform GPU-constrained enough that it's dropping frames playing this video.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

That is a bit surprising... Wild guess, if you remove this line, does it get fixed?

That forces the tabbox to be a stacking context, and I wonder if somehow WebRender is doing something not-too-clever there.

Comment 5

[David Turner]

Yep, confirmed that removing that line fixes the problem.

What's extra weird is that I see the same performance degradation (which is also fixed by removing that line) with fullscreen video, so there's definitely something weird going on.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Could you try to make that line translate: 0 instead? Does it also get fixed in that case?

Trying to see if it's will-change specific, or not. I'm suspecting this code is causing us to always consider the tab box animated, which isn't great!

Comment 7

[Timothy Nikkel (:tnikkel)]

Is that not the intention of will-change? If not, how should we treat will-change differently?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1930507 - Don't consider will-change: transform an always-active layer. r=nical,#gfx-reviewers

will-change doesn't mean it is changing. We already did the same thing
for opacity years ago.

In particular, will-change is useful to prevent side effects when the
property changes (like going from being a containing block for fixed-pos
descendants to not being it), but it doesn't seem useful to force
layerization, and it slows down things like video playback.

Remove some now-dead code, which also gives us a frame bit, yay.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #6)

Could you try to make that line translate: 0 instead? Does it also get fixed in that case?

Trying to see if it's will-change specific, or not. I'm suspecting this code is causing us to always consider the tab box animated, which isn't great!

If we are hitting that code it means we hitting the blob fallback code when drawing our browser chrome? That doesn't seem good. Why is that happening?

Comment 10

[David Turner]

Could you try to make that line translate: 0 instead? Does it also get fixed in that case?

I've just tested and that doesn't fix the problem. Also D228650 does not help.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Is that not the intention of will-change? If not, how should we treat will-change differently?

It already forces a stacking context and all the other side effects of transform. I don't think we should treat it as active unconditionally.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

In any case we should probably move it to a different bug if it doesn't help. Maybe just making it a stacking context is what's causing the regression?

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1915230

Comment 14

[David Turner]

As a quick check I just enabled the tile-cache tree printer by making the print at picture.rs:5783 unconditional, but the trees look identical (aside from IDs) in the good and bad cases. I'd have thought this would show if the stacking context was resulting in the content getting put in another layer of picture, but my understanding of all this is very limited.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Yeah that was my guess too... Glenn / Nical, any idea on where to look next?

Comment 16

[Phabricator Automation]

Comment on attachment 9436827 [details]
Bug 1930507 - Don't consider will-change: transform an always-active layer. r=nical,#gfx-reviewers

Revision D228650 was moved to bug 1930674. Setting attachment 9436827 [details] to obsolete.

Comment 17

[Glenn Watson [:gw]]

Could you try enabling gfx.webrender.debug.profiler and gfx.webrender.debug.gpu-time-queries and seeing if there's any obvious differences in either the graphs or the counters (or take a screenshot / video of them, paste here and I'll see if I can spot any obvious differences)?

Comment 18

[Timothy Nikkel (:tnikkel)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

(In reply to Timothy Nikkel (:tnikkel) from comment #7)

Is that not the intention of will-change? If not, how should we treat will-change differently?

It already forces a stacking context and all the other side effects of transform. I don't think we should treat it as active unconditionally.

The reason it forces a stacking context and all the other side effects of a transform is so that it can animate without any extra layout work, ie the whole point of it is to animate.

Comment 19

[Timothy Nikkel (:tnikkel)]

Maybe you should share a profile using https://profiler.firefox.com/ in graphics preset?

Comment 20

[David Turner]

I've uploaded the charts as I'm not too sure what to look for.

• debug.profiler default UI: debug_profiler_{good,bad}.png - there's a little more time in Frame CPU total but nothing drastic. I've also captured separate screenshots with just the memory parts which got cropped off: gpu_mem_{good,bad}.png.
• gpu-time-queries doesn't work on Raspberry Pi because our GPU drivers don't implement the timers extension. I've captured it on my x86-64 laptop (x86_time_queries_{good,bad}.png) where the bad case does spend significantly more time in "Composite"
• Profiler captures, good case: https://share.firefox.dev/4fKp2xc, bad case: https://share.firefox.dev/40MbPjy

Overall there's definitely a difference in the render tree produced with will-change: translate but it's not obvious to me why it seems to be harder work. The "bad" case has fewer primitives/flips/pictures but more "images".

Comment 21

[David Turner]

Attachment: debug_profiler_bad.png

Comment 22

[David Turner]

Attachment: debug_profiler_good.png

Comment 23

[David Turner]

Attachment: gpu_mem_bad.png

Comment 24

[David Turner]

Attachment: gpu_mem_good.png

Comment 25

[David Turner]

Attachment: x86_time_queries_bad.png

Comment 26

[David Turner]

Attachment: x86_time_queries_good.png

Comment 27

[Glenn Watson [:gw]]

Set as S3, but might be S2 depending on how many users it has a noticeable effect on?

Comment 28

[Nicolas Silva [:nical]]

Attachment: Renderdoc captures with/without will-change

Surprisingly, renderdoc captures with (labeled bad) and without (labeled good) the will-change: translate; in browser/themes/shared/sidebar.css look pretty much identical.

These were made from wrench captures, forcing wrench to re-do the entire frame in both cases so it isn't quite what's happening in the browser (regarding invalidation/caching/etc.), but it's the best we can get in a tool like renderdoc.

Comment 29

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1915230

Comment 30

[Emilio Cobos Álvarez (:emilio)]

Bug 1933823 probably explains what's going on... I believe that will-change somehow messes up our opaque region tracking.

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Yeah, this would do. will-change adds a wrapping item, so bye opaque region. That's rather unfortunate. We should consider ancestor items at least.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Ironically, bug 1908471 will fix this if my diagnostic is correct, because it moves the background to the container element (#browser). Can you confirm once that lands?

Comment 33

[Emilio Cobos Álvarez (:emilio)]

(Not that we should rely on that, but that'd confirm the diagnostic)

Comment 34

[David Turner]

Bug 1933823 probably explains what's going on... I believe that will-change somehow messes up our opaque region tracking.

Aha, that makes perfect sense and explains what I've been seeing (why the webrender traces looked very similar between good/bad, and why things got even worse when I ran my compositor on sw rendering)

Ironically, bug 1908471 will fix this if my diagnostic is correct, because it moves the background to the container element (#browser). Can you confirm once that lands?

Yep, confirmed that applying D229676 / Bug 1908471 makes the badness go away.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Ok, I guess:

• This is fixed by bug 1908471.
• Poor / footgunny opaque region tracking is tracked in bug 1933952.
• A test in bug 1933958 should prevent us from accidentally regressing this... At least once we start running our Linux tests in a more modern version of ubuntu which is in the works.

So I guess closing as a dupe for now as there's not much else to do here? Let me know if you disagree tho.

Comment 36

[David Turner]

That sounds good to me, thanks! My own tests will also catch if this regresses and I'll make some noise.