Closed Bug 1192910 Opened 4 years ago Closed 4 years ago

Still scheduling paints when scrolling near the top or the bottom of the page

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
firefox48 --- fixed

People

(Reporter: mstange, Assigned: kats)

References

(Depends on 2 open bugs)

Details

(Keywords: perf, Whiteboard: [gfx-noted])

Attachments

(2 files, 2 obsolete files)

Scrolling in the center of the scrollable range is fine, but scrolling around the top and bottom is not. That's probably because we're clamping the display port to the scrollable range, so maybe the margins change relative to the scroll position?
This makes layout/reftests/scrolling/scroll-behavior-6.html fail.
Comment on attachment 8703147 [details]
MozReview Request: Bug 1192910 - When modifying display port margins, don't schedule paints if the actual display port hasn't changed. r?mattwoodrow

https://reviewboard.mozilla.org/r/29297/#review26949
Attachment #8703147 - Flags: review?(matt.woodrow) → review+
https://reviewboard.mozilla.org/r/29297/#review27191

::: layout/base/nsLayoutUtils.cpp:1114
(Diff revision 1)
> +  GetDisplayPort(aContent, &oldDisplayPort);

Not sure if this is causing that test to fail, but you'll probably need to check if you had a displayport here, otherwise it'll just leave oldDisplayPort untouched, and so comparing it later doesn't make much sense.
Duplicate of this bug: 1238805
This patch will need rebasing across bug 1241371.
Attached patch Patch rebased across bug 1241371 (obsolete) — Splinter Review
(In reply to Botond Ballo [:botond] from comment #6)
> This patch will need rebasing across bug 1241371.

I rebased it locally, posting in case it's useful for someone.
Keywords: perf
Whiteboard: [gfx-noted]
Stealing; I forgot there was already a patch here and ended up reinventing it. Since all of {mstange, mattwoodrow, tnikkel, botond} have been involved in this patch already I'm going to assume there are no objections here. I'll get the tests passing and land it.
Assignee: nobody → bugmail.mozilla
Getting the scroll-behavior-6 test passing turned out to be a more nontrivial than I had thought. The way reftests work is that they listen for paints on the main thread and update their snapshot based on that. So if we skip paints (which is what this patch does) then the reftest snapshot is potentially stale (which is what was happening). Specifically, the reftest did a smooth-scroll animation with a 500ms timeout to wait for it to finish. The animation did in fact finish just fine, but because it didn't trigger main-thread paints, it didn't update the snapshot in the reftest harness.

I have a fix which seems to work - it basically schedules a paint as part of the APZ flush which happens at the end of every reftest. Conceptually this feels correct, because it's APZ code that's leaving the main thread in an "unpainted" state, and the APZ flush is conceptually supposed to stop that from happening. So extending the APZ flush to cover this scenario seems reasonable.

There's a couple of other test failures that I'm working through as well.
The main patch. It's equivalent to what Markus had originally, so I kept Markus as the author and r=me.
Attachment #8703147 - Attachment is obsolete: true
Attachment #8713801 - Attachment is obsolete: true
Attachment #8727170 - Flags: review+
This fixes the tests, by compensating for the fact that we now sometimes skip paints.
Attachment #8727171 - Flags: review?(mstange)
Attachment #8727171 - Flags: review?(mstange) → review+
Backed out because the OS X 10.10 R-e10s(R-e10s) test permafailed after this push.

Backouts:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c68584f32e4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea7eb03c4cb9

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9066ef314419
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23173671&repo=mozilla-inbound

14:46:24     INFO -  REFTEST TEST-START | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST TEST-LOAD | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html | 13243 / 13292 (99%)
14:46:24     INFO -  REFTEST TEST-UNEXPECTED-FAIL | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html | failed reftest-no-paint
14:46:24     INFO -  REFTEST TEST-END | file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: START file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] OnDocumentLoad triggering WaitForTestEnd
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] WaitForTestEnd: Adding listeners
14:46:24     INFO -  REFTEST INFO | Saved log: Initializing canvas snapshot
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: waiting for MozAfterPaint
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] SendUpdateCanvasForEvent with 1 rects
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 0 0 800 1000
14:46:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FIRE_INVALIDATE_EVENT
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: dispatching MozReftestInvalidate
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AttrModifiedListener fired
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_REFTEST_WAIT_REMOVAL
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_SPELL_CHECKS
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: done requesting APZ flush
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] SendUpdateCanvasForEvent with 1 rects
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 0 -100 800 1536
14:46:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,0,800,1000
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_FOR_APZ_FLUSH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: apz-repaints-flushed fired
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FINISH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: waiting for MozAfterPaint
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] AfterPaintListener in file:///builds/slave/test/build/tests/reftest/tests/layout/reftests/invalidation/scroll-inactive-layers.html
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] SendUpdateCanvasForEvent with 1 rects
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] Rect: 0 1436 785 2230
14:46:24     INFO -  REFTEST INFO | Saved log: Updating canvas for invalidation
14:46:24     INFO -  REFTEST INFO | Saved log: DoDrawWindow 0,1000,785,0
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: STATE_WAITING_TO_FINISH
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] MakeProgress: Completed
14:46:24     INFO -  REFTEST INFO | Saved log: [CONTENT] RecordResult fired
14:46:24     INFO -  REFTEST INFO | Saved log: RecordResult fired
14:46:24     INFO -  REFTEST INFO | Saved log: RecordResult fired
Flags: needinfo?(bugmail.mozilla)
Relanded the patches because the backout didn't fix the issue.
Flags: needinfo?(bugmail.mozilla)
It looks like the backout *did* fix the issue; the R-e10s failures that came after the backout were some other failure.
I have a patch for bug 1253690 that should take care of this. Will reland this after that's in.
Depends on: 1253690
I put 1253690 on inbound, so I'm going to reland this. Hopefully it'll stick this time...
https://hg.mozilla.org/mozilla-central/rev/2a295c8a814b
https://hg.mozilla.org/mozilla-central/rev/3e9a803d69ed
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This is a perf improvement but not worth uplifting to beta.
Comment on attachment 8727170 [details] [diff] [review]
Part 1 - Don't schedule a paint if the displayport position didn't change

Approval Request Comment
[Feature/regressing bug #]: APZ
[User impact if declined]: when scrolling near the top or bottom of pages, we do extra repaints for no good reason. Wastes CPU time and power.
[Describe test coverage new/current, TreeHerder]: this code is exercised quite a bit during normal browser usage and automated tests if APZ is enabled.
[Risks and why]: low risk, change is well contained. The biggest risk is that it might expose bugs that were previously not as obvious (which IMO is a good thing) that might look bad. Bug 1255068 is an example of this and so far the only one. (I would like that bug uplifted to 47 as well)
[String/UUID change made/needed]: none
Attachment #8727170 - Flags: approval-mozilla-aurora?
Attachment #8727171 - Flags: approval-mozilla-aurora?
Comment on attachment 8727170 [details] [diff] [review]
Part 1 - Don't schedule a paint if the displayport position didn't change

Improves APZ perf, Aurora47+
Attachment #8727170 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8727171 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Wes Kocher (:KWierso) from comment #27)
> I had to back this out from aurora for android assertions like
> https://treeherder.mozilla.org/logviewer.html#?job_id=2205149&repo=mozilla-
> aurora
> 
> https://hg.mozilla.org/releases/mozilla-aurora/rev/e6bd5cd06759

seems this 2 pushes were innocent so relanded in 
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/9c1b7984c81c
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/ede02634231e
Flags: needinfo?(bugmail.mozilla)
Depends on: 1264297
Depends on: 1276850
Depends on: 1241550
Depends on: 1404218
Depends on: 1529324
Regressions: 1564071
You need to log in before you can comment on or make changes to this bug.