Don't create displayports during display list building (in most common cases)

RESOLVED FIXED in Firefox 58

Status

()

RESOLVED FIXED
2 years ago
6 months ago

People

(Reporter: mattwoodrow, Assigned: tnikkel)

Tracking

Trunk
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
For retained-display lists we really need to know what the AGR is for a given frame *before* we start display list building (to determine what we need to build).

We currently only decide (some) displayports during building, so the results we looked up earlier might be incorrect, and nsDisplayListBuilder::mFrameToAnimatedGeometryRootMap will be out of date.

We currently hack around a subset of this problem using RecomputeAnimatedGeometryRoot, but calling FindAnimatedGeometryRoot for entirely outside of building (from nsLayoutUtils::PaintFrame) isn't fixed by that.

Can we instead compute this information upfront, so that once we setup the builder, all AGRs decisions are constant?

It looks like the main reason we do this is to mark the 'main' subframe as scrollable, and then possibly mark displayports on any ancestors of this as we bubble back up.

Could we do this work during reflow, or possibly even just a separate pass at the start of painting (during the outermost call to EnterPresShell?) so that we don't have to make modifications during building.

Getting rid of RecomputeAnimatedGeometryRoot seems like a pretty good win regardless.
Timothy, do you recall in what cases we forcibly set display ports on ancestor scroll frames and in what we rely on the propagation during display list building?

I'm really looking forward to a world in which the ancestor activation during display list building no longer happens.
Flags: needinfo?(tnikkel)
(Assignee)

Comment 2

2 years ago
There are two ways we currently create displayports during display list building.
1) We create a display port (if one does not exist) on the first scroll frame we encounter during display list building that is async scrollable.
2) If we encounter an async scrollable scroll frame that does not have a displayport but some descendant of it does have a displayport.

1) is handled properly by the current code, but retained display lists can't really handle that. The retained display list branch moves this displayport creation to immediately before display list building.

2) isn't really handled well by the current code. It should be quite rare. Whenever we set a displayport we should be setting a displayport on all async scrollable ancestors (there might be code paths where we don't that we should fix). However this doesn't prevent a page from moving a dom node with a displayport to be a descendant of a scrollframe that is async scrollable but that doesn't have a displayport. And thus we'd have to create a displayport during display list building.

Does that answer your question?
Flags: needinfo?(tnikkel)
Yes it does, thank you.

(In reply to Timothy Nikkel (:tnikkel) from comment #2)
> he retained display list branch moves this displayport
> creation to immediately before display list building.

So could we just land that piece in mozilla-central, as a patch for this bug?
(Assignee)

Comment 4

2 years ago
(In reply to Markus Stange [:mstange] from comment #3)
> (In reply to Timothy Nikkel (:tnikkel) from comment #2)
> > he retained display list branch moves this displayport
> > creation to immediately before display list building.
> 
> So could we just land that piece in mozilla-central, as a patch for this bug?

Sure, I think we mainly filed this bug because of retained display lists though.
(Reporter)

Comment 5

2 years ago
Extracted this from the graphics branch, do you think it's ready for review?
Flags: needinfo?(tnikkel)
(Reporter)

Updated

2 years ago
Assignee: nobody → tnikkel
(Assignee)

Comment 6

2 years ago
This patch contains the functional change. In the next patch we clean up this code.
Attachment #8911007 - Attachment is obsolete: true
Flags: needinfo?(tnikkel)
Attachment #8912132 - Flags: review?(mstange)
(Assignee)

Comment 7

2 years ago
Posted patch simplifySplinter Review
Attachment #8912133 - Flags: review?(mstange)
Comment on attachment 8912132 [details] [diff] [review]
create displayports before displaylist building

Review of attachment 8912132 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsLayoutUtils.cpp
@@ +3394,3 @@
>  nsLayoutUtils::MaybeCreateDisplayPort(nsDisplayListBuilder& aBuilder,
> +                                      nsIFrame* aScrollFrame,
> +                                      RepaintMode aRepaintMode) {

Might as well move the opening brace into the next line, since you're touching this line.
Attachment #8912132 - Flags: review?(mstange) → review+
Attachment #8912133 - Flags: review?(mstange) → review+
So this fixes 1) but not 2). Do you have plans to address 2)?
(Assignee)

Comment 10

2 years ago
(In reply to Markus Stange [:mstange] from comment #9)
> So this fixes 1) but not 2). Do you have plans to address 2)?

Not really. :) 2) should be relatively rare and the current code doesn't even deal with it properly. Retained display lists would probably make it a worse problem though when it does happen.

Ideas for fixing it:
1) if we have to create a displayport during display list building throw away the display list and start over after creating the display port
2) schedule another paint, when we do the next paint we'd create the displayport before starting the paint.
Both sound ok to me. Fixing this would let us get rid of AutoCurrentActiveScrolledRootSetter::InsertScrollFrame. But we don't have to do it now.
(Assignee)

Updated

2 years ago
Summary: Don't create displayports during display list building → Don't create displayports during display list building (in most common cases)
(Reporter)

Comment 12

2 years ago
Can we also delete RecomputeCurrentActiveGeometryRoot?
Once 2) is fixed, yes.

Comment 14

2 years ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/45a57448087d
Walk the frame tree looking for a scrollframe to add a displayport to, or one that already has a displayport before displaylist building. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7be72224105
Simplify some code now that we don't create displayports during display list building. r=mstange
Backed out for failing mochitest gfx/layers/apz/test/mochitest/test_bug982141.html on Android 4.3 API 16:

https://hg.mozilla.org/integration/mozilla-inbound/rev/91a6629e22f771b1a6ef892d8221dff6e7613857
https://hg.mozilla.org/integration/mozilla-inbound/rev/55795addb20e552ebd55834bed45324207cd57a2

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=e7be7222410583294475ff4c8bf649b9373d1760&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=133751207&repo=mozilla-inbound

[task 2017-09-28T07:21:21.264Z] 07:21:21     INFO -  391 INFO TEST-START | gfx/layers/apz/test/mochitest/test_bug982141.html
[task 2017-09-28T07:21:21.264Z] 07:21:21     INFO -  Buffered messages logged at 07:21:18
[task 2017-09-28T07:21:21.264Z] 07:21:21     INFO -  392 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | expected at least one paint in compositor test data
[task 2017-09-28T07:21:21.264Z] 07:21:21     INFO -  393 INFO TEST-PASS | gfx/layers/apz/test/mochitest/test_bug982141.html | found the RCD node
[task 2017-09-28T07:21:21.264Z] 07:21:21     INFO -  Buffered messages finished
[task 2017-09-28T07:21:21.265Z] 07:21:21     INFO -  394 INFO TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_bug982141.html | expected a single child APZC - got +0, expected 1
[task 2017-09-28T07:21:21.265Z] 07:21:21     INFO -      SimpleTest.is@SimpleTest/SimpleTest.js:312:5
[task 2017-09-28T07:21:21.265Z] 07:21:21     INFO -      testBug982141@gfx/layers/apz/test/mochitest/helper_bug982141.html:67:7
[task 2017-09-28T07:21:21.265Z] 07:21:21     INFO -      afterPaint@gfx/layers/apz/test/mochitest/helper_bug982141.html:31:7
[task 2017-09-28T07:26:29.972Z] 07:26:29     INFO -  395 INFO TEST-UNEXPECTED-FAIL | gfx/layers/apz/test/mochitest/test_bug982141.html | Test timed out.
[task 2017-09-28T07:26:29.974Z] 07:26:29     INFO -      reportError@SimpleTest/TestRunner.js:121:7
[task 2017-09-28T07:26:29.974Z] 07:26:29     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:142:7
Flags: needinfo?(tnikkel)
(Assignee)

Comment 16

2 years ago
Ah, the problem was that for deck frames we traverse into the first child regardless of which card in the deck is selected. Display list building only ever traverses into the selected card. I changed the new code to do the same.
Flags: needinfo?(tnikkel)

Comment 17

2 years ago
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ac9fffe4f7f
Walk the frame tree looking for a scrollframe to add a displayport to, or one that already has a displayport before displaylist building. r=mstange
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c8b85e80aeb
Simplify some code now that we don't create displayports during display list building. r=mstange
https://hg.mozilla.org/mozilla-central/rev/5ac9fffe4f7f
https://hg.mozilla.org/mozilla-central/rev/4c8b85e80aeb
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58

Updated

2 years ago
Depends on: 1405397

Comment 19

2 years ago
Bisection with Mozregression-gui implicates this bug as regressing bug 1381708

Updated

2 years ago
Depends on: 1381708

Updated

a year ago
Depends on: 1407267
Depends on: 1406364
You need to log in before you can comment on or make changes to this bug.