-
Notifications
You must be signed in to change notification settings - Fork 10
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
Decrease input-lag for large queries #158
Changes from all commits
83cd50e
83791e1
afdb51e
43b0980
a5ba796
52b4641
a2f8950
6fd0862
e83ed7f
fc8ece5
7b4e531
21801d3
5f6fe46
300cbc3
5521f65
df31cfb
d5d1734
6365924
7e33da2
44fcd2b
b6dbbc0
136f890
4c94483
8bd4792
626a6e3
ba5ac35
c5a30f0
6e557dc
4b9fbd0
aa9ec16
6f81b7e
8119bc5
ec8bece
293406a
2850ebe
4652c2d
2ccf192
e282f70
d89924b
4062fce
cbfb3ce
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
--- | ||
'@neo4j-cypher/react-codemirror-playground': patch | ||
'@neo4j-cypher/language-support': patch | ||
'@neo4j-cypher/react-codemirror': patch | ||
'neo4j-cypher-vscode-extension': patch | ||
'@neo4j-cypher/language-server': patch | ||
--- | ||
|
||
Moves semantic analysis to a separate worker file |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
import { validateSemantics } from '@neo4j-cypher/language-support'; | ||
OskarDamkjaer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
import workerpool from 'workerpool'; | ||
|
||
workerpool.worker({ validateSemantics }); | ||
|
||
type LinterArgs = Parameters<typeof validateSemantics>; | ||
|
||
export type LinterTask = workerpool.Promise< | ||
ReturnType<typeof validateSemantics> | ||
>; | ||
|
||
export type LintWorker = { | ||
validateSemantics: (...args: LinterArgs) => LinterTask; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
import { | ||
findEndPosition, | ||
parserWrapper, | ||
validateSyntax, | ||
} from '@neo4j-cypher/language-support'; | ||
import debounce from 'lodash.debounce'; | ||
import { join } from 'path'; | ||
import { Diagnostic, TextDocumentChangeEvent } from 'vscode-languageserver'; | ||
import { TextDocument } from 'vscode-languageserver-textdocument'; | ||
import workerpool from 'workerpool'; | ||
import { LinterTask, LintWorker } from './lint-worker'; | ||
|
||
const pool = workerpool.pool(join(__dirname, 'lint-worker.js'), { | ||
minWorkers: 2, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need 2 workers? I think you explained me this already. I tried including a
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
workerTerminateTimeout: 2000, | ||
}); | ||
|
||
let lastSemanticJob: LinterTask | undefined; | ||
|
||
async function rawLintDocument( | ||
change: TextDocumentChangeEvent<TextDocument>, | ||
sendDiagnostics: (diagnostics: Diagnostic[]) => void, | ||
) { | ||
const { document } = change; | ||
|
||
const query = document.getText(); | ||
if (query.length === 0) { | ||
return; | ||
} | ||
|
||
const syntaxErrors = validateSyntax(query, {}); | ||
|
||
sendDiagnostics(syntaxErrors); | ||
|
||
if (syntaxErrors.length === 0) { | ||
try { | ||
if (lastSemanticJob !== undefined && !lastSemanticJob.resolved) { | ||
void lastSemanticJob.cancel(); | ||
} | ||
|
||
const proxyWorker = (await pool.proxy()) as unknown as LintWorker; | ||
lastSemanticJob = proxyWorker.validateSemantics(query); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could pass positions here to avoid doing the the findposition in the main thread |
||
const result = await lastSemanticJob; | ||
|
||
sendDiagnostics( | ||
result.map((el) => findEndPosition(el, parserWrapper.parsingResult)), | ||
); | ||
} catch (err) { | ||
if (!(err instanceof workerpool.Promise.CancellationError)) { | ||
console.error(err); | ||
} | ||
} | ||
} | ||
} | ||
|
||
export const lintDocument = debounce(rawLintDocument, 600, { | ||
leading: false, | ||
trailing: true, | ||
}); | ||
|
||
export const cleanupWorkers = () => { | ||
void pool.terminate(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the types needed here if we have the whole package in the dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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