1200158 - [APZ] 100% CPU by scrolling treeherder with e10s on

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Antti Tervasmäki]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20150830030224

Steps to reproduce:

Go to treeherder (aka tinderbox), scroll to see more.. 


Actual results:

Plugin-container.exe starts to consume 100% CPU from one core and that tab practically freezes, scrolled page is drawn slowly in about 2 minutes, response to any mouse click, link or other tab, takes minutes to happen...




Expected results:

Scroll nicely like without e10s...

This does not happen with 2015-08-28 Nightly so it is regression.

Comment 1

[Alice0775 White]

Reproduced on windows7 and ubuntu14.04 with enabled e10s and APZ

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


Regressed by:Bug 1177018

Comment 2

[Alice0775 White]

STR
1. Open https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound
2. Scroll page by scrollbar(click scrollbar/scroll buttun or drag thumb) 
   or Scroll page by keyboards(up/down arrow etc)

Comment 3

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

Botond, please take a look at this.

Comment 4

[Botond Ballo [:botond]]

Investigating.

Markus had an idea that the problem might be that we're not simplifying the visible region after OR-ing the hit region into it.

Comment 5

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

Bug 1200158 - Simplify visible region after including the event regions in it. r=mstange

Comment 6

[Botond Ballo [:botond]]

It's hard for me to tell whether this helps because in my debug build, everything is slow, but I guess simplifying the region is something we should be doing anyways.

Comment 7

[Markus Stange [:mstange]]

The patch doesn't help much for me; my debugger tells me we're spending lots of time in the call to ScaleRegionToOutsidePixels here:
visibleRegion.OrWith(contState.ScaleRegionToOutsidePixels(data.mMaybeHitRegion));

So it looks like we need to simplify mMaybeHitRegion sooner.

Comment 8

[Markus Stange [:mstange]]

Yeah, this patch helps a lot more:

> diff --git a/layout/base/FrameLayerBuilder.cpp b/layout/base/FrameLayerBuilder.cpp
> --- a/layout/base/FrameLayerBuilder.cpp
> +++ b/layout/base/FrameLayerBuilder.cpp
> @@ -422,16 +422,17 @@ public:
>     * PaintedLayer.
>     */
>    void AccumulateEventRegions(nsDisplayLayerEventRegions* aEventRegions)
>    {
>      FLB_LOG_PAINTED_LAYER_DECISION(this, "Accumulating event regions %p against pld=%p\n", aEventRegions, this);
> 
>      mHitRegion.Or(mHitRegion, aEventRegions->HitRegion());
>      mMaybeHitRegion.Or(mMaybeHitRegion, aEventRegions->MaybeHitRegion());
> +    mMaybeHitRegion.SimplifyOutward(8);
>      mDispatchToContentHitRegion.Or(mDispatchToContentHitRegion, aEventRegions->DispatchToContentHitRegion());
>      mNoActionRegion.Or(mNoActionRegion, aEventRegions->NoActionRegion());
>      mHorizontalPanRegion.Or(mHorizontalPanRegion, aEventRegions->HorizontalPanRegion());
>      mVerticalPanRegion.Or(mVerticalPanRegion, aEventRegions->VerticalPanRegion());
>    }
> 
>    /**
>     * If this represents only a nsDisplayImage, and the image type supports being

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8654971 [details]
MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

After talking to Markus and Jeff, here's what we arrived at:

  - FindPaintedLayerFor() is a very hot code path. It's called
    for every display item on every paint.

  - I added code to this code path to scale hit regions and
    OR them into the visible region, using the result instead
    of the visible region.

  - The hit regions, particularly, the maybe-hit region, can
    be complex regions (regions with many rects).

  - Scaling the hit regions in FindPaintedLayerFor() is very
    wasteful, because we're doing the same scaling operation
    for every display item for a given layer. We should scale
    the regions once per layer, store the result, and use that
    in the hot code path.

  - Even with the scaling moved out of the hot code path, working
    with a complex maybe-hit region in the hot code path is
    undesirable, so we should simplify the maybe-hit region.
    The simplification should be done outside the hot code path,
    as in Markus' patch.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8654971 [details]
MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

Comment 11

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1200158 - Avoid expensive computations involving the maybe-hit region in hot code paths. r=mstange

Bug 1200158 - Avoid expensive region computations in FindPaintedLayerFor(). r=mstange

Comment 12

[Botond Ballo [:botond]]

The attached patches implement the approach described in comment 9. Subjectively, they improve the performance but not as much as I would have expected; this probably deserves some further investigation.

Comment 13

[Markus Stange [:mstange]]

Comment on attachment 8655157 [details]
MozReview Request: Bug 1200158 - Avoid expensive computations involving the maybe-hit region in hot code paths. r=mstange

https://reviewboard.mozilla.org/r/17849/#review15939

Comment 14

[Markus Stange [:mstange]]

Comment on attachment 8654971 [details]
MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

https://reviewboard.mozilla.org/r/17779/#review15941

Comment 15

[Sergio]

I'm not sure this is related or needs a new bug, with apz and e10s enabled webs like https://github.com/mozilla-jetpack/jpm/commits/master (any github commit page) or https://www.meneame.net/ have performance problems while using autoscroll (middle mouse click or pageUp pageDown) (100% cpu usage on one core) scrolling using the mouse wheel seems to work fine.

with layers.async-pan-zoom.enabled false the auto scroll feature works a lot better and cpu usage is lower.

Comment 17

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #12)
> The attached patches implement the approach described in comment 9.
> Subjectively, they improve the performance but not as much as I would have
> expected; this probably deserves some further investigation.

After getting the profiler working properly, it was straightforward to see the remaining problem: the maybe-hit region on Treeherder is so complex that even scaling it in AccumulateEventRegions() takes up too much time. Simplifying it after each call to AccumulateEventRegions() relieves the problem.

Comment 18

[Botond Ballo [:botond]]

Comment on attachment 8654971 [details]
MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

Comment 19

[Botond Ballo [:botond]]

Comment on attachment 8655157 [details]
MozReview Request: Bug 1200158 - Avoid expensive computations involving the maybe-hit region in hot code paths. r=mstange

Bug 1200158 - Avoid expensive computations involving the maybe-hit region in hot code paths. r=mstange

Comment 20

[Botond Ballo [:botond]]

Comment on attachment 8655157 [details]
MozReview Request: Bug 1200158 - Avoid expensive computations involving the maybe-hit region in hot code paths. r=mstange

Requesting re-review of this patch. (Apparently MozReview doesn't support this.)

Comment 21

[Botond Ballo [:botond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=617124363c7c

Comment 22

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #21)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=617124363c7c

Green modulo the infra bustage. Landing.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/825bd60177ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f385a5b64696

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/825bd60177ed
https://hg.mozilla.org/mozilla-central/rev/f385a5b64696