LayerRenderer.drawBackground can be expensive during a pageload

RESOLVED FIXED in Firefox 29

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mfinkle, Assigned: rnewman)

Tracking

(Depends on 1 bug, Blocks 1 bug, {perf})

Trunk
Firefox 29
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29+ fixed)

Details

Attachments

(4 attachments, 1 obsolete attachment)

While loading cnn.com, LayerRenderer.drawBackground shows up in profiles at #38, taking ~150ms called 59 times.

The expensive call seems to be drawing the shadowlayer:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/gfx/LayerRenderer.java#604

What do we use the shadow layer for?
Most of the time is taken up here
Blocks: 947390
Keywords: perf
OS: Linux → Android
Hardware: x86_64 → All
mShadowView is undocumented, and has been around since the very first implementation of LayerView:

changeset:   81790:eaf778e88070
user:        Patrick Walton <pwalton@mozilla.com>
date:        Wed Nov 09 17:39:29 2011 -0800
summary:     Bug 695448 - Implement a Java compositor, and use it to scroll. r=?

(which isn't attached to or discussed in that bug, nor any of its dependencies that I can see. Trail runs cold.)

Seeing needinfo on Patrick to comment on what it's for, and why it's so expensive.
Flags: needinfo?(pwalton)
The shadow layer is the shadow that you see around the content when you overscroll. Except that now you can't overscroll any more, so we can probably do without it.
Flags: needinfo?(pwalton)
Since overscrolling is gone, you can nuke the shadow layer.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Posted patch Eliminate shadow layer. v1 (obsolete) — Splinter Review
This seems to work on my device...
Attachment #8351240 - Flags: review?(bugmail.mozilla)
Also looks like overscroll was eliminated in 27, so this should probably be uplifted, no?
This also eliminates the overscroll color, which is also unnecessary.
Attachment #8351241 - Flags: review?(bugmail.mozilla)
Attachment #8351240 - Attachment is obsolete: true
Attachment #8351240 - Flags: review?(bugmail.mozilla)
Perf win with this patch, mfinkle?
Flags: needinfo?(mark.finkle)
(In reply to Richard Newman [:rnewman] from comment #8)
> Perf win with this patch, mfinkle?

LayerRenderer.drawBackground moves from #38 down to #195 in the profile list. I don't see any visual regressions with the patch either.
Flags: needinfo?(mark.finkle)
Comment on attachment 8351241 [details] [diff] [review]
Eliminate shadow layer. v2

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

LGTM. Also flagging wesj for feedback in case there's some reason this shouldn't come out.

::: mobile/android/base/gfx/LayerRenderer.java
@@ -137,5 @@
>      public LayerRenderer(LayerView view) {
>          mView = view;
> -        mOverscrollColor = view.getContext().getResources().getColor(R.color.background_normal);
> -
> -        CairoImage shadowImage = new BufferedCairoImage(view.getShadowPattern());

You can also get rid of LayerView.getShadowPattern and drawable-mdpi/shadow.png.
Attachment #8351241 - Flags: review?(bugmail.mozilla)
Attachment #8351241 - Flags: review+
Attachment #8351241 - Flags: feedback?(wjohnston)
Figured I'd land this and we can back it out or tweak it if Wes has further feedback. Thanks for the holiday review, kats!
Flags: needinfo?(wjohnston)
Target Milestone: --- → Firefox 29
https://hg.mozilla.org/mozilla-central/rev/b0b40893a1b4
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
There are a lot of times when the viewport still isn't set correctly (for instance, load Twitter, scroll down a ways in the stream, and then click reply on a tweet) leading to graphical corruption pretty frequently showing with this patch. Other than that, I was just holding off on doing this until we were sure the other stuff was good to go.
Flags: needinfo?(wjohnston)
(In reply to Wesley Johnston (:wesj) from comment #13)
> There are a lot of times when the viewport still isn't set correctly (for
> instance, load Twitter, scroll down a ways in the stream, and then click
> reply on a tweet) leading to graphical corruption pretty frequently showing
> with this patch. Other than that, I was just holding off on doing this until
> we were sure the other stuff was good to go.

I've seen this on, IIRC, the occasional Blogger site -- a hall of mirrors off to the side where the viewport wasn't right. Is this something that's on track to be fixed (and if so, what's the bug number?), or should we back this out?

(Turns out this is a nice panning perf win, so I'd definitely rather keep it than back it out!)
Flags: needinfo?(wjohnston)
No bug. You mind filing (and marking them blocking this?)?

Avoid uplifiting until we feel good about things, and be prepared re-evaluate and back this out on beta, but we really should fix the viewport bugs and this will help push us to do it.
Flags: needinfo?(wjohnston)
Depends on: 955956
Done. Marking tracking so we can watch this as 29 rides the trains.

(Also uncovered the excitement of Bug 955958 while I was trying to repro. Fun times.)
Depends on: 956939
If we decide to back this out because of graphical corruption, I would like to try a partial backout first. I originally saw the Shadow causing some slow down. We then decided to try to remove the Overflow color too. If the regressions are related to the Overflow color, let's only add that back.
Brian, Wes - Can you also check that this patch actually fixes the corruption? I have not seen it myself.
Attachment #8358608 - Flags: review?(wjohnston)
Flags: needinfo?(bnicholson)
Yep, this fixes bug 956939.
Flags: needinfo?(bnicholson)
Comment on attachment 8358608 [details] [diff] [review]
Add overflow color back v0.1

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

::: mobile/android/base/gfx/LayerRenderer.java
@@ +589,5 @@
>  
>              GLES20.glDisable(GLES20.GL_SCISSOR_TEST);
>  
> +            // Draw the overscroll background area as a solid color
> +            clear(mOverscrollColor);

I'd like to try using mBackgroundColor here. It should match the page and look more like the page just continues on. Up to you though.
Attachment #8358608 - Flags: review?(wjohnston) → review+
(In reply to Wesley Johnston (:wesj) from comment #20)
> Comment on attachment 8358608 [details] [diff] [review]
> Add overflow color back v0.1
> 
> Review of attachment 8358608 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/gfx/LayerRenderer.java
> @@ +589,5 @@
> >  
> >              GLES20.glDisable(GLES20.GL_SCISSOR_TEST);
> >  
> > +            // Draw the overscroll background area as a solid color
> > +            clear(mOverscrollColor);
> 
> I'd like to try using mBackgroundColor here. It should match the page and
> look more like the page just continues on. Up to you though.

We already do use the background color a few lines below, although we call setScissorRect first. Given that we are setting the background color and still saw a corruption, and Brian verified the current patch fixes bug 956939 - I want to just land this patch for now. We can file a new bug for removing the overflow color.

Perhaps we can really cleanup drawBackground:
* use background color instead of overflow color in the general clear() call
* stop calling setScissorRect and the now redundant clear with the background color
* stop calling GLES20.glDisable(GLES20.GL_SCISSOR_TEST); twice too
Blocks: 956939
No longer depends on: 956939
Attachment #8351241 - Flags: feedback?(wjohnston)
You need to log in before you can comment on or make changes to this bug.