Closed Bug 993952 Opened 6 years ago Closed 2 years ago
Big async reflow when toggling between two layouts
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.
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
will we get obvious performance gains from the user point of view? thanks
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.
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.
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 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?
hi Fabrice, the triage team tend to minus this one. can you provide more background on the blocking decision? Thanks
(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 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
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?
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
File a bug to re-enable the unit tests.
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?
triage: let's put this to backlog
blocking-b2g: 1.3T? → backlog
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...
But for bug 999353, the candidate panel for pinyin (Simplified Chinese) is always required, no matter whether auto-correction is enabled or not.
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 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.
(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
(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.
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
Firefox OS is not being worked on
Status: NEW → RESOLVED
Closed: 6 years ago → 2 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.