952878 - LayerRenderer.drawBackground can be expensive during a pageload

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: profile-cnn-shadowlayer-1.png

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?

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: profile-cnn-shadowlayer-3.png

Most of the time is taken up here

Comment 2

[Richard Newman [:rnewman]]

mShadowView is undocumented, and has been around since the very first implementation of LayerView:

changeset:   81790:eaf778e88070
user:        Patrick Walton <[e-mail]>
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.

Comment 3

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

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.

Comment 4

[Patrick Walton (:pcwalton)]

Since overscrolling is gone, you can nuke the shadow layer.

Comment 5

[Richard Newman [:rnewman]]

Attachment: Eliminate shadow layer. v1

This seems to work on my device...

Comment 6

[Richard Newman [:rnewman]]

Also looks like overscroll was eliminated in 27, so this should probably be uplifted, no?

Comment 7

[Richard Newman [:rnewman]]

Attachment: Eliminate shadow layer. v2

This also eliminates the overscroll color, which is also unnecessary.

Comment 8

[Richard Newman [:rnewman]]

Perf win with this patch, mfinkle?

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 10

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

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.

Comment 11

[Richard Newman [:rnewman]]

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!

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b0b40893a1b4

Comment 13

[Wesley Johnston (:wesj)]

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.

Comment 14

[Richard Newman [:rnewman]]

(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!)

Comment 15

[Wesley Johnston (:wesj)]

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.

Comment 16

[Richard Newman [:rnewman]]

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.)

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: Add overflow color back v0.1

Brian, Wes - Can you also check that this patch actually fixes the corruption? I have not seen it myself.

Comment 19

[Brian Nicholson (:bnicholson)]

Yep, this fixes bug 956939.

Comment 20

[Wesley Johnston (:wesj)]

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.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(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

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Pushed partial backout
https://hg.mozilla.org/integration/fx-team/rev/e9f1c39fe072

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e9f1c39fe072