Profile: http://people.mozilla.org/~bgirard/cleopatra/#report=1502035bb127ab8c08c3deef3463e54c4880945d doScroll is responsive for 16% of the time while scrolling the contact app.

Vivian mentioned this an hour ago. Great confirmation from the profile. Lets fix this. This will help all over the place.

We can probably avoid the sync reflow here if we track the last value via onscroll.

Blocking a blocker - needs an owner.

Hub, This is a 1.2 blocker I need you to make your #1 priority as it's blocking a Scrolling FPS regression.

Pinging Vivien to this, not sure the onscroll is the best solution as we may not have enough information to keep track in the scollevent, plus it means attaching a sync event handler thats constantly fired We may be able to keep track of the scroll position event ourself, we also need to try avoid doing 2 reads of offsetLeft etc when at least one and probably both are causing sync reflow

As long as you don't dirty the DOM in between, doing x queries only causes 1 reflows. So we're only going to get the win if we can avoid queries all properties that cause a sync reflow. Otherwise we need to move that work at some point after the primary draw reflow or perhaps the platform can have special cases to answer this particular case without having to reflow. You'd have to ask someone from Layout for that.

Paging dbaron or bz. If you have a suggestion for comment 5 and comment 6. Thanks !

Wip patch in which we only set the property once per doScroll(). I can't say if we get really better performance or not, but if someone else want to carry on and run test, you're welcome!

Thanks Fabrice ! I'll take over the patch.

Comment on attachment 803758 [details] [diff] [review] wip patch Review of attachment 803758 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementPanning.js @@ +401,3 @@ > // recalculate scrolling direction > + let xScrollable = node.scrollWidth > node.clientWidth; > + let yScrollable = node.scrollHeight > node.clientHeight; clientWidth/clientHeight will also reflow the dirty nodes I believe. :(

(In reply to Benoit Girard (:BenWa) from comment #10) > Comment on attachment 803758 [details] [diff] [review] > wip patch > > Review of attachment 803758 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementPanning.js > @@ +401,3 @@ > > // recalculate scrolling direction > > + let xScrollable = node.scrollWidth > node.clientWidth; > > + let yScrollable = node.scrollHeight > node.clientHeight; > > clientWidth/clientHeight will also reflow the dirty nodes I believe. :( Ha. Then we should cache them and listen for resizes.

(In reply to Fabrice Desré [:fabrice] from comment #11) > (In reply to Benoit Girard (:BenWa) from comment #10) > > Comment on attachment 803758 [details] [diff] [review] > > wip patch > > > > Review of attachment 803758 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: dom/browser-element/BrowserElementPanning.js > > @@ +401,3 @@ > > > // recalculate scrolling direction > > > + let xScrollable = node.scrollWidth > node.clientWidth; > > > + let yScrollable = node.scrollHeight > node.clientHeight; > > > > clientWidth/clientHeight will also reflow the dirty nodes I believe. :( > > Ha. Then we should cache them and listen for resizes. Those attributes can change their value based on any modification to the DOM or styles, not just window resizes, so caching them would probably not do the right thing.

The basic reason we end up doing layout when you ask for geometry information is that we assume you want up-to-date information. What information is the script actually looking for and why?

(In reply to Boris Zbarsky [:bz] from comment #13) > The basic reason we end up doing layout when you ask for geometry > information is that we assume you want up-to-date information. > > What information is the script actually looking for and why? We have a js implementation of kinetic panning in b2g (it will disappear, don't worry) that basically look for the element to scroll in the DOM, and moves it by changing scrollLeft/scrollTop or calling scrollBy() at regular intervals. We also use scrollWidth and clientWidth to check whether we can scroll at all. In a typical RAF callback, for a given node we: - read scrollWidth, scrollHeight, clientWidth, clientHeight. - assign a new value to scrollLeft and scrollTop. That leads to lots of "JS is triggering a sync reflow" in profiles. I have profiles where we spend 20% of our time reflowing (this is when scrolling a message thread in Gaia sms app).

OK. So we have some existing APIs on nsIDOMWindowUtils to get geometry information without flushing. The obvious problem with that approach is that you might get stale geometry information... Do you have some sort of out-of-band information that would tell you that the staleness is ok in this case?

A slightly different approach, where I make sure that we don't flush during the pan handler. That does not prevent all the JS sync reflow, and I could not really see obvious improvements in fps with this patch, but I may have missed something

Proposed patch following discussion with Vivien.

(In reply to Fabrice Desré [:fabrice] from comment #16) > Created attachment 806658 [details] [diff] [review] > wip 2 > > A slightly different approach, where I make sure that we don't flush during > the pan handler. That does not prevent all the JS sync reflow, and I could > not really see obvious improvements in fps with this patch, but I may have > missed something I talked to Vivien last Friday and we discussed a more adequate solution. See comment 17

Comment on attachment 807361 [details] [diff] [review] Bug 914854 - don't call scrollTop and scrollLeft that often. Review of attachment 807361 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementPanning.js @@ +407,2 @@ > xScrollable = node.scrollWidth > node.clientWidth; > yScrollable = node.scrollHeight > node.clientHeight; You're still touching these at every doScroll() and they cause a flush.

(In reply to Fabrice Desré [:fabrice] from comment #19) > Comment on attachment 807361 [details] [diff] [review] > Bug 914854 - don't call scrollTop and scrollLeft that often. > > Review of attachment 807361 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/browser-element/BrowserElementPanning.js > @@ +407,2 @@ > > xScrollable = node.scrollWidth > node.clientWidth; > > yScrollable = node.scrollHeight > node.clientHeight; > > You're still touching these at every doScroll() and they cause a flush. ew. yeah. good point. back to the drawing board.

Check my patch, it does avoid that at the cost of some c++

(In reply to Fabrice Desré [:fabrice] from comment #21) > Check my patch, it does avoid that at the cost of some c++ Indeed this patch show much better result in the profiler. Interestingly we don't really have any FPS change with b2gperf. That puzzles me.

Try build https://tbpl.mozilla.org/?tree=Try&rev=14579968fa56

I'll make a try push from m-c instead. The above is with my inbound tree. Not the best idea for green.

(In reply to Hubert Figuiere [:hub] from comment #17) > Created attachment 807361 [details] [diff] [review] > Bug 914854 - don't call scrollTop and scrollLeft that often. > > Proposed patch following discussion with Vivien. adequate solution is to avoid reflows. I think the question is why fabrice's patch still triggers JS reflows now.

This bug would result in a big overall improvement. Lets try to get this figured out.

(In reply to Fabrice Desré [:fabrice] from comment #16) > Created attachment 806658 [details] [diff] [review] > wip 2 > > A slightly different approach, where I make sure that we don't flush during > the pan handler. That does not prevent all the JS sync reflow, and I could > not really see obvious improvements in fps with this patch, but I may have > missed something Are you sure you are still seeing sync JS reflows? I see a lot of Main Thread IO when there is a - IPDL:PBrowser:RecvRealTouchMoveEvent - IPDL:PLayerTransaction:SendPGrallocBufferConstructor - IPDL:PLayerTransaction:SendUpdate But I don't see any js sync reflows in the profiler. For what it worth I'm trying in settings since contacts and sms have custom handlers to update the label based on the position in the list, and I assume that those can trigger js reflows.

Ok, so I was testing with sms using the reference-workload-light and a sms thread with images. I didn't test with the settings app, but your explanation makes sense. The main thread IO with IPC are a profiler bug (it erroneously thinks that the ipc write() happens on the main thread). So I guess I can clean up the patch a bit and ask for feedback to people that actually know this part of the codebase ;)

(In reply to Fabrice Desré [:fabrice] from comment #29) > Ok, so I was testing with sms using the reference-workload-light and a sms > thread with images. I didn't test with the settings app, but your > explanation makes sense. The main thread IO with IPC are a profiler bug (it > erroneously thinks that the ipc write() happens on the main thread). > > So I guess I can clean up the patch a bit and ask for feedback to people > that actually know this part of the codebase ;) For sms and contacts the plan is to use |position: sticky| ftw (http://coreyford.name/files/position-sticky-presentation/) to avoid JS here.

Vivien, I removed a lot of code from the BEP.js because I could not find any case where the patch is failing. If you have some, I'll add the DOMWindow code path back in. Boris, this patch adds a way to scroll elements without flushing layout. This helps a lot with scrolling performance in b2g (I now get consistently >55fps on a leo device)

Thanks Fabrice !

Fabrice this is awesome.

Comment on attachment 810702 [details] [diff] [review] patch >+ bool ScrollBy(int32_t aDx, int32_t aDy); This needs much better documentation about what the arguments mean. What I thought they meant is not what this patch implements (but more on that below). >+ nsIFrame* GetPrimaryFrame(mozFlushType aType, bool aFlushLayout = true); How about we introduce a Flush_None value for mozFlushType? Esp because what's happening here might not be a layout flush. >+ nsIScrollableFrame::DEVICE_PIXELS, Why? I would have expected CSS px here, not device px.... >+ CSSIntPoint after = sf->GetScrollPositionCSSPixels(); That's exploitable. See the comments about how scrollBy can destroy the world. >+ return (before.x != after.x || before.y || after.y); Why is the latter 2/3 of this condition correct? >+nsDOMWindowUtils::ScrollNode(nsIDOMNode* aNode, This is exposed to web pages, looks like. Is that expected? Seems like it would be better to have this as an Element method only exposed to privileged enough script.

Now using CSS pixels consistently, and a Flush_None type, along with fixes to glaring mistakes. (In reply to Boris Zbarsky [:bz] from comment #34) > That's exploitable. See the comments about how scrollBy can destroy the > world. It's not clear to me how I'm supposed to use it without destroying the world though. > > >+nsDOMWindowUtils::ScrollNode(nsIDOMNode* aNode, > > This is exposed to web pages, looks like. Is that expected? Seems like it > would be better to have this as an Element method only exposed to privileged > enough script. This is not exposed to content, like all the other methods from nsIDOMWindowUtils. I double checked on a web page.

> It's not clear to me how I'm supposed to use it without destroying the world though. You don't. What it means is that you can't use "sf" after the call without first checking whether it's died (using an nsWeakFrame). > This is not exposed to content, like all the other methods from nsIDOMWindowUtils Er... A bunch of windowutils methods have explicit "is caller chrome?" checks, precisely because in some cases content _can_ get its hands on a windowutils. Again, this should just be a method on Element, exposed to the relevant scopes. There's no need for the windowutils indirection here.

(In reply to Boris Zbarsky [:bz] from comment #36) > > Again, this should just be a method on Element, exposed to the relevant > scopes. There's no need for the windowutils indirection here. The caller being in chrome JS, I can't call directly on Element. That's the only reason for the windowutils indirection.

> The caller being in chrome JS, I can't call directly on Element. I don't mean mozilla::dom::Element. I mean Element.webidl.

Comment on attachment 810887 [details] [diff] [review] patch v2 Review of attachment 810887 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/browser-element/BrowserElementPanning.js @@ +395,4 @@ > > function doScroll(node, delta) { > + return utils.scrollNode(node, delta.x, delta.y); > + } You don't really need the doScroll function if you do that, you can just |isScrolling = utils.scrollNode(target, delta.x, delta.y);| in scroll() @@ +395,4 @@ > > function doScroll(node, delta) { > + return utils.scrollNode(node, delta.x, delta.y); > + } The case normally hit by |if (node instanceof Ci.nsIDOMWindow) | is when the body is not scrollable but is bigger than the window when the other cases are for scrollable divs for example. I don't think there is such cases in Gaia but you still want to check if this can happens. I suggest that you install a small app that contains nothing but a body taller and larger than the window. @@ +403,5 @@ > } > if (node.frameElement) { > return node.frameElement; > } > return null; nit: I guess this function can be written |return node.parentNode || node.frameElement || null;| @@ +407,5 @@ > return null; > } > > function scroll(delta) { > + let target; let's keep target out of this function next to isScrolling and firstScroll @@ +424,2 @@ > } > return isScrolling; let current = root; let firstScroll = true; function scroll(delta) { while (current) { if (utils.scrollNode(currentTarget, delta.x, delta.y)) { firstScroll = false; return true; } // TODO The current code looks for possible scrolling regions only if // this is the first scroll action but this should be more dynamic. if (!firstScroll) { return false; } current = ContentPanning._findPannable(targetParent(current)); } // There is nothing scrollable here. return false; } seems easier to read to me (but it could be just for me). @@ +424,4 @@ > } > return isScrolling; > } > + no need for the extra whitespace :)

Updated with : - no more nsIDOMWindowUtils indirection, but a [ChromeOnly] method in Element.idl - using a weakFrame to check whether we can still touch the scroll frame after scrolling. Vivien, I put the nsIDOMWindow code back because we need that for a lots of 3rd party apps, including the marketplace.

Comment on attachment 811407 [details] [diff] [review] patch v3 >+Element::ScrollBy(int32_t aDx, int32_t aDy) { Curly on next line, please. >+ return (before.x != after.x || before.y != after.y); return before != after; Please document in the IDL the units, and the fact that the scrolling does not flush? And make the method name, in both IDL and C++, reflect the non-flushing clearly. r=me with those fixed.

Comment on attachment 811407 [details] [diff] [review] patch v3 Review of attachment 811407 [details] [diff] [review]: ----------------------------------------------------------------- r+ with nits. ::: dom/base/nsDOMWindowUtils.cpp @@ +3426,5 @@ > GetComputedStyle(elem, aProperty, getter_AddRefs(style)); > NS_ENSURE_SUCCESS(res, res); > > return style->GetPropertyValue(aProperty, aResult); > } nit: I think you want to avoid this line change in this file that is unrelated to the patch. ::: dom/browser-element/BrowserElementPanning.js @@ +389,3 @@ > let firstScroll = true; > let target; > let isScrolling = false; nit: |isScrolling| is not needed anymore, let's remove it.

Also it may be helpful to have a followup for the |if doc instanceof Ci.nsIDOMWindow| if calling |getComputedStyle().overflow[X/Y] can trigger reflows (?) since this is call for each scrolling actions.

Follow up bug filed?

(In reply to Andreas Gal :gal from comment #45) > Follow up bug filed? yes, bug 922438