Closed Bug 993952 Opened 6 years ago Closed 2 years ago

Big async reflow when toggling between two layouts

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(tracking-b2g:backlog, b2g-v1.3T affected, b2g-v1.4 affected, b2g-v2.0 fixed)

RESOLVED WONTFIX
tracking-b2g backlog
Tracking Status
b2g-v1.3T --- affected
b2g-v1.4 --- affected
b2g-v2.0 --- fixed

People

(Reporter: janjongboom, Unassigned)

References

Details

(Keywords: perf, Whiteboard: [c=effect p= s= u=tarako])

Attachments

(1 file, 1 obsolete file)

46 bytes, text/x-github-pull-request
Details | Review
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.
blocking-b2g: --- → 1.3T?
Attached file Patch (obsolete) —
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
Attachment #8403876 - Flags: review?(rlu)
will we get obvious performance gains from the user point of view? thanks
Flags: needinfo?(janjongboom)
Blocks: 994000
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.
Flags: needinfo?(janjongboom)
blocking-b2g: 1.3T? → 1.3T+
Keywords: perf
Whiteboard: [c=effect p= s= u=]
Status: NEW → ASSIGNED
OS: Mac OS X → Gonk (Firefox OS)
Priority: -- → P1
Hardware: x86 → ARM
Whiteboard: [c=effect p= s= u=] → [c=effect p= s= u=tarako]
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.
Attachment #8403876 - Flags: review?(rlu)
Flags: needinfo?(janjongboom)
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
Attachment #8403876 - Flags: review?(rlu)
Flags: needinfo?(janjongboom)
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?
Attachment #8403876 - Flags: review?(rlu)
hi Fabrice, the triage team tend to minus this one. can you provide more background on the blocking decision? Thanks
Flags: needinfo?(fabrice)
(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.
Flags: needinfo?(fabrice)
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
Attachment #8403876 - Flags: review?(rlu)
I'm reviewing and testing this patch again.
However, the travis run is failed, will look into to see what happened.
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.
Attachment #8403876 - Flags: review?(rlu) → review+
Are you updating the test code for this or should I?
Flags: needinfo?(rlu)
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.
Flags: needinfo?(rlu) → needinfo?(janjongboom)
Land this first with the unit tests disabled,
https://github.com/mozilla-b2g/gaia/commit/2aa585a17b4df8ee2ae279b4ee80ff3b1ae9ab9f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
File a bug to re-enable the unit tests.
Flags: needinfo?(janjongboom)
Just back this out with the following commit due to regression bug 999353,
https://github.com/mozilla-b2g/gaia/commit/aad7a8dc103fc245c18edf030f082f73bf2e90a9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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?
blocking-b2g: 1.3T+ → 1.3T?
Flags: needinfo?(jcheng)
triage: let's put this to backlog
blocking-b2g: 1.3T? → backlog
Flags: needinfo?(jcheng)
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...
Flags: needinfo?(rlu)
But for bug 999353, the candidate panel for pinyin (Simplified Chinese) is always required, no matter whether auto-correction is enabled or not.
Flags: needinfo?(rlu)
Attached file 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.
Attachment #8403876 - Attachment is obsolete: true
Attachment #8413651 - Flags: review?(rlu)
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.
Attachment #8413651 - Flags: review?(rlu)
(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
Flags: needinfo?(rlu)
(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.
Flags: needinfo?(rlu)
Jan, please unassign yourself if you no longer actively working on this.
Status: REOPENED → NEW
Summary: Big async reflow when toggling between two keyboards → Big async reflow when toggling between two layouts
Assignee: janjongboom → nobody
blocking-b2g: backlog → ---
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.