993952 - Big async reflow when toggling between two layouts

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Keyboard, defect, P1)

Description

[Jan Jongboom [:janjongboom] (inactive)]

On Tarako switching between keyboards is very expensive even when there are no sync reflows anymore. This is because we do a display: block / none on the keyboards to toggle visibility. This causes the element to completely reflow when toggling between layouts. On Tarako this costs a lot of time:

reflow: 33.79ms
reflow: 50.19ms
reflow: 34.95ms
reflow: 32.57ms
reflow: 36.15ms
reflow: 70.16ms

Need to verify if changing this won't increase memory footprint.

Comment 1

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch

Memory numbers, I don't think there's an actual difference in it (although I don't get the browser process back on this patch for some reason):

With this patch

                          |     megabytes     |
           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER
            b2g 3999    1   40.1    0 36.9 40.4 47.3 140.3       0 root
         (Nuwa) 4054 3999    0.9    0  0.6  2.5  7.2  50.7       0 root
     Homescreen 4069 4054    3.0   18  6.8 10.5 17.9  65.8       8 app_4069
(Preallocated a 4134 4054    0.9   18  5.7  8.2 14.3  58.7       1 root

On upstream/v1.3t

                          |     megabytes     |
           NAME  PID PPID CPU(s) NICE  USS  PSS  RSS VSIZE OOM_ADJ USER
            b2g 3764    1   33.1    0 36.8 40.0 47.6 141.7       0 root
         (Nuwa) 3818 3764    0.9    0  0.1  2.0  7.6  50.7       0 root
     Homescreen 3833 3818    3.0   18  5.1  8.4 17.0  65.8       8 app_3833
        Browser 3882 3818    1.0   18  4.4  6.6 14.1  59.7      10 app_3882
(Preallocated a 3904 3818    0.8   18  5.1  7.2 14.5  59.7       1 root

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

will we get obvious performance gains from the user point of view? thanks

Comment 3

[Jan Jongboom [:janjongboom] (inactive)]

Currently the thread is blocked when switching layouts for 30-70 ms. in which the keyboard will not respond to anything. If the user will notice that, I think so. Think we need to see it in combination with the other reflows in keyboard.

Comment 4

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8403876 [details] [review]
Patch

Jan,

Really strange, not sure if you would see this in your tests, but after applying this patch, the keyboard could not show up.
This would be easy to repro for the first time you trigger the keyboard after flashing the build (or reboot).

I'll clear the review for now.

Comment 5

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8403876 [details] [review]
Patch

Not completely surprising, the ui-tests also failed because of this. I tested it together with https://github.com/mozilla-b2g/gaia/pull/18076. I updated the PR with a patch that fixes the issue (bug 993367 not required):

https://github.com/comoyo/gaia/commit/33d50a2034fc1f6b9f0bd2e8ad34474c61385a0e

Comment 6

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8403876 [details] [review]
Patch

Sorry for the delay.
However, I probably found a regression when trying this patch out.

STR
===
1 launch keyboard.
2. Press "?123" to switch to number layout and press "ABC" to switch back.
3. => You would find a dark border at the bottom of the keyboard.

Do we need the patch in bug 993367 to get over this issue?

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

hi Fabrice, the triage team tend to minus this one. can you provide more background on the blocking decision? Thanks

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Joe Cheng [:jcheng] from comment #7)
> hi Fabrice, the triage team tend to minus this one. can you provide more
> background on the blocking decision? Thanks

That looked an easy win 5 days ago. it looks more risky now with the issues that Rudy found. Feel free to unblock.

Comment 9

[Jan Jongboom [:janjongboom] (inactive)]

Comment on attachment 8403876 [details] [review]
Patch

Hmm, weird thing. I saw it only on fields with autocorrect, which I didn't have on Tarako.

Fix: https://github.com/comoyo/gaia/commit/6ef421b554faf5c0e9265a4f4b9e0fa1391371dd

Comment 10

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I'm reviewing and testing this patch again.
However, the travis run is failed, will look into to see what happened.

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8403876 [details] [review]
Patch

This change looks good to me, but the travis CI failed due to marionette python test.
I think this is because right now #keyboard element is with height=0, so it won't be considered isDisplayed.

Will update the test code for this to land.

Comment 12

[Jan Jongboom [:janjongboom] (inactive)]

Are you updating the test code for this or should I?

Comment 13

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

I was looking at the unit test code around dimension and found it may have some problems to calculate the correct height of activeIme after we add the CSS styles.
(Besides, it seems those test code does not run all the combinations of different row numbers, but maybe we should look at the current issue first.)

I'll prefer to have an alternative patch by using visibility: hidden on each keyboard panel to see if this could get unit testing passed.
I'll try to implement this if you don't beat me to that.

Comment 14

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Land this first with the unit tests disabled,
https://github.com/mozilla-b2g/gaia/commit/2aa585a17b4df8ee2ae279b4ee80ff3b1ae9ab9f

Comment 15

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

File a bug to re-enable the unit tests.

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Just back this out with the following commit due to regression bug 999353,
https://github.com/mozilla-b2g/gaia/commit/aad7a8dc103fc245c18edf030f082f73bf2e90a9

Comment 17

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

As comment 8 and a recent regression caused by this change, I would suggest re-considering setting this as a blocker.

Joe, what do you think?

Comment 18

[Joe Cheng [:jcheng] (please needinfo)]

triage: let's put this to backlog

Comment 19

[Jan Jongboom [:janjongboom] (inactive)]

The candidate panel is not present on Tarako anyway right? Which is a bit weird to me, because we didn't want autocorrect panel to be there, but now we removed that as well...

Comment 20

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

But for bug 999353, the candidate panel for pinyin (Simplified Chinese) is always required, no matter whether auto-correction is enabled or not.

Comment 21

[Jan Jongboom [:janjongboom] (inactive)]

Attachment: Patch v2

I think I tested only against upstream/v1.3t. This is tested against master, and has as far as I can see also working candidate panel in Chinese.

Comment 22

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8413651 [details] [review]
Patch v2

Jan,

There are 2 issues that I saw with this patch,

  1. The keyboard height is a little bit different after this patch, it would have 6 pixels less so that the app window would be resized incorrectly.

  2. The dependent issue, bug 999353, still could be reproduced.

Could you please help take a look?
Thanks.

Comment 23

[Jan Jongboom [:janjongboom] (inactive)]

(In reply to Rudy Lu [:rudyl] from comment #22)
> Comment on attachment 8413651 [details] [review]
> Patch v2
> 
> Jan,
> 
> There are 2 issues that I saw with this patch,
> 
>   1. The keyboard height is a little bit different after this patch, it
> would have 6 pixels less so that the app window would be resized incorrectly.
> 
>   2. The dependent issue, bug 999353, still could be reproduced.
> 
> Could you please help take a look?
> Thanks.

Which device are you running this on? Also can you please r- patches, then they stand out in my bugmail :p

Comment 24

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #23)

> Which device are you running this on? Also can you please r- patches, then
> they stand out in my bugmail :p

I tested this on buri.
For r-, sure, will do next time, :), if necessary.

Comment 25

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Jan, please unassign yourself if you no longer actively working on this.

Comment 26

[BMO Automation]

Firefox OS is not being worked on