Skip to content

fix(web): disable fat-finger predictions for perf#5314

Closed
mcdurdin wants to merge 2 commits intomasterfrom
fix/web/5258-fat-finger-performance
Closed

fix(web): disable fat-finger predictions for perf#5314
mcdurdin wants to merge 2 commits intomasterfrom
fix/web/5258-fat-finger-performance

Conversation

@mcdurdin
Copy link
Copy Markdown
Member

@mcdurdin mcdurdin commented Jun 18, 2021 •
edited
Loading

Fixes #5258. And a little bit more.

The core change here is disabling fat finger predictions. The changes here improve perf from 380ms/keystroke to <10msec/keystroke.

The downside to this is obviously that fat-finger based predictions are no longer available. In my (limited) tests, I was not able to observe a visible reduction in prediction quality. In my opinion, reduction in prediction quality is outweighed by improvement in performance. (This is a shame and makes me sad, but I think we need to be willing to make significant decisions like this, even if we have invested a lot of effort into the code. See sunk cost fallacy.)

But this is not a complete solution. I have left the code in, commented out. If we decide to remove fat finger altogether, there is a lot of additional code we strip out. I think we need to do this, with a view to re-adding it later if we can bring performance up to scratch.

Three changes. Tested on Samsung Galaxy A90, 3800 character document in Keyman app.

Before: 380ms/keystroke.

  1. Implement sliding window for buildTransformFrom (not currently used, see point 3). But did improve performance by about 30msec, so thought I'd leave it in.

  2. Set CSS for the input backing textarea to display: none, as we never actually need to focus it. Improved performance further by 60msec by eliminating a Layout recalc in the webview.

  3. It was clear after making those changes, that the vast majority of the calc time left was in fat finger calculations. Disabled fat fingering . Improved performance further by 280msec (310msec total incl. point 1).

After: 10ms/keystroke.

@mcdurdin mcdurdin added this to the A15S7 milestone Jun 18, 2021
@mcdurdin mcdurdin added the fix label Jun 18, 2021
@mcdurdin mcdurdin marked this pull request as draft June 21, 2021 02:54
@jahorton jahorton mentioned this pull request Jun 21, 2021
Merged
@jahorton
Copy link
Copy Markdown
Contributor

jahorton commented Jun 21, 2021 •
edited
Loading

  1. Implement sliding window for buildTransformFrom (not currently used, see point 3). But did improve performance by about 30msec, so thought I'd leave it in.

Actually, it's still used, even then.

keyman/common/core/web/input-processor/src/text/inputProcessor.ts

Lines 157 to 159 in 349e144

// Now that we've done all the keystroke processing needed, ensure any extra effects triggered
// by the actual keystroke occur.
ruleBehavior.finalize(this.keyboardProcessor, outputTarget);

keyman/web/source/dom/domOverrides.ts

Lines 19 to 41 in 349e144

let headlessRuleBehaviorFinalize = text.RuleBehavior.prototype.finalize;
text.RuleBehavior.prototype.finalize = function(this: text.RuleBehavior, processor: text.KeyboardProcessor, outputTarget: text.OutputTarget) {
// Execute the standard baseline stuff first.
headlessRuleBehaviorFinalize.call(this, processor);
// If the transform isn't empty, we've changed text - which should produce a 'changed' event in the DOM.
let ruleTransform = this.transcription.transform;
if(ruleTransform.insert != "" || ruleTransform.deleteLeft > 0 || ruleTransform.deleteRight > 0) {
if(outputTarget instanceof targets.OutputTarget && outputTarget.getElement() == dom.DOMEventHandlers.states.activeElement) {
dom.DOMEventHandlers.states.changed = true;
}
}
let keyman = com.keyman.singleton;
// KMEA and KMEI (embedded mode) use direct insertion of the character string
if(keyman.isEmbedded) {
// A special embedded callback used to setup direct callbacks to app-native code.
keyman['oninserttext'](ruleTransform.deleteLeft, ruleTransform.insert, ruleTransform.deleteRight);
if(outputTarget instanceof targets.OutputTarget) {
keyman.refreshElementContent(outputTarget.getElement());
}
}
}

It's literally at the core of how embedded operates.

@jahorton
Copy link
Copy Markdown
Contributor

jahorton commented Jun 25, 2021 •
edited
Loading

So... now that #5319's been merged in, I think points 1 and 3 are now moot. Point 2 (embedded display: none) sounds pretty useful though, so that's likely worth preserving, even if it'll need a bit more vetting first.

I suppose point 1 can see a bit of effect, though #5319's context windowing will have preapplied this for the lion's share of cases already. The only aspect not covered by at PR's context windowing is for the results of the base keystroke.

@mcdurdin
Copy link
Copy Markdown
Member Author

mcdurdin commented Jun 25, 2021

Point 2 (embedded display: none) sounds pretty useful though, so that's likely worth preserving, even if it'll need a bit more vetting first.

I've been continuing to test this with no side effects observed on Android. Have not tested on iOS. I'll be trying to setup a pair of PRs to tidy this up today, so we can close this off and get the release out.

@mcdurdin
Copy link
Copy Markdown
Member Author

mcdurdin commented Jun 25, 2021

I'll be trying to setup a pair of PRs to tidy this up today, so we can close this off and get the release out.

I've been diverted onto other fires so will not get to this until next week now.

@mcdurdin
Copy link
Copy Markdown
Member Author

mcdurdin commented Jun 27, 2021

Replaced with #5319, #5376

@mcdurdin mcdurdin closed this Jun 27, 2021
@mcdurdin mcdurdin deleted the fix/web/5258-fat-finger-performance branch June 27, 2021 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

android/engine/ android/ common/web/ common/ fix web/

Projects

None yet

Milestone

A15S7

Development

Successfully merging this pull request may close these issues.

refactor(web): context operations should use a sliding window

2 participants

@mcdurdin @jahorton