Open Bug 1418816 Opened 2 years ago Updated 9 months ago

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

Categories

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

defect

Tracking

()

REOPENED

People

(Reporter: me, Assigned: jnicol)

References

Details

(Keywords: perf, Whiteboard: [qf:p2:responsiveness])

Attachments

(5 files, 4 obsolete files)

16.45 KB, text/html
Details
46 bytes, text/x-phabricator-request
mattwoodrow
: review+
Details | Review
46 bytes, text/x-phabricator-request
mattwoodrow
: review+
Details | Review
46 bytes, text/x-phabricator-request
mattwoodrow
: review+
Details | Review
46 bytes, text/x-phabricator-request
mattwoodrow
: review+
Details | Review
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.
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.
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.
Component: Untriaged → DOM: Content Processes
Keywords: perf
Product: Firefox → Core
Whiteboard: [qf]
Priority: -- → P2
Component: DOM: Content Processes → Graphics
Attachment #8929880 - Attachment description: firefox-table-fixed-bg-freeze.html → 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');)
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?
Assignee: nobody → jnicol
Flags: needinfo?(jnicol)
Whiteboard: [qf] → [qf][qf:p1][qf:f60]
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.
Component: Graphics → Graphics: Layers
Flags: needinfo?(jnicol)
See Also: → 1357359
Whiteboard: [qf][qf:p1][qf:f60] → [qf:p1][qf:f60]
Whiteboard: [qf:p1][qf:f60] → [qf:f61][qf:p1]
Whiteboard: [qf:f61][qf:p1] → [qf:f64][qf:p1]
Whiteboard: [qf:f64][qf:p1] → [qf:p1:f64]
Hi Jamie, we looked at this during QF triage. Is this a priority? I don't have depth of knowledge to understand impact.
Flags: needinfo?(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.
Flags: needinfo?(jnicol)
Jamie, what's the news on this?
Flags: needinfo?(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.
Flags: needinfo?(jnicol)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 9006214 [details]
Bug 1418816 - Part 1: Add repeat size property to ImageLayer. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9006214 - Flags: review+
Comment on attachment 9006215 [details]
Bug 1418816 - Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9006215 - Flags: review+
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.
Attachment #9006216 - Flags: review+
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.
Attachment #9006217 - Flags: review+
Sorry for the spam, haven't quite figured out phabricator yet. The new commits are simply rebased and fix some merge conflicts.
Attachment #9006214 - Attachment is obsolete: true
Attachment #9006215 - Attachment is obsolete: true
Attachment #9006216 - Attachment is obsolete: true
Attachment #9006217 - Attachment is obsolete: true
Comment on attachment 9009582 [details]
Bug 1418816 - Part 1: Add repeat size property to ImageLayer. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9009582 - Flags: review+
Comment on attachment 9009583 [details]
Bug 1418816 - Part 2: Handle repeated image layers in ImageLayerComposite. r=mattwoodrow

Matt Woodrow (:mattwoodrow) has approved the revision.
Attachment #9009583 - Flags: review+
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.
Attachment #9009584 - Flags: review+
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.
Attachment #9009585 - Flags: review+
Pushed by jnicol@mozilla.com:
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
Backout by aciure@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/c03ebd9cf525
Backed out 4 changesets for reftest perma failure bugs/289480.html a=backout
Backout by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4273b6a762ad
Backed out 4 changesets for reftest perma failure bugs/289480.html a=backout
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla64 → ---
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

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

Flags: needinfo?(jnicol)

Hey jnicol, is this still underway?

Flags: needinfo?(jnicol)
Whiteboard: [qf:p1:f64] → [qf:p2:responsiveness]

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.

Flags: needinfo?(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 :)

You need to log in before you can comment on or make changes to this bug.