1185553 - Limit the number of reflows when entering the task manager

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Task Manager, defect)

Description

[Etienne Segonzac (:etienne)]

Opening the task manager is still painfully slow.
Long term, I'd like to use the windowmanager's DOM instead of making a whole new one each time.

But in the meantime, here are a few low hanging fruits:
* we should pause the statusbar as soon as |cardviewbeforeshow| is dispatched

* we have a innocent looking currentCard property that causes a reflow :/ we should make it a method (can be a separate bug). But more importantly, we should make sure we bypass it during the display phase of the cardview (we know the current index already, no need to go through the scrollOffset to get it

* haven't tried it, but we should and place all cards in a document fragment then append it at the end

Comment 1

[Marcus Cavanaugh [:mcav]]

I'm likely going to have bandwidth for this shortly; will assign myself when I know for sure.

Comment 2

[Sam Foster [:sfoster] (he/him)]

> Long term, I'd like to use the windowmanager's DOM instead of making a whole
> new one each time.

We still need to get the icon/button tray and title etc from somewhere. In the past I tried re-styling the AppWindow's identification overlay a while back to press it into service for the card view, and while it feels like the right thing to do, there was enough mis-match in requirements that it got awkward very quickly. Could be worth revisiting though. The guiding principle is that properties of an app window should be on AppWindow. Card.js should be extremely thin, if it needs to exist at all. Ideally, TaskMananger should have next to no rendering code. We have been slowly removing the barriers to make this possible, and the scroll snapping list was one of the big ones. 

> * we have a innocent looking currentCard property that causes a reflow :/ 

Doh, yeah good catch. In principle we shouldn't ever need to access the scrollLeft on the element if we just cache the offsets in a scroll handler. Why would making a it a method vs a getter make a difference - provided we consult a cached/calculated value?

> * haven't tried it, but we should and place all cards in a document fragment
> then append it at the end

Right, it should help and is cheap to implement and would give us finer control over exactly when we insert the cards - by decoupling the preparation of the DOM from the actual insertion and reflow(s).

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Sam Foster [:sfoster] from comment #2)
> > * we have a innocent looking currentCard property that causes a reflow :/ 
> 
> Doh, yeah good catch. In principle we shouldn't ever need to access the
> scrollLeft on the element if we just cache the offsets in a scroll handler.
> Why would making a it a method vs a getter make a difference - provided we
> consult a cached/calculated value?

No difference. Just that a getter looks more innocent :)
If getting the position is "free" then a getter makes perfect sense.
If not, then naming it `getPositionFromDOM()` will at least make people think twice before using it in a critical path.

Comment 4

[Sam Foster [:sfoster] (he/him)]

Its possible that adding a scroll event listener to cache the scroll offsets will take us off the fastest path, so we'll need to experiment and profile (and/or consult graphics team)

Comment 5

[Etienne Segonzac (:etienne)]

(In reply to Sam Foster [:sfoster] from comment #4)
> Its possible that adding a scroll event listener to cache the scroll offsets
> will take us off the fastest path, so we'll need to experiment and profile
> (and/or consult graphics team)

Yes, and worse case, we can keep the scrollLeft but make sure we're not using it on the critical path to launch (and that we rename it ;)).

Comment 6

[Etienne Segonzac (:etienne)]

I have a patch coming up for this one.

Comment 7

[GitHub Autolander Bot]

Attachment: [gaia] etiennesegonzac:bug-1185553 > mozilla-b2g:master

Comment 8

[GitHub Autolander Bot]

Attachment: [gaia] etiennesegonzac:bug-1185553 > mozilla-b2g:master

Comment 9

[Etienne Segonzac (:etienne)]

Comment on attachment 8671989 [details] [review]
[gaia] etiennesegonzac:bug-1185553 > mozilla-b2g:master

Here we go!
I left some comments on the PR to explain myself a little.

There a small part of the patch which is touching window manager stuffs, feel free to add :21 to the reviewers if you don't feel comfortable reviewing it (but it's definitely not mandatory).

Incidentally this might help with bug 1211663, or at least I can't reproduce it anymore.

Comment 10

[Etienne Segonzac (:etienne)]

Oh and of course this is not doing much until more than 10 apps are opened.

Comment 11

[Marcus Cavanaugh [:mcav]]

Comment on attachment 8671989 [details] [review]
[gaia] etiennesegonzac:bug-1185553 > mozilla-b2g:master

This looks good! It does seem smoother with lots of windows opened.

My comments are almost entirely just suggesting that we put some of your GitHub comments into actual source code comments; for optimization-type things especially, it's helpful to know why the code is doing things that might otherwise seem weird if they don't know it's for optimization purposes. But not required.

Comment 14

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/6ddf72e3155af59446556812dd97ab35f347f2af