Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): optimization via lazy construction of OSK layers ⏩ #11264

Merged
merged 3 commits into from
Jun 21, 2024

Conversation

jahorton
Copy link
Contributor

Continues from #11263, addressing part of #11166.

One particularly easy way to load and display the OSK more quickly is to simply... not make the parts you don't need yet. We can do just-in-time (JIT) construction of OSK layers when they're needed.

While the default layer is pretty much always needed, and some keyboards will sometimes start in one or two other layers... a large number of keyboards have extra layers that can wait.

As there's one more followup on this set of PRs...

@keymanapp-test-bot skip

@jahorton
Copy link
Contributor Author

Current build-failure cause:

15:26:42   renderer_core.ts(181,54): error TS2341: Property 'layers' is private and only accessible within class 'OSKLayerGroup'.

Right, some of the tools will want access to that.

@jahorton jahorton force-pushed the feat/web/osk-layer-deferral branch from b231b4d to 44c178f Compare April 23, 2024 03:32
@jahorton jahorton force-pushed the change/web/keyboard-preprocess-optimization branch from c90e6a3 to 1fccce5 Compare April 23, 2024 03:32
@darcywong00 darcywong00 modified the milestones: B17S6, A18S1 Apr 28, 2024
@darcywong00 darcywong00 modified the milestones: A18S1, A18S2 May 11, 2024
@mcdurdin mcdurdin modified the milestones: A18S2, A18S3 May 24, 2024
@mcdurdin mcdurdin modified the milestones: A18S3, A18S4 Jun 7, 2024
@jahorton jahorton marked this pull request as ready for review June 19, 2024 08:54
@jahorton jahorton requested a review from ermshiperete as a code owner June 19, 2024 08:54
if(!layer) {
layer = this.buildLayer(id);
if(layer) {
this._layers[id] = layer;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line seems unnecessary since that's already done in buildLayer.

@jahorton
Copy link
Contributor Author

jahorton commented Jun 21, 2024

Build failure seen for "Test: Language Modeling Layer (Common)":


15:56:57 
  LMLayer using the trie model

15:56:57 
    Prediction

15:56:57 
    Regression tests

15:56:57 
      should use the default searchTermToKey()

15:56:58 
        expected +0 to be at least 1

15:56:58 
        AssertionError: expected +0 to be at least 1
            at file:///C:/BuildAgent/work/99b311828f4ee7c/keyman/common/predictive-text/unit_tests/headless/worker-trie-integration.js:85:16

The worker's "test mode" is on, so we shouldn't be exiting early... but perhaps recent changes from #11784 and its predecessors have had an unintended consequence here.

Base automatically changed from change/web/keyboard-preprocess-optimization to master June 21, 2024 04:22
@jahorton jahorton merged commit 56ae8a8 into master Jun 21, 2024
14 of 15 checks passed
@jahorton jahorton deleted the feat/web/osk-layer-deferral branch June 21, 2024 04:23
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 18.0.60-alpha

Copy link
Member

@mcdurdin mcdurdin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any stats on improvement in performance?

let oskHeight = 0;

// In case the keyboard's layers have differing row counts, we check them all for the maximum needed oskHeight.
for (const layerID in layers) {
const layer = layers[layerID];
let nRows = layer.rows.length;
let nRows = layer.row.length;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a bit to find it, but the reason is on line 1319. It shifts from using the set of pre-built OSK layers (which have a .rows property) to using the underlying touch-layout specification (which uses .row, not .rows) as the basis for our calculations here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. A bit squidgy, but make sense!

@jahorton
Copy link
Contributor Author

jahorton commented Jul 2, 2024

Do you have any stats on improvement in performance?

Not in isolation. I did collect stats for the set of three it was a part of though, and that can be found on #11265. Note the last bit before the "User Testing" section in particular - that's the group stats.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants