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

RESOLVED FIXED in Firefox 43

Status

()

Core
Panning and Zooming
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Antti Tervasmäki, Assigned: botond)

Tracking

(Blocks: 1 bug, {perf, regression})

43 Branch
mozilla43
x86_64
Windows
perf, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox42 unaffected, firefox43 fixed)

Details

(URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Updated

2 years ago
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64

Comment 1

2 years ago
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
Blocks: 1177018
Status: UNCONFIRMED → NEW
tracking-e10s: --- → ?
Component: Untriaged → Panning and Zooming
Ever confirmed: true
Keywords: perf, regression
OS: Windows 10 → All
Product: Firefox → Core
Summary: 100% CPU by scrolling treeherder with e10s on → [APZ] 100% CPU by scrolling treeherder with e10s on

Updated

2 years ago
OS: All → Windows

Comment 2

2 years ago
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)
Blocks: 1178298
Botond, please take a look at this.
Flags: needinfo?(botond)
(Assignee)

Comment 4

2 years ago
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.
(Assignee)

Comment 5

2 years ago
Created attachment 8654971 [details]
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
Attachment #8654971 - Flags: review?(mstange)
(Assignee)

Comment 6

2 years ago
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.
Flags: needinfo?(botond)
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.
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
Assignee: nobody → botond
Status: NEW → ASSIGNED
status-firefox42: --- → unaffected
status-firefox43: --- → affected
(Assignee)

Comment 9

2 years ago
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.
Attachment #8654971 - Attachment is obsolete: true
Attachment #8654971 - Flags: review?(mstange)
(Assignee)

Comment 10

2 years ago
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
Attachment #8654971 - Attachment description: MozReview Request: Bug 1200158 - Simplify visible region after including the event regions in it. r=mstange → MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange
Attachment #8654971 - Attachment is obsolete: false
Attachment #8654971 - Flags: review?(mstange)
(Assignee)

Comment 11

2 years ago
Created 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 region computations in FindPaintedLayerFor(). r=mstange
Attachment #8655157 - Flags: review?(mstange)
(Assignee)

Comment 12

2 years ago
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 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
Attachment #8655157 - Flags: review?(mstange) → review+
Comment on attachment 8654971 [details]
MozReview Request: Bug 1200158 - Define PaintedLayerData::AccumulateEventRegions() out of line. r=mstange

https://reviewboard.mozilla.org/r/17779/#review15941
Attachment #8654971 - Flags: review?(mstange) → review+

Comment 15

2 years ago
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.
tracking-e10s: ? → +

Updated

2 years ago
Duplicate of this bug: 1200479
(Assignee)

Comment 17

2 years ago
(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.
(Assignee)

Comment 18

2 years ago
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
(Assignee)

Comment 19

2 years ago
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
Attachment #8655157 - Attachment description: MozReview Request: Bug 1200158 - Avoid expensive region computations in FindPaintedLayerFor(). r=mstange → MozReview Request: Bug 1200158 - Avoid expensive computations involving the maybe-hit region in hot code paths. r=mstange
(Assignee)

Comment 20

2 years ago
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.)
Attachment #8655157 - Flags: review+ → review?(mstange)
(Assignee)

Comment 21

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=617124363c7c
Attachment #8655157 - Flags: review?(mstange) → review+
(Assignee)

Comment 22

2 years ago
(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

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/825bd60177ed
https://hg.mozilla.org/integration/mozilla-inbound/rev/f385a5b64696
Blocks: 1201115
https://hg.mozilla.org/mozilla-central/rev/825bd60177ed
https://hg.mozilla.org/mozilla-central/rev/f385a5b64696
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox43: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.