Closed Bug 1200158 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Panning and Zooming, defect)

43 Branch
x86_64
Windows
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
e10s + ---
firefox42 --- unaffected
firefox43 --- fixed

People

(Reporter: anttit, Assigned: botond)

References

()

Details

(Keywords: perf, regression)

Attachments

(2 files)

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.
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
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
OS: All → Windows
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)
Botond, please take a look at this.
Flags: needinfo?(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.
Bug 1200158 - Simplify visible region after including the event regions in it. r=mstange
Attachment #8654971 - Flags: review?(mstange)
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
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)
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)
Bug 1200158 - Avoid expensive region computations in FindPaintedLayerFor(). r=mstange
Attachment #8655157 - Flags: review?(mstange)
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+
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.
Duplicate of this bug: 1200479
(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 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 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
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)
Attachment #8655157 - Flags: review?(mstange) → review+
(In reply to Botond Ballo [:botond] from comment #21)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=617124363c7c

Green modulo the infra bustage. Landing.
Blocks: 1201115
https://hg.mozilla.org/mozilla-central/rev/825bd60177ed
https://hg.mozilla.org/mozilla-central/rev/f385a5b64696
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.