984414 - Closing a collection has a very expensive reflow in it (45 ms. on Keon)

Status: RESOLVED WORKSFORME

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Jan Jongboom [:janjongboom] (inactive)]

Reflow info when closing a collection.

reflow: 4.7ms
reflow: 2.88ms
reflow: 2.6ms
reflow: 8.96ms
reflow: 1.29ms
reflow: 45.13ms <-- what's going on here?
reflow: 1.38ms

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

So this bug got a bit bigger than I wanted at first but hey. Most important:

* Removes the big reflow in closing a collection
* Brings number of reflows when closing collection down to 2
* Brings number of reflows when opening collections back to 1 (only setTitle in Collection.js reflows). Not counting building the icons.
* Doesn't create a reflow for every new icon

So the patch consists of four commits, to make it a bit easier to review. They are:

596b6c2 - This is the big reflow that we saw in the original comment on this issue (the 45 ms one). Had to remove the overflow: hidden tho, if there is a case in which this is going to cause problems then I'd like to hear about it. (f? for crdlc)

f1a4bc8 - This removes the reflow on toggling 'visible' on #collection. In here the collection container gets display: none when the header anim is done. I solved it in CSS now.

f8e39ee - We create & remove bgimage-overlay every time we go in/out a collection. I have no clue why, because I could never see the element as the background of the collection is set on another element. But this commit re-uses the element instead of throwing it away every time.

163110c - This looks very big but it's mainly because I had to restructure Result.js so that the init method is on the prototype. At the moment the browser doesn't know what the image size is going to be of the icon+text, and thus has to resize the moment the icon is loaded (as the text is already). I added width/height attributes on both the img elements to avoid that. Still for some reason a reflow still happens when setting the src on the image, so using background-image for it... And that way we never reflow anymore when rendering icons (was 1 reflow per image cause the icons are async).

Comment 2

[Cristian Rodriguez (:crdlc)]

The code LGTM although I can see wrong behaviors that I will explain in the best way so you can understand me :)

----

1) Restart device
2) Once the homescreen is rendered, click on "Social" Smart Collection

Look at the header of the collection while it is loading, you can see a string "Source:" on the top left during a couple of seconds

----

1) Search something on ev.me
2) Swipe to next page
3) Go back to ev.me search

The dock appears and disappears in a strange way thought (although I am not pretty sure if it happened in master, maybe the answer is yes, so you could forget this behavior)

----

1) Search "Real Madrid" in the ev.me field
2) Pull down in order to discover the wallpaper feature
3) Click on "wallpaper" button on the top right
4) Confirm that you want to change the wallpaper
5) Click on home button (you are in the "Real Madrid search")
6) Click home button again (you are in the landing page without searches)

At that moment, try to type something on the search field (nothing happens) and try to click on the "magnifying glass" icon, the "Set as wallpaper" confirm appears again.

Thanks Jan for you excellent work

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Hi Christian, thanks for the feedback.

1. Fixed (https://github.com/comoyo/gaia/commit/f7d24d02ed56931069b90795cd578d42ad03d4c9)
2. Is the current behavior on master as well, but yeah it's all very janky there. I filed bug 985911 with some investigations, but couldn't solve it in short time.
3. Wow, nice find, I never even heard about the wallpaper feature. Fixed (https://github.com/comoyo/gaia/commit/8d1d9b2756c14c47357489d3c7d876d82bcca177)

Branch has been updated.

(FYI, just putting f- on the issue is easier for me, as it will stand out in my bugmail).

Comment 4

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8393632 [details] [review]
Patch

In general LGTM although all changes have been performed in ev.me side and Ran is the best person to check them. I think that there are some changes from display to visibility properties and ,of course, you are avoiding reflows but almost sure you are not freeing up memory (i think that it was the root reason when it was done by ev.me team)

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

FYI, here is memory profile I ran of this branch vs. master:

Path = Homescreen -> Focus on I'm thinking of -> Home button (Idle) -> Search for 'afc ajax' -> Home button (Idle) -> Open music Collection -> Home button (Idle)

branch          state       USS       PSS
bug984414       Idle        12.2      16.5
bug984414       Focus       16.3      20.6
bug984414       Idle        16.3      20.7
bug984414       Search      24.8      29.3
bug984414       Idle        24.6      29.0
bug984414       Collection  25.0      29.5
bug984414       Idle        24.6      29.1

branch
master          Idle        12.2      16.6
master          Focus       17.1      21.5
master          Idle        17.1      21.5
master          Search      20.0      24.5
master          Idle        19.9      24.4
master          Collection  29.0      33.5
master          Idle        29.3      33.8

Comment 6

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8393632 [details] [review]
Patch

Quite the refactor. Kudos on the guts.

Currently though the BgImage "fullscreen" mode doesn't work. I commented on Github.

Comment 7

[Jan Jongboom [:janjongboom] (inactive)]

Hi Ran, can you take a look at GH?

Comment 8

[Ran Ben Aharon [:ranbena] EverythingMe]

Yep, replied in GH

Comment 9

[Ran Ben Aharon [:ranbena] EverythingMe]

Amir can you join us in reviewing this?

Comment 10

[Amir Nissim (:amirn)]

Great work Jan!

Code looks good, added small comments on github.
Can you please |rebase master| so we can test how this affects the changes made in bug 989731? (you will have a small conflict in CloudApps.js: https://github.com/EverythingMe/gaia/commit/7fcb57ecff687a64f937cc6aab3bdb8c6d6b06f2#diff-2)

Thanks :)

Comment 11

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8393632 [details] [review]
Patch

See GH

Comment 12

[Ran Ben Aharon [:ranbena] EverythingMe]

Comment on attachment 8393632 [details] [review]
Patch

Assigning Amir as the reviewer

Comment 13

[Amir Nissim (:amirn)]

Jan, can you please rebase your branch on master?

I am currently not able to test it since it is missing the patch for bug 992644:
https://github.com/EverythingMe/gaia/commit/6d618e27dd5da4f8e2b0a69cd2ccf63a4396605d

Thanks.

Comment 14

[Amir Nissim (:amirn)]

Comment on attachment 8393632 [details] [review]
Patch

clearing r? until PR is rebased

Comment 15

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8393632 [details] [review]
Patch

Rebased, and made sure the visuals match master.

Comment 16

[Jan Jongboom [:janjongboom] (inactive)]

Please note that market apps miss 2 pixels on the top due to bug 1000046 (also on master).

Comment 17

[Amir Nissim (:amirn)]

Comment on attachment 8393632 [details] [review]
Patch

Sorry for taking so long. This is a big patch :)

1. I have some questions on Github:
https://github.com/mozilla-b2g/gaia/pull/17324/files#r12084361
https://github.com/mozilla-b2g/gaia/pull/17324/files#r12047639
https://github.com/mozilla-b2g/gaia/pull/17324/files#r12087958
https://github.com/mozilla-b2g/gaia/pull/17324/files#r12090734

2. Noticed 2 regressions:
2.1 ordering app inside collection acts weird, see:
https://www.youtube.com/watch?v=MJk41R3ImnU

2.2 when opening a collection the grid icons and the collection bg are visible before the collection is in view. video compare with master:
https://www.youtube.com/watch?v=3IZHu4_gd_U

3. Please add these 2 commits to the patch:
https://github.com/EverythingMe/gaia/commits/comoyo-bug984414-reflow

4. f? Vivien regarding FPS and memory usage affected by this patch (off-viewport elements and dropping display:block)


Thanks!

Comment 18

[Amir Nissim (:amirn)]

Comment on attachment 8393632 [details] [review]
Patch

clearing until issues fixed.

Comment 19

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

Comment on attachment 8393632 [details] [review]
Patch

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #5)
> FYI, here is memory profile I ran of this branch vs. master:
> 
> Path = Homescreen -> Focus on I'm thinking of -> Home button (Idle) ->
> Search for 'afc ajax' -> Home button (Idle) -> Open music Collection -> Home
> button (Idle)
> 
> branch          state       USS       PSS
> bug984414       Idle        12.2      16.5
> bug984414       Focus       16.3      20.6
> bug984414       Idle        16.3      20.7
> bug984414       Search      24.8      29.3
> bug984414       Idle        24.6      29.0
> bug984414       Collection  25.0      29.5
> bug984414       Idle        24.6      29.1
> 
> branch
> master          Idle        12.2      16.6
> master          Focus       17.1      21.5
> master          Idle        17.1      21.5
> master          Search      20.0      24.5
> master          Idle        19.9      24.4
> master          Collection  29.0      33.5
> master          Idle        29.3      33.8

The PSS numbers are quite odd here. I would not have expected changes to the homescreen app to affects PSS. Do you know why it has happens ?

Also it seems like we still consume 4 extra megabytes when the Search is opened (both in non-idle/idle), do you know why ?

Comment 20

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #19)
> Comment on attachment 8393632 [details] [review]
> Patch
> 
> The PSS numbers are quite odd here. I would not have expected changes to the
> homescreen app to affects PSS. Do you know why it has happens ?
Nope, I'm not that good in memory consumption to be honest. What is normally the case for this? Could be because we keep some state in DOM instead of destroying and re-creating elements.

> Also it seems like we still consume 4 extra megabytes when the Search is
> opened (both in non-idle/idle), do you know why ?
I guess this comes because we now load the collection stuff here as well, you see the spike on master when opening the collection (+10 megs) that we don't see on this branch. I don't think it's intentional though, but we end up consuming less memory than on master in the end...

Comment 21

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

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #20)
> (In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails,
> needinfo? please) from comment #19)
> > Comment on attachment 8393632 [details] [review]
> > Patch
> > 
> > The PSS numbers are quite odd here. I would not have expected changes to the
> > homescreen app to affects PSS. Do you know why it has happens ?
> Nope, I'm not that good in memory consumption to be honest. What is normally
> the case for this? Could be because we keep some state in DOM instead of
> destroying and re-creating elements.
> 

PSS is normally the amount of memory
Do you mind getting a memory print again ? The PSS changes really does not makes sense to me. Any local changes in the app should not changes that too much afaik.

> > Also it seems like we still consume 4 extra megabytes when the Search is
> > opened (both in non-idle/idle), do you know why ?
> I guess this comes because we now load the collection stuff here as well,
> you see the spike on master when opening the collection (+10 megs) that we
> don't see on this branch. I don't think it's intentional though, but we end
> up consuming less memory than on master in the end...

Maybe this won't happen anymore with the new homescreen that should open a separate app for the search ?
Kevin, the new homescreen does all the search / collection stuffs into a separate process right ?

Comment 22

[Kevin Grandon :kgrandon]

(In reply to Vivien Nicolas (:vingtetun) (:21) (NOT reading bugmails, needinfo? please) from comment #21)
> Maybe this won't happen anymore with the new homescreen that should open a
> separate app for the search ?
> Kevin, the new homescreen does all the search / collection stuffs into a
> separate process right ?

Yup, for the vertical homescreen both search and collections are in separate processes. Collections specifically are handled as activities. It's possible that we might want some of these optimizations if they are applicable for the vertical homescreen in case we do the same type of activity chaining for tarako-level devices.

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

Closing because we have vertical homescreen now