1201548 - [APZ] Laggy scrolling on certain page

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Alice0775 White]

STR
1. Open https://www.reddit.com/r/canada/
   or https://www.reddit.com/r/CrappyDesign/
2. Scroll page by scrollbar(click scrollbar/scroll buttun or drag thumb) 
   or Scroll page by keyboards(up/down arrow etc)

Actual Results:
Scrolling is slow and laggy.


Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=81dd43b43784&tochange=2e4a1822a511

Regressed by:Bug 1177018


Bug 1200158 does not fix this slow/laggy scroll.

Comment 1

[Alice0775 White]

This page is also affected
 https://www.reddit.com/r/ImGoingToHellForThis/  (May contain undesirable content)

Comment 2

[Alice0775 White]

Attachment: zipped html for the record

Comment 3

[mdew]

Seems to affected by the background image, if you select the option on the right "Use subreddit style", performance is better.

Comment 4

[Botond Ballo [:botond]]

Thanks for the report!

The time is being spent in PaintedLayerData::AccumulateEventRegions(), scaling the hit region. Looks like the hit region can also be very complex (bug 1200158 only addressed the maybe-hit region). On the page from comment 2, it has over 800 rects.

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

Bug 1201548 - Simplify the hit region in AccumulateEventRegions(). r=mstange

This avoids the subsequent scaling, and the use of the region in
FindPaintedLayerFor(), being too expensive.

Comment 6

[Botond Ballo [:botond]]

This patch simplifies the hit region in AccumulateEventRegions(), the way we did for the maybe-hit region in bug 1200158.

If we're concerned about the correctness implications of simplifying the hit region, there are a couple of alternative approaches I can envision:

  - Simplify a copy of the hit region and scale that, leaving the original
    unsimplified. This way, the only place that sees the simplified version 
    is FindPaintedLayerFor().

  - Continue to scale the unsimplified region, but only do it to the final
    accumulated region the first time FindPaintedLayerFor() needs it, not in 
    every call to AccumulateEventRegions().

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #6)
>   - Simplify a copy of the hit region and scale that, leaving the original
>     unsimplified. This way, the only place that sees the simplified version 
>     is FindPaintedLayerFor().

Markus suggested a variation on this where we just use the bounds of the event regions in FindPaintedLayerFor().

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8657176 [details]
MozReview Request: Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

This is done for performance reasons, as the event regions can be complex.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8657176 [details]
MozReview Request: Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

https://reviewboard.mozilla.org/r/18325/#review16427

::: layout/base/FrameLayerBuilder.cpp:2600
(Diff revision 2)
> -          visibleRegion.OrWith(data.mScaledMaybeHitRegion);
> +          if (data.mScaledHitRegionBounds.Intersects(aVisibleRect)) {
> +            break;
> +          }
> +          if (data.mScaledMaybeHitRegionBounds.Intersects(aVisibleRect)) {

Let's combine those two if statements with ||.

::: layout/base/FrameLayerBuilder.cpp:3459
(Diff revision 2)
> +  mHitRegion.SimplifyOutward(8);

I think we shouldn't simplify the hit region for correctness reasons.

Comment 10

[Botond Ballo [:botond]]

(In reply to Markus Stange [:mstange] from comment #9)
> ::: layout/base/FrameLayerBuilder.cpp:3459
> (Diff revision 2)
> > +  mHitRegion.SimplifyOutward(8);
> 
> I think we shouldn't simplify the hit region for correctness reasons.

Ah, sorry, I meant to remove that from the patch!

Comment 11

[Botond Ballo [:botond]]

Comment on attachment 8657176 [details]
MozReview Request: Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

This is done for performance reasons, as the event regions can be complex.

Comment 12

[Markus Stange [:mstange]]

Comment on attachment 8657176 [details]
MozReview Request: Bug 1201548 - When testing whether the visible rect intersects event regions in FindPaintedLayerFor(), only use the bounds of the event regions. r=mstange

https://reviewboard.mozilla.org/r/18325/#review16433

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/065f00e9b2fd

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/065f00e9b2fd