623485 - Scroll indicators for content scrolling regress scrolling speed from 55FPS -> 50 (SW) 22 (EGL HW)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Panning/Zooming, defect)

Description

[Oleg Romashin (:romaxa)]

I found that enabled scroll indicators are making fennec scrolling a bit slower in software rendering mode and much slower in EGL  rendering mode

Software 55->50 FPS drop
Hardware 55->21 FPS drop.

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #0)
> I found that enabled scroll indicators are making fennec scrolling a bit slower
> in software rendering mode and much slower in EGL  rendering mode
> 
> Software 55->50 FPS drop
> Hardware 55->21 FPS drop.

We knowingly took the regression in software because it was small enough and the feature was wanted. The slow down in EGL is surprising, we should dig into that.

Comment 2

[Oleg Romashin (:romaxa)]

Ok, I see two problems here:
1) on maemo6 I see lot of "status == LOCAL_GL_FRAMEBUFFER_COMPLETE" (this is EGL backend problem, I'll check that)

2) sounds like scroll indicators layer created incorrectly, not cached and updated too often
  a) Every 2-3 scroll movements/paints, we create new TextureImage 6x54 (vertical) and/or 451x6 (horizontal) scrolling indicator 
  b) In zoomed state on scroll we create big texture sz[766,440], and on each scroll event we update small region of that Texture(Layer). which is really wrong. it means we create big transparent ThebesLayer and repainting scrollbars on that layer on each scroll event...
  c) On each scroll paint we update scroll indicators layer.... this is totally wrong, indicators should be created on dragStart, painted in own layer and while scrolling only Texture Blit on GPU should happen.

Comment 3

[Oleg Romashin (:romaxa)]

So EGL specific slowness is - caused by wrong combination of
TextureImageEGL ctor, TextureImageEGL::Resize, SetBlitFramebufferForDestTexture.
Also Updating/uploading GL data into GPU on each scroll event very very bad thing.

2) a) b) c) cause slowness also for software rendering.

Comment 4

[Oleg Romashin (:romaxa)]

for fixing 2) I think we should move scroll indicators into separate image layer.. and make sure that layout does not do anything wrong by recognizing it as fixed positioned frames.

not sure yet how to specify that scroll indicator should be ImageLayer...
roc: could you advice any hits?

Comment 5

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

> Hardware 55->21 FPS drop.

Do we know what this looks like on Android?

Comment 6

[Oleg Romashin (:romaxa)]

Attachment: Revert scrollbars patch

would be nice if someone could test exact FPS number with this patch.

Comment 7

[Oleg Romashin (:romaxa)]

Probably we should take this patch in https://bugzilla.mozilla.org/attachment.cgi?id=495092&action=edit

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Are these indicators in XUL? We have a XUL attribute layers="true" that forces the element into its own layer.

Comment 9

[Oleg Romashin (:romaxa)]

> the element into its own layer.

Interesting, scrollindicators according to bug 461843 fix are "box" xul elements... but without layers="true"... I'll try that.

Comment 10

[Oleg Romashin (:romaxa)]

tried to setup layer="true" attr for scroll box, and that did not make any difference.. scroll indicators still updated on each paint.

- <box id="vertical-scroller" class="scroller" orient="vertical" right="2" top="0"/>
+ <box id="vertical-scroller" class="scroller" orient="vertical" right="2" top="0" layer="true"/>

-  <box id="horizontal-scroller" class="scroller" orient="horizontal" left="0" bottom="2"/>
+  <box id="horizontal-scroller" class="scroller" orient="horizontal" left="0" bottom="2" layer="true"/>

Comment 11

[Oleg Romashin (:romaxa)]

Attachment: Dump paint list output

Comment 12

[Oleg Romashin (:romaxa)]

Attachment: Texture Resize/paint with layer="true"

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: Texture Resize/paint with layer="false"

backtraces are different but in both cases we do repaint scrollindicator xull box on each motion event

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

break in ThebesLayerBuilder::InvalidateThebesLayerContents to see why we're doing that

Comment 15

[Oleg Romashin (:romaxa)]

Attachment: FrameLayerBuilder::InvalidateThebesLayerContents backtraces

Comment 16

[Oleg Romashin (:romaxa)]

I catched 3 different backtraces:
jar:https://bug623485.bugzilla.mozilla.org/attachment.cgi?id=505365!/back/back1.txt
jar:https://bug623485.bugzilla.mozilla.org/attachment.cgi?id=505365!/back/back2.txt
jar:https://bug623485.bugzilla.mozilla.org/attachment.cgi?id=505365!/back/back3.txt

have not tested yet which one is correct.

Comment 17

[Oleg Romashin (:romaxa)]

Ok , back2.txt and back3.txt are related to our xul box frame with layer="true"

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, so during scrolling we reflowing the scrollindicators and making them dirty with NS_FRAME_IS_DIRTY. You need to figure out what is causing them to have NS_FRAME_IS_DIRTY set.

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Set frame as dirty backtrace

Ok, we do change scroll indicator position with SetTop call... and that cause force layer reflow.

Comment 20

[Oleg Romashin (:romaxa)]

Should I just check that if size attribute (w | h) not changed and frame has layer flag then ignore DIRTY set for that frame?

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Can you use transforms to position the indicator instead?

Comment 22

[Benjamin Stover (:stechz)]

Attachment: Use transforms to move scrollbar

Comment 23

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

Comment on attachment 506918 [details] [diff] [review]
Use transforms to move scrollbar

Nice. My patch for this is almost identical.I would just drop the local "str" and set MozTransform directly.

Works well in my testing too.

Comment 24

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

Comment on attachment 506918 [details] [diff] [review]
Use transforms to move scrollbar

Oleg - I am happy to take this patch if you see an improvement

Comment 25

[Oleg Romashin (:romaxa)]

Thanks, I'll try it asap.

Comment 26

[Oleg Romashin (:romaxa)]

Comment on attachment 506918 [details] [diff] [review]
Use transforms to move scrollbar

Cool, this patch fixes all the problems.
I think we should push it ASAP.

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 506918 [details] [diff] [review]
Use transforms to move scrollbar

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>-    if (scaleX)
>-      this._horizontalScrollbar.left = contentScroll.x * scaleX;
>+    if (scaleX) {
>+      this._horizontalScrollbar.style.MozTransform = "translateX(" + Math.round(contentScroll.x * scaleX) + "px)";
>+    }

Nit: remove the {}

>     if (scaleY) {
>       const SCROLLER_MARGIN = 2;
>-      this._verticalScrollbar.top = contentScroll.y * scaleY;
>+      let y = Math.round(contentScroll.y * scaleY);
> 
>       // right scrollbar is out of view when showing the left sidebar,
>       // the 'solution' for now is to reposition it if needed
>+      let x;

You can just let x = 0; here and remove the else case

I can fix that when landing.

Comment 28

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Also SCROLLER_MARGIN seems unuseful now.

I want to wait for this afternoon to know if we want that for b4 or not before landing.

Comment 29

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

OK - Let's land the transform patch for beta 4

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

I've landed Ben's patch: http://hg.mozilla.org/mobile-browser/rev/bc006e989f9d

Is there anything else to do for this bug?

Comment 31

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

(In reply to comment #30)
> I've landed Ben's patch: http://hg.mozilla.org/mobile-browser/rev/bc006e989f9d
> 
> Is there anything else to do for this bug?

nope. sounds like this fixed the performance problem.

Comment 32

[Oleg Romashin (:romaxa)]

yes, with this I see only texture composition to GPU, no updates, no repaints