622090 - Stop using getBoundingClientRect in fennec MouseMove/dragMove (scrolling) code

Status: RESOLVED WONTFIX

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

Description

[Oleg Romashin (:romaxa)]

getBoundingClientRect - is very expensive function, which has expensive recursion and FlushLayout.

see https://bugzilla.mozilla.org/show_bug.cgi?id=606672#c36

We must rewrite that code and improve scrolling from 45->60 FPS

Comment 1

[Oleg Romashin (:romaxa)]

Attachment: Minor scrolling optimization

Comment 2

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

Comment on attachment 500335 [details] [diff] [review]
Minor scrolling optimization

I would love to make scrolling faster, but the _boundingClientRect change seems to fragile. I don't want to take it.

I'd definitely take the frameLoader.viewportScrollX/Y change by itself, if it makes any difference.

Comment 3

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

Attachment: wip

We could probably do something like that to be more robust

Comment 4

[Oleg Romashin (:romaxa)]

Here is another part:
things looks really bad here, because we do next:

- call getBoundingClientRect (call FlushLayout)
 http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1150
- scroll change parts of layout
 http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1170
- call again getBoundingClientRect which flushLayout e.t.c
http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#841

This makes scrolling FPS drop 60->47 :(

see https://bugzilla.mozilla.org/show_bug.cgi?id=606672#c36

Comment 5

[Ilkka Otsala]

Attachment: Patch proposal for feedback

Here is one approach for getting rid of getBoundingClientRect. The implementation is not bullet proof yet, there is some cases where calculations goes a bit wrong. 

But anyhow, to be able to get rid of those getBoundingClientRect calls, one possible approach is to maintain few variables for current visibility of side and top bars and according to those try calculate out how much certain side bar pans away.

I compared how much time is spent in both implementations, in the new one and
in old one. According to my measurements new implementation for vertical
panning is ~70% faster and for horizontal panning ~35% faster.

I really appreciate feedback and suggestions.

Comment 6

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

Comment on attachment 503242 [details] [diff] [review]
Patch proposal for feedback

Just some thoughts:
* This overall approach seems OK to me, but it is very complicated and will break easily as we change code around it
* There is a lot of "accounting" - left, right, top real sizes and visible sizes - making it a little hard to follow
* You use | getBrowser()._frameLoader.viewportScrollX | a few times. I think we should cache getBrowser() in a local and maybe add a helper method to browser.xml to get some of these values. Cache getBrowser(), yes. Add helpers to browser.xml, maybe.

I have said it before and will say it again: Mozilla uses element.boxObject to get width and height in many, many places. I would be fine with using it in the MainDragger code and the browser.xml code. That might make the patch much simpler and still improve performance.

So, feedback+, but we need to clean this up and potentially use boxObject as an alternative.

Comment 7

[Ilkka Otsala]

Attachment: panControlsAwayOffset patch V2

Rewrote the patch with following enhancements:
- no more member variables for visible pixels 
- less calculations
- faster: vertical 6 x faster, horizontal 5 x faster compared to original
- rewrote also tryFloatToolbar & tryUnfloatToolbar (getBoundingClientRect calls)

Comment 8

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

Comment on attachment 504687 [details] [diff] [review]
panControlsAwayOffset patch V2

>diff --git a/chrome/content/browser-ui.js b/chrome/content/browser-ui.js

>+  get tabsContainerW() {
>+    if (!this._tabsContainerW) {
>+      let tabsContainer = document.getElementById("tabs-container");
>+      this._tabsContainerW = tabsContainer.boxObject.width;
>+    }
>+    return this._tabsContainerW;
>+  },

The tabs container can change width as more tabs cause it to overflow. You can't cache this, imo.

We could just remove this function and use | Elements.tabs.boxObject.width | wherever you need it

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js

>   tryFloatToolbar: function tryFloatToolbar(dx, dy) {

>-    let [leftvis, ritevis, leftw, ritew] = Browser.computeSidebarVisibility(dx, dy);
>-    if (leftvis > 0 || ritevis > 0) {
>+    let controlsScrollBoxPosition = Browser.getScrollboxPosition(Browser.controlsScrollboxScroller);
>+    // if controlsScrollBoxPosition.x is smaller than BrowserUI.tabsContainerW, -> left side bar is visible
>+    // if controlsScrollBoxPosition.x is greater than BrowserUI.tabsContainerW, -> right side bar is visible
>+    // So, if controlsScrollBoxPosition.x != BrowserUI.tabsContainerW  -> either left or right tool bar is visible:
>+    if (controlsScrollBoxPosition.x != BrowserUI.tabsContainerW) {

Put the comments above the | let controlsScrollBoxPosition = ... |

Use | Elements.tabs.boxObject.width |

>   tryUnfloatToolbar: function tryUnfloatToolbar(dx, dy) {

>-    let [leftvis, ritevis, leftw, ritew] = Browser.computeSidebarVisibility(dx, dy);
>-    if (leftvis == 0 && ritevis == 0) {
>+    let controlsScrollBoxPosition = Browser.getScrollboxPosition(Browser.controlsScrollboxScroller);
>+    // For following comparison see tryFloatToolbar().
>+    if (controlsScrollBoxPosition.x == BrowserUI.tabsContainerW) {

Same

>   _panControlsAwayOffset: function(doffset) {
> 
>-    rect = Rect.fromRect(Browser.pageScrollbox.getBoundingClientRect()).map(Math.round);
>-    if (doffset.x < 0 && rect.right < window.innerWidth)
>-      x = Math.max(doffset.x, rect.right - window.innerWidth);
>-    if (doffset.x > 0 && rect.left > 0)
>-      x = Math.min(doffset.x, rect.left);
>+    // Check left and right side bars:
>+    if (doffset.x) {
>+      var leftSideBarVisiblePixels = 0;
>+      var rightSideBarVisiblePixels = 0;
>+      var controlsScrollBoxPosition = Browser.getScrollboxPosition(Browser.controlsScrollboxScroller); 

use | let |

>+      // if controlsScrollBoxPosition.x is smaller than BrowserUI.tabsContainerW, -> left side bar is visible
>+      // if controlsScrollBoxPosition.x is greater than BrowserUI.tabsContainerW, -> right side bar is visible
> 
>-    // XXX could we use getBrowser().getBoundingClientRect().height here?
>-    let height = Elements.contentViewport.getBoundingClientRect().height;
>-    height -= Elements.contentNavigator.getBoundingClientRect().height;
>+      if (controlsScrollBoxPosition.x < BrowserUI.tabsContainerW)
>+        leftSideBarVisiblePixels = BrowserUI.tabsContainerW - controlsScrollBoxPosition.x;
>+      else if (controlsScrollBoxPosition.x > BrowserUI.tabsContainerW)
>+        rightSideBarVisiblePixels = controlsScrollBoxPosition.x - BrowserUI.tabsContainerW;

Use | Elements.tabs.boxObject.width | but make a local variable

Watch for trailing spaces and prefer | let | over | var |

Comment 9

[Ilkka Otsala]

Attachment: panControlsAwayOffset patch V3

Fixed comments.

Comment 10

[Oleg Romashin (:romaxa)]

Attachment: Additional fix replace getBoundingClientRect->boxObject

Tested Ilkka's patch with this patch, and no more getboundclientrect calls and works fine

Comment 11

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

Just curious but if we starts to use boxObject, can't we use boxObject in computeSidebarVisibility instead of getBoundingClientRect() and save some code changes?

Comment 12

[Oleg Romashin (:romaxa)]

not sure... boxObject values quite good updated for things between dragStart and dragStop... but in between boxObject values much different from what getBoundingClientRect returns IIRC

Comment 13

[Oleg Romashin (:romaxa)]

And for this specific case we don't care about possibly updates of self.getBoundingClientRect between dragStart and dragStop.

Comment 14

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

Comment on attachment 505045 [details] [diff] [review]
panControlsAwayOffset patch V3

We need to test this for regressions, but the code looks good. I want to wait to land until after we branch for b4.

Comment 15

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

Comment on attachment 505422 [details] [diff] [review]
Additional fix replace getBoundingClientRect->boxObject

Is this part needed? I like your reasoning for not using boxObject, except during a drag. _getViewportSize looks like it could be called at non-dragging times as well.

Comment 16

[Oleg Romashin (:romaxa)]

Attachment: mouseMove ony for boxObject

hmm, another place where it used is _updateCacheViewport, and that is also called from mouseMove... tested previous patch quickly, and updateCacheViewport still works fine with boxObject. 
Here is the another (more safe) version of boundingclientRect removal, but I think we should be fine with first version too.

Comment 17

[Ilkka Otsala]

Attachment: panControlsAwayOffset & computeSidebarVisibility patch

Rewrote computeSidebarVisibility function to get rid of few getBoundingClientRect calls. Scrolling is now pretty much getBoundingClientRect free if Oleg's _getViewportSize patch is included as well.

Anyhow, on local pages there are still getBoundingClientRect calls, because receiving "scroll" message causes hideTitlebar and hideSidebars get called on each scroll movement. See: http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1140

Should we care much about local pages?

Comment 18

[Oleg Romashin (:romaxa)]

> receiving "scroll" message causes hideTitlebar and hideSidebars get called on
> each scroll movement. See:
> http://mxr.mozilla.org/mobile-browser/source/chrome/content/browser.js#1140
this looks very wrong to me.... local pages scrolling is not very fast at all. so I'm not sure do we need another bug for this issue or try to fix it here...

Comment 19

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

I tried this code:

browser.addEventListener("resize", function() { Cu.reportError("resizing browser"); }, false);

and it works! So we can add an event handler to browser XBL.

we could cache the bounding client rect and update when dirty. Make a _cachedBCR field and a _getBoundingClientRect() method that checks the field and returns it if not null or calls the real getBCR and sets _cachedBCR. That way we just replace all getBCR -> _getBCR in the binding.

Thoughts?

Comment 20

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

Just to follow thorugh: The "resize" hanlder in the browser binding would just set _cachedBCR back to null.

Comment 21

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

Ugh! This won't work for remote browsers, which get the event in the child process. I had a local browser open in the background during my tests :(

We could still try this idea and use a message in the browser binding. It would not instantaneous, but could be very close. We might even consider using a sync message from child process, if we found it was faster and we could keep the amount of work to a minimum - to reduce blocking affects.

Comment 22

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

Attachment: use "resize" and a cached BCR

Comment 23

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

Comment on attachment 506444 [details] [diff] [review]
use "resize" and a cached BCR

This patch uses a "resize" event and message to keep a cached BRC rect up to date in the browser widget. The cached BCR is used in a few places.

Comment 24

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

Attachment: use "resize" and a cached BCR #2

This patch adds a call to updateCacheViewport in the "resize" message handler, as suggested by Stover on IRC

Comment 25

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

Comment on attachment 505722 [details] [diff] [review]
mouseMove ony for boxObject

I'd rather use the cached BCR patch

Comment 26

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

Comment on attachment 505771 [details] [diff] [review]
panControlsAwayOffset & computeSidebarVisibility patch

I am slightly worried about the affect of boxObject in computeSidebarVisibility when called from other places in the code. I would want to test this a bit more, before saying "yes" or "no"

Comment 27

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

no of these patches are ready for checkin. I found issues with attachment 506447 [details] [diff] [review] that need to be debugged

Comment 28

[Oleg Romashin (:romaxa)]

mfinkle could you try to recall and explain what kind of issues you've seen with attachment 506447 [details] [diff] [review]? otherwise we kind of blocked and it is gonna be hard to improve scrolling speed without this fix... or we should create some other API or make simple version of GetBoundingClientRects...

Comment 29

[Oleg Romashin (:romaxa)]

Attachment: Updated to mc tip

Latest tip with bcr/resize patch:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fd8095bb0864/try-android-r7/fennec-6.0a1.en-US.eabi-arm.apk

Comment 30

[BMO Automation]

Closing all opened bug in a graveyard component