Decrease input-lag for large queries #158

OskarDamkjaer · 2023-12-21T11:22:14Z

Overview

This ended up being quite a bit larger than I had anticipated/hoped for, but the main changes are:

Use workerpool pkg to create a pool of semantic analysis workers in both codemirror & the server itself. It will cancel outdated and unfinished semanatic analsysis runs when new ones get triggered. I also debounced it for the language server so it fires when the user has stopped typing (chose 600ms somewhat arbitrarily, codemirror has a built in debounce of 750ms for comparison)
I detect slow parses in codemirror and switch to using prismjs for tokenization. Prism is a syntax highlighting library that has cypher support and can be used as a crude parser via it's tokenize method. In my testing it is about ~20x our parser.

A few future tasks:
[ ] Figure out a smarter way to bundle (see note on it below)
[ ] Reduce duplication of the worker code
[x] Improve react perf by debouncing the on change there as well

Outdated description below

Since the semantic analysis takes so much longer than the normal errors, I split it to it's own call.

I started by trying to get the same runSemanticAnalysis webworker implementation working everywhere with the web-worker node package, but it turned out to be really tricky to get it to work in all combinations of environment (jest/web/plain node) and targets (esm/cjs). I ended up leaving the function more or less as it was and then I do an esm only webworker in the react-codemirror package and then I use nodes built in worker_threads to get it to work in the server!

Future improvement - smarter debouncing

When the document is very big, it's easy to queue up a few semantic analysis calls in the webworker (even when debounced) since they can take so long. I think it'd make sense to make the calls into a queue and when one call is finished, all but the latest call are discarded. I carded this here: https://trello.com/c/EvujJSED/219-dont-start-new-semantic-analysis-while-the-old-one-is-still-running-debounce-while-running

Bundling concerns

We should consider how we can split the bundle like this:

Right now only the worker loads the semantic analysis but it's still in the main bundle. My attempt at splitting failed because it was tricky to configure the worker to do the dynamic import. The other option is if we don't bundle, the client application will use their dead code elimination to make sure it doesn't get in the final bundle - i think this would be easier. I've added a card: worker-import-meta-url

.vscode/settings.json

packages/language-support/src/highlighting/syntaxValidation/semanticAnalysisWrapper.ts

packages/language-server/src/worker.ts

packages/react-codemirror/package.json

packages/language-server/package.json

packages/language-support/src/highlighting/syntaxValidation/syntaxValidation.ts

packages/language-server/src/server.ts

packages/language-server/src/worker.ts

packages/language-server/src/lint-worker.ts

OskarDamkjaer · 2024-01-11T14:19:40Z

packages/language-server/src/linting.ts

+      }
+
+      const proxyWorker = (await pool.proxy()) as unknown as LintWorker;
+      lastSemanticJob = proxyWorker.validateSemantics(query);


could pass positions here to avoid doing the the findposition in the main thread

OskarDamkjaer · 2024-01-15T11:08:31Z

packages/react-codemirror/package.json

  "sideEffects": false,
  "scripts": {
-    "build": "concurrently 'npm:build-types' 'npm:build-esm' 'npm:build-commonjs'",
+    "build": "echo done",


Is this correct?

no, this I need to correct. I only got the workerpool working in codemirror by not bundling. But we still need to build for non-typescript consumers

ncordon · 2024-01-17T10:49:56Z

packages/react-codemirror/src/e2e_tests/syntax-validation.spec.tsx

+  page.on('worker', (worker) => {
+    console.log('Worker created: ' + worker.url());
+    worker.on('close', (worker) =>
+      console.log('Worker destroyed: ' + worker.url()),
+    );
+  });


Why do we need this?

We don't, I just used it to debug why the e2e tests were failing

ncordon · 2024-01-17T14:20:13Z

packages/react-codemirror/src/lang-cypher/parser-adapter.ts

+    if (document.length === 0) {
+      this.config.setUseLightVersion?.(false);
+    }


We are assuming someone will wipeout the whole editor always to revert back to the antlr parsing?

Yes. I was originally considering a parse time threshold (if prism parses under Xms it's probably fine with antlr), but that risks some really odd behaviour for outliers. Some queries might be fast with antlr and slow with prism for unexpected reasons and we risk rapid flipping between parsers if the thresholds are poorly set.

Flipping back when the document is simple to understand, and in the worst case scenario (paste large -> empty all -> paste large again), we're still avoiding the main issue (delay between inputs) as we switch back to prism after the first parse

ncordon · 2024-01-17T16:18:50Z

packages/language-server/src/linting.ts

+import { LinterTask, LintWorker } from './lint-worker';
+
+const pool = workerpool.pool(join(__dirname, 'lint-worker.js'), {
+  minWorkers: 2,


Why do we need 2 workers? I think you explained me this already.

I tried including a console.log of the pool.stats() at the beginning of the linting here and at the beginning of the semantic linting and I can see 1 occupied worker, 1 idle one at most.

{totalWorkers: 2, busyWorkers: 1, idleWorkers: 1, pendingTasks: 0, activeTasks: 1}

The intention is to always have an idle worker available, so that we can avoid the startup times of spawning a new thread. My reasoning is that when one thread is busy, we can use the idle one without startup cost and then the old busy thread will be terminated and respawned so it's ready for the next call

I've not measured the start up time/performance gains though

ncordon · 2024-01-29T11:05:52Z

packages/language-server/package.json

@@ -47,6 +49,7 @@
    "watch": "tsc -b -w"
  },
  "devDependencies": {
+    "@types/lodash.debounce": "^4.0.9",


Are the types needed here if we have the whole package in the dependencies?

The lodash project doesn't have any types, they are provided by the "@types" package from microsoft who add types to untyped libraries, that's why it's imported separately. The reason it's in devDependencies is because the types are only needed at the build step

ncordon · 2024-01-29T11:11:27Z

packages/language-support/src/highlighting/syntaxValidation/semanticAnalysisWrapper.ts

@@ -29,7 +31,27 @@ export function doSemanticAnalysis(query: string): SemanticAnalysisResult {
    const errors: SemanticAnalysisElement[] = semanticErrorsResult.$errors.data;


I don't think the things coming back from the transpiled semantic analysis include the severity? So they should not be a SemanticAnalysisElement[]

Ah right, thanks! updated now 👍

ncordon · 2024-01-29T11:20:25Z

packages/react-codemirror/package.json

  },
  "devDependencies": {
    "@neo4j-ndl/base": "^1.10.1",
    "@playwright/experimental-ct-react": "^1.39.0",
    "@playwright/test": "^1.36.2",
+    "@types/lodash.debounce": "^4.0.9",


Same comment, do we need the types package here if it's in the compile dependencies?

ncordon · 2024-01-29T11:21:13Z

packages/react-codemirror/src/CypherEditor.tsx

+  private debouncedOnChange = this.props.onChange
+    ? debounce(this.props.onChange, 200)
+    : undefined;
+


I think the description of the pr is outdated judging by this?

Ah, yes - my bad

* proof of concept * merge main * works again * fix overuse of main channel * mellan * tests work * fix worker thread for node as well * self review * self review * self review * fix build * tests * fix e2e test * merge semantic analsysis and syntax errors again * review comments * self review * fix tests * worker pool-ish for vscode * smarter 'pool' mgmt * kindaworks * works * cleanup new parser adapter * restore pkg json * workerpool * worker pool for client as well * cleanup workers * fix errors * fix tests * fix todos * fix build * test-are-green * works for vite * fixlint * self review * re-add proper build * fix input lag even when parse is fast * add changeset * fix e2e test * pr comments

OskarDamkjaer added 10 commits December 8, 2023 12:53

proof of concept

83cd50e

merge main

83791e1

works again

afdb51e

fix overuse of main channel

43b0980

mellan

a5ba796

Merge branch 'main' into worker_merge_main

52b4641

tests work

a2f8950

fix worker thread for node as well

6fd0862

self review

e83ed7f

self review

fc8ece5

OskarDamkjaer commented Dec 21, 2023

View reviewed changes

.vscode/settings.json Outdated Show resolved Hide resolved

OskarDamkjaer commented Dec 21, 2023

View reviewed changes

packages/language-support/src/highlighting/syntaxValidation/semanticAnalysisWrapper.ts Show resolved Hide resolved

OskarDamkjaer commented Dec 21, 2023

View reviewed changes

packages/language-server/src/worker.ts Outdated Show resolved Hide resolved

OskarDamkjaer commented Dec 21, 2023

View reviewed changes

packages/react-codemirror/package.json Outdated Show resolved Hide resolved

self review

7b4e531

OskarDamkjaer assigned ncordon Dec 21, 2023

OskarDamkjaer added 3 commits December 21, 2023 13:10

fix build

21801d3

tests

5f6fe46

fix e2e test

300cbc3

ncordon reviewed Jan 4, 2024

View reviewed changes

packages/language-server/package.json Outdated Show resolved Hide resolved

ncordon reviewed Jan 4, 2024

View reviewed changes

packages/language-support/src/highlighting/syntaxValidation/syntaxValidation.ts Show resolved Hide resolved

ncordon reviewed Jan 4, 2024

View reviewed changes

packages/language-server/src/server.ts Outdated Show resolved Hide resolved

ncordon reviewed Jan 4, 2024

View reviewed changes

packages/language-server/src/worker.ts Outdated Show resolved Hide resolved

OskarDamkjaer added 4 commits January 4, 2024 17:31

merge semantic analsysis and syntax errors again

5521f65

review comments

df31cfb

self review

d5d1734

fix tests

6365924

OskarDamkjaer requested a review from ncordon January 4, 2024 16:47

OskarDamkjaer added 2 commits January 5, 2024 13:39

worker pool-ish for vscode

7e33da2

smarter 'pool' mgmt

44fcd2b

fix tests

4b9fbd0

OskarDamkjaer commented Jan 10, 2024

View reviewed changes

packages/language-server/src/lint-worker.ts Show resolved Hide resolved

OskarDamkjaer changed the title ~~Move semantic analysis work to a worker~~ Decrease input-lag for large queries Jan 10, 2024

OskarDamkjaer added 2 commits January 10, 2024 14:45

fix todos

aa9ec16

fix build

6f81b7e

OskarDamkjaer marked this pull request as draft January 10, 2024 15:39

OskarDamkjaer commented Jan 11, 2024

View reviewed changes

OskarDamkjaer added 3 commits January 12, 2024 14:26

test-are-green

8119bc5

works for vite

ec8bece

fixlint

293406a

OskarDamkjaer marked this pull request as ready for review January 15, 2024 10:02

OskarDamkjaer commented Jan 15, 2024

View reviewed changes

ncordon reviewed Jan 17, 2024

View reviewed changes

OskarDamkjaer added 6 commits January 22, 2024 12:38

self review

2850ebe

re-add proper build

4652c2d

fix input lag even when parse is fast

2ccf192

add changeset

e282f70

merge conflicts

d89924b

fix e2e test

4062fce

OskarDamkjaer force-pushed the new_entrypoint branch from 1f197f8 to 4062fce Compare January 24, 2024 11:20

OskarDamkjaer assigned ncordon and unassigned ncordon Jan 24, 2024

ncordon approved these changes Jan 29, 2024

View reviewed changes

pr comments

cbfb3ce

OskarDamkjaer merged commit 1e210cb into main Jan 30, 2024
4 checks passed

OskarDamkjaer deleted the new_entrypoint branch January 30, 2024 09:45

OskarDamkjaer mentioned this pull request Jul 31, 2024

Fix CypherEditor state handling and add tests #251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decrease input-lag for large queries #158

Decrease input-lag for large queries #158

OskarDamkjaer commented Dec 21, 2023 •

edited

Loading

OskarDamkjaer Jan 11, 2024

OskarDamkjaer Jan 15, 2024

ncordon Jan 17, 2024

OskarDamkjaer Jan 18, 2024

ncordon Jan 17, 2024

OskarDamkjaer Jan 18, 2024

ncordon Jan 17, 2024

OskarDamkjaer Jan 18, 2024

ncordon Jan 17, 2024

OskarDamkjaer Jan 18, 2024

ncordon Jan 29, 2024

OskarDamkjaer Jan 29, 2024

ncordon Jan 29, 2024

OskarDamkjaer Jan 29, 2024

ncordon Jan 29, 2024

ncordon Jan 29, 2024

OskarDamkjaer Jan 29, 2024

		@@ -29,7 +31,27 @@ export function doSemanticAnalysis(query: string): SemanticAnalysisResult {
		const errors: SemanticAnalysisElement[] = semanticErrorsResult.$errors.data;

Decrease input-lag for large queries #158

Decrease input-lag for large queries #158

Conversation

OskarDamkjaer commented Dec 21, 2023 • edited Loading

Overview

Outdated description below

Future improvement - smarter debouncing

Bundling concerns

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OskarDamkjaer commented Dec 21, 2023 •

edited

Loading