1418816 - Table with a fixed top-left background image causes severe slowdown

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect, P2)

Description

[Sanqui]

Attachment: firefox-table-fixed-bg-freeze.html (this bugzilla-hosted copy doesn't trigger the bug, because of reference to relative file that's not there, url('bg.png');)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171113102334

Steps to reproduce:

Using Firefox 57 on Linux.
1. Open https://sanqui.net/etc/firefox-table-fixed-bg-freeze.html
2. Try scrolling on the page
3. Try highlighting some text using the mouse


Actual results:

Text is intermittently disappearing.  Highlighting text lags severely.  Switching to another tab takes a significant amount of time to render.  The system may freeze.


Expected results:

There should be no slowdown or rendering throttling of other tabs.

Additional information:
No significant slowdown is encountered when `browser.tabs.remote.force-disable` is set to true.

Comment 1

[Henri Kemppainen]

I can reproduce on Fedora, with 57 and nightly.

One of the web content processes is running out of file descriptors, with the usual symptoms.

Comment 2

[Henri Kemppainen]

Bumping the limit to 4k (see bug 1401776), my browser survives (it does get a little slow).  Fd use peaked above 3k, and now has settled at ~2300 for a few minutes.. that's pretty bad.

Comment 3

[Bas Schouten (:bas.schouten)]

Jamie, this is highly likely to be a layerization issue. We don't want it to be this easy to DoS the browser. How difficult would this be to address, or does some of your existing work already address this?

Comment 4

[Jamie Nicol [:jnicol]]

It looks similar to bug 1357359, the second part of which I couldn't fix. Basically:

* Each fixed position background item needs its own layer the size of the viewport.
* We can't optimize to image layers because the image is tiled.
* So each of them gets its own large painted layer

Last year I made a start at extending ImageLayers to work for this case, but wasn't able to finish it.
I'll give it another go.

Comment 5

[David Bolter [:davidb] (NeedInfo me for attention)]

Hi Jamie, we looked at this during QF triage. Is this a priority? I don't have depth of knowledge to understand impact.

Comment 6

[Jamie Nicol [:jnicol]]

I was actually working on this again this morning. I have a fix for some platforms (regular compositor), but not for advanced layers yet. Hopefully should next week.

It's hard to say how common this is in the real world, but my hunch is that it's worth fixing.

Comment 7

[Bas Schouten (:bas.schouten)]

Jamie, what's the news on this?

Comment 8

[Jamie Nicol [:jnicol]]

The solution is repeated image layers. Have it working with ImageLayerComposite. Not had much time to do the ImageLayerMLGPU implementation recently but I do plan to.

Comment 9

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 1: Add repeat size property to ImageLayer. r=mattwoodrow

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow

Depends on D4924

Comment 11

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 3: Handle repeated image layers in advanced layers. r=mattwoodrow

Depends on D4925

Comment 12

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 4: Allow image layers to be created for repeated backgrounds. r=mattwoodrow

Depends on D4926

Comment 13

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9006214 [details]
Bug 1418816 - Part 1: Add repeat size property to ImageLayer. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9006215 [details]
Bug 1418816 - Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 15

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9006216 [details]
Bug 1418816 - Part 3: Handle repeated image layers in advanced layers. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 16

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9006217 [details]
Bug 1418816 - Part 4: Allow image layers to be created for repeated backgrounds. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 17

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 1: Add repeat size property to ImageLayer. r=mattwoodrow

Comment 18

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow

Depends on D6024

Comment 19

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 3: Handle repeated image layers in advanced layers. r=mattwoodrow

Depends on D6025

Comment 20

[Jamie Nicol [:jnicol]]

Attachment: Bug 1418816 - Part 4: Allow image layers to be created for repeated backgrounds. r=mattwoodrow

Depends on D6026

Comment 21

[Jamie Nicol [:jnicol]]

Sorry for the spam, haven't quite figured out phabricator yet. The new commits are simply rebased and fix some merge conflicts.

Comment 22

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9009582 [details]
Bug 1418816 - Part 1: Add repeat size property to ImageLayer. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 23

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9009583 [details]
Bug 1418816 - Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 24

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9009584 [details]
Bug 1418816 - Part 3: Handle repeated image layers in advanced layers. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 25

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9009585 [details]
Bug 1418816 - Part 4: Allow image layers to be created for repeated backgrounds. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c4cec04f28d
Part 1: Add repeat size property to ImageLayer. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/807f0f510c25
Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/75d9a625be06
Part 3: Handle repeated image layers in advanced layers. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/d722f5a4a8af
Part 4: Allow image layers to be created for repeated backgrounds. r=mattwoodrow

Comment 27

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/3c4cec04f28d
https://hg.mozilla.org/mozilla-central/rev/807f0f510c25
https://hg.mozilla.org/mozilla-central/rev/75d9a625be06
https://hg.mozilla.org/mozilla-central/rev/d722f5a4a8af

Comment 28

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/c03ebd9cf525
Backed out 4 changesets for reftest perma failure bugs/289480.html a=backout

Comment 29

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4273b6a762ad
Backed out 4 changesets for reftest perma failure bugs/289480.html a=backout

Comment 30

[Andrei Ciure[:aciure]]

Backed out 4 changesets (bug 1418816) for reftest perma failure bugs/289480.html

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&revision=d722f5a4a8affac6aee03ed33bd72275c8bafbf2

failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel%2Crunning%2Cpending%2Crunnable&revision=d722f5a4a8affac6aee03ed33bd72275c8bafbf2&selectedJob=200170142&searchStr=android%2C4.3%2Capi16%2B%2Cdebug%2Creftests%2Ctest-android-em-4.3-arm7-api-16%2Fdebug-reftest-17%2Cr%28r17%29

backout: https://hg.mozilla.org/mozilla-central/rev/c03ebd9cf525d1118edaaaca0bdce72318ac049c

Comment 31

[Joel Maher ( :jmaher ) (UTC -8)]

as a note this was the root cause of some performance regressions:
== Change summary for alert #16026 (as of Wed, 19 Sep 2018 16:16:27 GMT) ==

Regressions:

778%  tp5o responsiveness linux64 pgo e10s stylo            0.66 -> 5.81
707%  tp5o_webext responsiveness linux64 pgo e10s stylo     1.18 -> 9.55
429%  tp5o_webext responsiveness linux64 opt e10s stylo     1.31 -> 6.96
428%  tp5o responsiveness linux64 opt e10s stylo            0.85 -> 4.48
  7%  tp5o_scroll linux64 opt e10s stylo                    1.15 -> 1.23
  5%  tps windows7-32 opt e10s stylo                        11.75 -> 12.35
  4%  tp5o_scroll windows7-32 pgo e10s stylo                1.38 -> 1.44
  4%  tp5o_scroll windows10-64 opt e10s stylo               1.37 -> 1.42

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=16026

Please look into these prior to landing again, the responsiveness regressions are quite extreme

Comment 32

[Mike Conley (:mconley) (:⚙️)]

Clearing needinfo so I can add a new one to hack myself on top of jnicol's stack. :)

Comment 33

[Mike Conley (:mconley) (:⚙️)]

Hey jnicol, is this still underway?

Comment 34

[Jamie Nicol [:jnicol]]

I've stopped looking at it since I was moved on to webrender. Feel free to take, it would be awesome to have it fixed!

FYI I think the regression is because we end up with very small images being repeated over a large area. And since this uses the naive approach of a separate draw call for each that is expensive. My intention was to use a heuristic based on the repeat size to visible region ratio when deciding whether to use an active layer. Or something like that.

Comment 35

[Jamie Nicol [:jnicol]]

Sorry, misread your needinfo hack comment as you intending to hack on top of my patch stack! But now realise it was just a nudge :)

Comment 36

[Gregory Pappas [:gregp]]

Performs well now.