1187432 - Don't schedule paints for scrolls handled by APZ

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Matt Woodrow (:mattwoodrow)]

When we have a scroll event has been handled by APZ (and APZ hasn't requested a content repaint because we're running out of displayport), we should avoid scheduling a paint on the main thread.

Currently we always process the scroll events on the main thread as well (to get up to date clicking handling information), but this also schedules a paint.

This paint will trigger full display list building, layerization, and then a layer transaction to the compositor. However, since we snap the displayport to tile boundaries, the majority of these will results in no changes other than a different transform on the scrollable layer (a change which APZ has already made).

This is a lot of wasted work, so we should try avoid it.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

(In reply to Matt Woodrow (:mattwoodrow) from comment #0)
> Currently we always process the scroll events on the main thread as well (to
> get up to date clicking handling information), but this also schedules a
> paint.

Where does this happen?

Comment 2

[Matt Woodrow (:mattwoodrow)]

I can get a full callstack soon, but ScrollFrameHelper::ScrollVisual() is calling SchedulePaint.

There may be other places too, unsure.

Comment 3

[Matt Woodrow (:mattwoodrow)]

Ugh, there appears to be 3 callers.

nsSliderFrame::CurrentPositionChanged

nsLayoutUtils::SetDisplayPortMargins

mozilla::ScrollFrameHelper::ScrollVisual

Blocking all of those might be a pain.

Stacks: https://pastebin.mozilla.org/8840593

Comment 4

[Matt Woodrow (:mattwoodrow)]

It looks like we snap display port margins to tile boundaries when we *retrieve* them, not when we set them.

We'd have to move that to the setter in order to skip the SchedulePaint when they are the same.

Comment 5

[Matt Woodrow (:mattwoodrow)]

Attachment: avoid-scheduling-apz-paints

This seems to work really well.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8643271 [details] [diff] [review]
avoid-scheduling-apz-paints

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

This looks sane to me fwiw

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2247,5 @@
>  
> +  if (LastScrollOrigin() != nsGkAtoms::apz) {
> +    mOuter->SchedulePaint();
> +  } else {
> +    // If this was an apz scroll, and then displayport (relative to the

s/then/the/

Comment 7

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8643271 [details] [diff] [review]
avoid-scheduling-apz-paints

Note that this is going to make our bugs with perspective transforms and background-attachment:fixed worse, since we won't immediately update the positions with a main thread transaction.

Comment 8

[Matt Woodrow (:mattwoodrow)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +2247,5 @@
> >  
> > +  if (LastScrollOrigin() != nsGkAtoms::apz) {
> > +    mOuter->SchedulePaint();
> > +  } else {
> > +    // If this was an apz scroll, and then displayport (relative to the
> 
> s/then/the/

Thanks, fixed locally.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8643271 [details] [diff] [review]
avoid-scheduling-apz-paints

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2225,5 @@
>      gScrollFrameActivityTracker->AddObject(this);
>    }
>  }
>  
> +void ScrollFrameHelper::ScrollVisual(nsPoint aOldScrolledFramePos, const nsRect& aDisplayPort)

Can we call this aOldDisplayPort?
And I'd do the shifting both by the old and by the new scroll position in this function.

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: avoid-scheduling-apz-paints v2

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8643287 [details] [diff] [review]
avoid-scheduling-apz-paints v2

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

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2437,5 @@
>  
> +  nsRect displayPort;
> +  nsLayoutUtils::GetDisplayPort(content, &displayPort);
> +
> +  // Update frame position for scrolling

I'll move this comment back.

Comment 12

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8643287 [details] [diff] [review]
avoid-scheduling-apz-paints v2

Do you want to move the display port equal computation into ScrollToImpl and just pass a bool to ScrollVisual? nsPoint aOldScrolledFramePos on ScrollVisual is unused so you can remove that to clean it up a little.

Then you can assert or check that we actually have a displayport is the last scroll origin was apz.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c599591ec73d

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c599591ec73d