keymanapp / keyman

Keyman cross platform input methods system running on Android, iOS, Linux, macOS, Windows and mobile and desktop web
https://keyman.com/
Other
372 stars 102 forks source link

change(web): OSK optimization, improved responsiveness πŸͺ  #11140

Closed jahorton closed 1 month ago

jahorton commented 1 month ago

The set of PRs culminating here heavily restructures how the OSK loads and updates during use - especially after device rotation and when swapping layers. This should help the OSK load faster and change layers faster.

Based on local profiling experiments, observations, and data, with this PR + #11129... (details in a comment below)

The technical catch-all for the underlying issue is known as "layout reflow thrashing". (This is the focus of #11177.)

While #11177 optimizes a lot of the lower layout-reflow related code to avoid layout reflow thrashing, this PR is concerned with the high-level version of it: our engine, as currently written, sometimes includes redundant refreshLayout calls on the OSKView and VisualKeyboard level. As both currently use getComputedStyle and change DOM / styling data, it's quite critical - and impactful - to functionally eliminate redundant calls to their refreshLayout methods.

Related PRs

Since triggering layout reflows is expensive, #11177 reworked the OSK to use precalculated values as often as possible while batching layout changes together, allowing us to skip many of the reflow operations that were previously hindering responsiveness. There was also some layout reflow during the key highlighting to key preview generation process; similar changes were made there as well, which may help improve "average" keystroke responsiveness as well.

Fortunately for us, we've already been leveraging precalculated values for certain parts of functionality for a while. We can leverage those same values to avoid excessive use of getComputedStyle by just recomputing it directly, given our knowledge of the tendencies of our own codebase.

On an older Android device (6.0.1, but using Chrome 80+) which has been noted to be significantly affected by OSK performance, these changes significantly cut the time needed for the OSK to swap active layers, often by a factor of two or so. The delay is still perceptible, with the first swap to each previously-unshown layer the most pronounced. Swaps back to already-shown layers are consistently improved.

User Testing

TEST_WINDOWS_DESKTOP: Test general use of KeymanWeb on a Windows desktop or laptop. Report any unexpected behavior.

TEST_ANDROID_TABLET: Test general use of KeymanWeb on an Android tablet. Report any unexpected behavior.

TEST_IOS_PHONE: Test general use of KeymanWeb on an iPhone. Report any unexpected behavior.

TEST_ANDROID_PHONE_RAPID_TYPING: Try to repro #10592. Include occasional layer changes during your typing bursts. Report any unexpected behavior.

TEST_ANDROID_SPACEBAR_RECENT: Using Keyman for Android version 12 or greater, verify that the spacebar is always appropriately captioned (based on user settings in the menu) for the active keyboard, with the text properly sized.

keymanapp-test-bot[bot] commented 1 month ago

User Test Results

Test specification and instructions

jahorton commented 1 month ago

So, what's the performance for loading a keyboard look like presently?

image

The green box is the part that this helps optimize.

What about when we first swap to a new layer?

image

The big "recalculate style" is needed to ensure that each key receives proper styling. (The layer has become visible [display: block] for the first time, a prerequisite to getting valid style information.) After all, it is quite possible for keyboard authors (see: sil_cameroon_azerty, among other keyboards) to set key-specific CSS stylesheet styling... and we can only get that reliably when each element fully enters the DOM rendering pipeline. Note that once the one style-recalculation pass is complete... no more reflow-related segments show up, with the layout-recalculation segment passing pretty quickly otherwise.

jahorton commented 1 month ago

After some additional work, keyboard loading has been streamlined a fair bit.

Using the same old Android test device...

image

What you see above is the full profile for loading the sil_euro_latin keyboard with an empty context. Note that quite a sizable portion of it is spent simply preprocessing the layout data! Upon inspection, a fair bit of time is spent on property enumeration and initialization. It's slightly longer than is actually spent building the OSK's element hierarchy!

image

Once the OSK layout data is fully preprocessed, there's then the matter of resetting context, determining the correct initial layer, and then displaying it:

image

Keyboards are also set automatically to default as the base layer; changing to shift automatically would incur an extra refreshLayout(). Previously, that highlighted setLayerId call would set off the second round, which would be re-run at the end.

Once it's time to actually do the layout refresh...

image

The reason for the early reflow seen in that image:

image

We do want that one instance where we can get the true, exact dimensions of the keyboard used for all the other calculations. There are certain pre-calculations on the 'key' level that require being in the DOM as well, and those also require referencing computed styles. It has to occur at some point, and we need those values in order to properly batch all the lower-level layout operations. (By not changing any value after this until the full change-batch is ready, those key-level getComputedValue calls don't trigger additional reflows.)

Shortly after that whole process completes, there's some optimizable overhead happening with Web engine & Android app synchronization:

image

image

I am consistently seeing a double updateKMText (which, if text is empty, triggers a context reset) followed by an explicit context reset on every keyboard load. That's not fixed here, but what is fixed is that each of the three reset calls now only triggers layout once, rather than twice.

And no, I don't know why fullContextMatch -> any is triggering a style-recalculation. Neither function should even be touching the DOM... maybe the WebView decided to force a rendering update then?

jahorton commented 1 month ago

Also of note: I have done nothing to mitigate or prevent extra reflows from occurring due to predictive-text banner functionality - and it is triggering some of the reflow seen in the images above - especially within the 3x context reset section.

Not shown - the layout refresh once the stylesheets finish loading. It's not a keyboard-loading bottleneck, at least, and is pretty well optimized now.

jahorton commented 1 month ago

With regard to the predictive-text banner:

image

Note that there is a configureForKeyboard function that may be used to reset things upon a keyboard change in addition to a more-standard refreshLayout() that can handle device rotations, etc. We should be able to precompute the font data for standard keystrokes in order to mitigate layout reflow here.

Fortunately, this is the one spot where the predictive-text banner actually polls any style information on a per-keystroke basis. Even then... we could just "store" the changes in a closure and execute them at the same time as we make changes in the main OSK's layout.

Note the highlighted style-recalculation section here, toward the bottom-left:

image


Another offender looks to be the banner's updateFade method, which doesn't double-check whether or not any style edits are truly needed:

https://github.com/keymanapp/keyman/blob/73b8ff2bb4c0c9217fab991dd5c6b00d10a0bc23/web/src/engine/osk/src/banner/suggestionBanner.ts#L203-L226

There's nearly always some reflow visible within the profiling associated with its portions on the timeline.

bharanidharanj commented 1 month ago

Test Results

bharanidharanj commented 1 month ago

Test Results

bharanidharanj commented 1 month ago

Test Results

bharanidharanj commented 1 month ago

Test Results

jahorton commented 1 month ago

Huh, another neat thing to note...

image

The act of fetching previously-untapped cookies takes a surprising amount of time, it seems. 74.3ms when loading sil_euro_latin for the first time in a host-page's lifetime this run, and it seems reasonably consistent. This is run synchronously during Web engine initialization once the keyboard is being loaded in.

jahorton commented 1 month ago

After running several profiles to collect approximate time data using the in-office SM-T350 test device (Android 6.0.1), I have the following details to report. To smooth things out a bit, note that each scenario + codebase combination had at least three separate profile runs backing its data.

Keyboard load times

Keyboards used: sil_euro_latin, sil_cameroon_azerty, khmer_angkor.

This data does not include the time spent waiting for the keyboard stylesheets to be applied. It only measures "time until usable."

With #11129 and #11140 in place:

Without them in place:

Conclusion:

App-keyboard initialization

How much time is required to activate Keyman Engine for Web within the Keyman app keyboard's host page in total? Start to usable-finish?

I happened to notice significant difference in the amount of time it took to reset for each profile between the old and new versions of the code, so I thought this number might prove interesting. And, it does!

To be clear, this was done locally with a Debug-configuration - and thus, unminified Web. The "base cost" number may vary with minification.

Default keyboard: sil_euro_latin.

  1. Base cost: Using the --debug version of KeymanWeb at the core, the base KeymanWeb script took approximately 1.0 to 1.1 seconds to load.

After that...

With #11129 and #11140 in place:

  1. Between 475 to 560 ms were taken to initialize Keyman Engine for Web (empty keyboard due to async script loading)
  2. Approximately 1 second was taken to load and activate sil_euro_latin, starting from when the keyboard's script first began to be processed.

For an approximate total average time, let's go with 2.6 seconds to initialize. (1.1 + 0.5 + 1)

Without them in place:

  1. Between 810 to 850 ms were taken to initialize Keyman Engine for Web (empty keyboard due to async script loading)
  2. Approximately 2.3 to 2.4 seconds were taken to load and activate sil_euro_latin, starting from when the keyboard's script first began to be processed.
    • I am not immediately sure why there's extra time compared to the base load.
    • I will note, though, that I tended to leave out small intermediate "tasks" unrelated to the main thing being measured in the previous section. It's quite possible that those add up significantly.

For an approximate total average time, let's go with 4.2 seconds to initialize. (1.1 + 0.8 + 2.3)

2.6 / 4.2 = 0.6190% 5.2 / 2.6 ~= 1.6154.

So.., the host-page now initializes about 60% faster / in about 62% of the time it used to take!

Repeated layer-swapping

This is the aspect most directly targeted by #11129, though #11140 also brings some improvements of its own to the table.

My goal with this test was to test the performance of cross-layer multitapping. Sadly, it seems that when the profiler is active, it somehow interferes with proper layer-changing multitap processing. That being said, the simple act of swapping layers repeatedly in close succession still yields some remarkable insights worth discussing.

The input sequence:

With #11129 and #11140 in place:

Without these two PRs in place:

Concluding Thoughts

Note that the many of the big penalties being eliminated in the previous section above would have only been introduced in 17.0-alpha, so it's not likely to be that big of a speedup from 16.0. The other sections, though? Fair game.

As such, I think we can safely claim a 33% speed keyboard-swapping speed increase, a 60% app-keyboard initialization-speed increase, and probably at least a 33%, if not 50%, layer-swapping speed increase relative to 16.0-stable. I understand if we'd rather get hard numbers from 16.0 rather than recent 17.0-beta code before making that claim, though.

jahorton commented 1 month ago

Here's one sample Chrome profile after these changes:

android host-page init profile.json

I would consider everything through 3563ms, and maybe the next "Task" afterward, part of host-page initialization. The "Task" from around 1550ms to 2043ms would be the KMW initializing with an empty keyboard for the OSK (followed by layout from around 2100ms to 2237ms), with the task starting at 2385ms or so being when the actual keyboard is first loaded. (The "Task" around 2243ms is handling function calls from the Android app.)

Obviously, spending that much time to load an empty keyboard, just to turn around and load a proper one, isn't ideal. That might be something worth looking at further. This PR does at least make both keyboard loads notably faster, but what'd be even faster is to build the OSK once (for the right keyboard) instead of twice (once for a stand-in default, then once for the right keyboard).

A zoomed-over overview of the main timeline for initialization as seen in the profile:

zoomed-out profile timeline

mcdurdin commented 1 month ago

After all, it is quite possible for keyboard authors (see: sil_cameroon_azerty, among other keyboards) to set key-specific CSS stylesheet styling... and we can only get that reliably when each element fully enters the DOM rendering pipeline.

While per-keyboard CSS is certainly available, layout is intended to be 100% the responsibility of Keyman Engine for Web. Keyboard authors should not be using CSS to change any layout dimensions, and results are undefined if they do so. I know that this is at best only implied in the limited documentation on keyboard CSS, but my opinion is we should improve the documentation to clarify that rather than allow keyboard authors to move or scale elements in the keyboard CSS -- that way lies madness.

Given that, we never need to rely on the renderer in order to calculate dimensions. We should know all the key squares at load time.

mcdurdin commented 1 month ago

Disappointed that this change increases the file size by 1.5kb 😒 I was hoping that some simplification would have been the result!

mcdurdin commented 1 month ago

It's slightly longer than is actually spent building the OSK's element hierarchy!

I can't easily quote the relevant lines from the image. Images are a pain. I also don't have enough context to really know what I am looking at -- how many times are each of those lines called, for example?

That whole polyfill() function feels like it could be rewritten to work a whole lot faster, as we already know the structure, so we shouldn't ever have to iterate over properties. But I don't really want to shave that yak right now.

jahorton commented 1 month ago

After all, it is quite possible for keyboard authors (see: sil_cameroon_azerty, among other keyboards) to set key-specific CSS stylesheet styling... and we can only get that reliably when each element fully enters the DOM rendering pipeline.

While per-keyboard CSS is certainly available, layout is intended to be 100% the responsibility of Keyman Engine for Web. Keyboard authors should not be using CSS to change any layout dimensions, and results are undefined if they do so. I know that this is at best only implied in the limited documentation on keyboard CSS, but my opinion is we should improve the documentation to clarify that rather than allow keyboard authors to move or scale elements in the keyboard CSS -- that way lies madness.

Given that, we never need to rely on the renderer in order to calculate dimensions. We should know all the key squares at load time.

Agreed. That said, font-size calculations certainly depend on font-face and font-size styling - styling that I don't think we prohibit, even if we do allow ways to specify much of that in the touch-layout. Any stylesheet-based tweaks on a per-key basis could affect the internal layout of keyboard keys - the scaling of key caps, in particular. I don't think sil_cameroon_azerty actually gives us much trouble here, but some of its stylesheet rules get really selective:

/* Colored vowels on touch */
 .kmw-keyboard-sil_cameroon_azerty:not(.desktop) .kmw-key:-webkit-any(
     [id$='K_Q'],
     [id$='K_E'],
     [id$='K_I'],
     [id$='K_U'],
     [id$='K_O'],
     [id$='rightalt-K_P'],
     [id$='rightalt-K_T'],
     [id$='rightalt-K_F'],
     [id$='rightalt-K_H'],
     [id$='rightalt-caps-K_P'],
     [id$='rightalt-caps-K_T'],
     [id$='rightalt-caps-K_F'],
     [id$='rightalt-caps-K_H'],
     [id$='rightalt-shift-K_P'],
     [id$='rightalt-shift-K_T'],
     [id$='rightalt-shift-K_F'],
     [id$='rightalt-shift-K_H']
 ):not(.kmw-key-touched),
 .kmw-keyboard-sil_cameroon_azerty:not(.desktop) ~ #kmw-popup-keys .kmw-key:-webkit-any( /* Working for now: https://github.com/keymanapp/keyman/issues/9864 */
     [id*='U_025B'], 
     [id*='U_0259'],
     [id*='U_0258'],
     [id*='U_0153'],
     [id*='U_0289'],
     [id*='U_0268'],
     [id*='U_0254'],
     [id*='U_00F8'],
     [id*='U_03B1'],
     [id*='U_00E6'],
     [id*='U_018F'],
     [id*='U_0152'],
     [id*='U_0244'],
     [id*='U_0197'],
     [id*='U_0186'],
     [id*='U_00D8'],
     [id*='U_0190'],
     [id*='U_00C6'],
     [id*='U_2C6D']
 ):not(.kmw-key-touched),
 .kmw-keyboard-sil_cameroon_azerty:not(.desktop) #kmw-popup-keys .kmw-key:-webkit-any( /* Future: https://github.com/keymanapp/keyman/issues/9864*/
     [id*='U_025B'], 
     [id*='U_0259'],
     [id*='U_0258'],
     [id*='U_0153'],
     [id*='U_0289'],
     [id*='U_0268'],
     [id*='U_0254'],
     [id*='U_00F8'],
     [id*='U_03B1'],
     [id*='U_00E6'],
     [id*='U_018F'],
     [id*='U_0152'],
     [id*='U_0244'],
     [id*='U_0197'],
     [id*='U_0186'],
     [id*='U_00D8'],
     [id*='U_0190'],
     [id*='U_00C6'],
     [id*='U_2C6D']
 ):not(.kmw-key-touched) {
     background: var(--vowel-fade); 
     background-image: var(--vowel-grad); 
 }

If there were even a hit of font-size adjustment - say, font-size: 1.1em - our key-cap scaling needs to know in order to match 16.0 behavior.

A screenshot of sil_cameroon_azerty's default layer, with vowels colored by that CSS:

image

jahorton commented 1 month ago

That whole polyfill() function feels like it could be rewritten to work a whole lot faster, as we already know the structure, so we shouldn't ever have to iterate over properties. But I don't really want to shave that yak right now.

Yeah, I hear you - that's why I highlighted its impact. It's tracked as part of #11166 for now.

mcdurdin commented 1 month ago

If there were even a hit of font-size adjustment - say, font-size: 1.1em - our key-cap scaling needs to know in order to match 16.0 behavior.

Why is that needed for calculating key squares? For presentation, sure. But the calculation of gestures should never change based on CSS

mcdurdin commented 1 month ago

I started reviewing this, but I think we need to split it into separate, smaller PRs because there are a heck of a lot of entangled changes.

jahorton commented 1 month ago

If there were even a hit of font-size adjustment - say, font-size: 1.1em - our key-cap scaling needs to know in order to match 16.0 behavior.

Why is that needed for calculating key squares? For presentation, sure. But the calculation of gestures should never change based on CSS

Uh... by "key cap", I mean the text label we put on keys, not the key square. It affects the text that goes inside the key square.

jahorton commented 1 month ago

I started reviewing this, but I think we need to split it into separate, smaller PRs because there are a heck of a lot of entangled changes.

Should I make totally new PRs, or put this in draft, spin part off and do a rebase afterward to clean things up?

mcdurdin commented 1 month ago

Should I make totally new PRs, or put this in draft, spin part off and do a rebase afterward to clean things up?

I don't mind, whichever is less troublesome for you. Appreciate you going the extra mile to break this down πŸ˜ƒ.

jahorton commented 1 month ago

This PR has now been rebased in order to split what was 1 PR into four separate PRs, with this one as the final one in the sequence. Prior comments will generally include references to contents now found in #11175, #11176, and #11177 as well as this PR.

jahorton commented 1 month ago

After double-checking the transition of key-cap scaling to use of precalculated values, I realized that I hadn't set the key-height parameter correctly. That's been fixed now in #11177, which I then merged into this one. I've also added a new user test related to it here.

bharanidharanj commented 1 month ago

Test Results

keyman-server commented 1 month ago

Changes in this pull request will be available for download in Keyman version 17.0.307-beta

sentry-io[bot] commented 1 month ago

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

Did you find this useful? React with a πŸ‘ or πŸ‘Ž