perf(lsp): Only evict caches on JS side when things actually change #23293

nathanwhit · 2024-04-09T04:18:49Z

Currently we evict a lot of the caches on the JS side of things on every request, namely script versions, script file names, and compiler settings (as of #23283, it's not quite every request but it's still unnecessarily often).

This PR reports changes to the JS side, so that it can evict exactly the caches that it needs too. We might want to do some batching in the future so as not to do 1 request per change.

cli/tsc/99_main_compiler.js

bartlomieju · 2024-04-09T19:15:43Z

I can no longer see op_script_version op calls in the profile (createProgram goes from 50ms to 10ms), however I get these errors:

Failed to request to tsserver TypeError: Cannot read properties of undefined (reading 'kind')
    at isVariableDeclaration (ext:deno_tsc/00_typescript.js:26883:17)
    at isSingleVariableDeclaration (ext:deno_tsc/00_typescript.js:140911:12)
    at tryGetFunctionFromVariableDeclaration (ext:deno_tsc/00_typescript.js:140914:10)
    at getFunctionInfo (ext:deno_tsc/00_typescript.js:140898:18)
    at Object.getRefactorActionsToConvertFunctionExpressions [as getAvailableActions] (ext:deno_tsc/00_typescript.js:140819:18)
    at ext:deno_tsc/00_typescript.js:138302:224
    at flatMapIterator (ext:deno_tsc/00_typescript.js:333:21)
    at flatMapIterator.next (<anonymous>)
    at arrayFrom (ext:deno_tsc/00_typescript.js:870:16)
    at Object.getApplicableRefactors (ext:deno_tsc/00_typescript.js:138300:12)
[Error - 21:14:39] Request textDocument/codeAction failed.
  Message: Invalid request
  Code: -32600 
{"type":"mark","name":"lsp.code_lens","count":10,"args":{"textDocument":{"uri":"file:https:///Users/ib/dev/apps/decohub/apps/linx.ts"}}},
{"type":"measure","name":"lsp.code_lens","count":10,"duration":0.004},
{"type":"mark","name":"lsp.code_action","count":16,"args":{"textDocument":{"uri":"file:https:///Users/ib/dev/apps/decohub/apps/linx.ts"},"range":{"start":{"line":16,"character":7},"end":{"line":16,"character":7}},"context":{"diagnostics":[],"triggerKind":2}}},
{"type":"mark","name":"tsc.request.getApplicableRefactors"},
{"type":"mark","name":"tsc.host.getApplicableRefactors","count":11,"args":["file:https:///Users/ib/dev/apps/decohub/apps/linx.ts",{"pos":375,"end":375},{"quotePreference":"double","includeCompletionsForModuleExports":true,"includeCompletionsForImportStatements":true,"includeCompletionsWithSnippetText":true,"includeAutomaticOptionalChainCompletions":true,"includeCompletionsWithInsertText":true,"includeCompletionsWithClassMemberSnippets":true,"includeCompletionsWithObjectLiteralMethodSnippets":true,"useLabelDetailsInCompletionEntries":true,"allowIncompleteCompletions":true,"importModuleSpecifierPreference":"shortest","importModuleSpecifierEnding":"index","allowTextChangesInNewFiles":true,"providePrefixAndSuffixTextForRename":true,"provideRefactorNotApplicableReason":true,"jsxAttributeCompletionStyle":"auto","includeInlayParameterNameHints":"all","includeInlayParameterNameHintsWhenArgumentMatchesName":false,"includeInlayFunctionParameterTypeHints":true,"includeInlayVariableTypeHints":true,"includeInlayVariableTypeHintsWhenTypeMatchesName":false,"includeInlayPropertyDeclarationTypeHints":true,"includeInlayFunctionLikeReturnTypeHints":true,"includeInlayEnumMemberValueHints":true,"autoImportFileExcludePatterns":[]},null,""]},
{"type":"measure","name":"tsc.request.getApplicableRefactors","count":16,"duration":21.72},
Failed to request to tsserver TypeError: Cannot read properties of undefined (reading 'kind')
    at isVariableDeclaration (ext:deno_tsc/00_typescript.js:26883:17)
    at isSingleVariableDeclaration (ext:deno_tsc/00_typescript.js:140911:12)
    at tryGetFunctionFromVariableDeclaration (ext:deno_tsc/00_typescript.js:140914:10)
    at getFunctionInfo (ext:deno_tsc/00_typescript.js:140898:18)
    at Object.getRefactorActionsToConvertFunctionExpressions [as getAvailableActions] (ext:deno_tsc/00_typescript.js:140819:18)
    at ext:deno_tsc/00_typescript.js:138302:224
    at flatMapIterator (ext:deno_tsc/00_typescript.js:333:21)
    at flatMapIterator.next (<anonymous>)
    at arrayFrom (ext:deno_tsc/00_typescript.js:870:16)
    at Object.getApplicableRefactors (ext:deno_tsc/00_typescript.js:138300:12)
[Error - 21:14:46] Request textDocument/codeAction failed.
  Message: Invalid request
  Code: -32600

cli/lsp/diagnostics.rs

cli/tsc/99_main_compiler.js

bartlomieju

LGTM, I profiled this again and can definitely see an improvement 👍

dsherret

LGTM. Great work!

nayeemrmn

LGTM!

…23293) Currently we evict a lot of the caches on the JS side of things on every request, namely script versions, script file names, and compiler settings (as of #23283, it's not quite every request but it's still unnecessarily often). This PR reports changes to the JS side, so that it can evict exactly the caches that it needs too. We might want to do some batching in the future so as not to do 1 request per change.

ceifa · 2024-04-11T14:25:53Z

I can no longer see op_script_version op calls in the profile (createProgram goes from 50ms to 10ms), however I get these errors:

Failed to request to tsserver TypeError: Cannot read properties of undefined (reading 'kind')
    at isVariableDeclaration (ext:deno_tsc/00_typescript.js:26883:17)
    at isSingleVariableDeclaration (ext:deno_tsc/00_typescript.js:140911:12)
    at tryGetFunctionFromVariableDeclaration (ext:deno_tsc/00_typescript.js:140914:10)
    at getFunctionInfo (ext:deno_tsc/00_typescript.js:140898:18)
    at Object.getRefactorActionsToConvertFunctionExpressions [as getAvailableActions] (ext:deno_tsc/00_typescript.js:140819:18)
    at ext:deno_tsc/00_typescript.js:138302:224
    at flatMapIterator (ext:deno_tsc/00_typescript.js:333:21)
    at flatMapIterator.next (<anonymous>)
    at arrayFrom (ext:deno_tsc/00_typescript.js:870:16)
    at Object.getApplicableRefactors (ext:deno_tsc/00_typescript.js:138300:12)
[Error - 21:14:39] Request textDocument/codeAction failed.
  Message: Invalid request
  Code: -32600 
{"type":"mark","name":"lsp.code_lens","count":10,"args":{"textDocument":{"uri":"file:https:///Users/ib/dev/apps/decohub/apps/linx.ts"}}},
{"type":"measure","name":"lsp.code_lens","count":10,"duration":0.004},
{"type":"mark","name":"lsp.code_action","count":16,"args":{"textDocument":{"uri":"file:https:///Users/ib/dev/apps/decohub/apps/linx.ts"},"range":{"start":{"line":16,"character":7},"end":{"line":16,"character":7}},"context":{"diagnostics":[],"triggerKind":2}}},
{"type":"mark","name":"tsc.request.getApplicableRefactors"},
{"type":"mark","name":"tsc.host.getApplicableRefactors","count":11,"args":["file:https:///Users/ib/dev/apps/decohub/apps/linx.ts",{"pos":375,"end":375},{"quotePreference":"double","includeCompletionsForModuleExports":true,"includeCompletionsForImportStatements":true,"includeCompletionsWithSnippetText":true,"includeAutomaticOptionalChainCompletions":true,"includeCompletionsWithInsertText":true,"includeCompletionsWithClassMemberSnippets":true,"includeCompletionsWithObjectLiteralMethodSnippets":true,"useLabelDetailsInCompletionEntries":true,"allowIncompleteCompletions":true,"importModuleSpecifierPreference":"shortest","importModuleSpecifierEnding":"index","allowTextChangesInNewFiles":true,"providePrefixAndSuffixTextForRename":true,"provideRefactorNotApplicableReason":true,"jsxAttributeCompletionStyle":"auto","includeInlayParameterNameHints":"all","includeInlayParameterNameHintsWhenArgumentMatchesName":false,"includeInlayFunctionParameterTypeHints":true,"includeInlayVariableTypeHints":true,"includeInlayVariableTypeHintsWhenTypeMatchesName":false,"includeInlayPropertyDeclarationTypeHints":true,"includeInlayFunctionLikeReturnTypeHints":true,"includeInlayEnumMemberValueHints":true,"autoImportFileExcludePatterns":[]},null,""]},
{"type":"measure","name":"tsc.request.getApplicableRefactors","count":16,"duration":21.72},
Failed to request to tsserver TypeError: Cannot read properties of undefined (reading 'kind')
    at isVariableDeclaration (ext:deno_tsc/00_typescript.js:26883:17)
    at isSingleVariableDeclaration (ext:deno_tsc/00_typescript.js:140911:12)
    at tryGetFunctionFromVariableDeclaration (ext:deno_tsc/00_typescript.js:140914:10)
    at getFunctionInfo (ext:deno_tsc/00_typescript.js:140898:18)
    at Object.getRefactorActionsToConvertFunctionExpressions [as getAvailableActions] (ext:deno_tsc/00_typescript.js:140819:18)
    at ext:deno_tsc/00_typescript.js:138302:224
    at flatMapIterator (ext:deno_tsc/00_typescript.js:333:21)
    at flatMapIterator.next (<anonymous>)
    at arrayFrom (ext:deno_tsc/00_typescript.js:870:16)
    at Object.getApplicableRefactors (ext:deno_tsc/00_typescript.js:138300:12)
[Error - 21:14:46] Request textDocument/codeAction failed.
  Message: Invalid request
  Code: -32600

Just got the exactly same error on VSCode output (also the types is not working correctly):

deno 1.42.2 (release, x86_64-pc-windows-msvc)
v8 12.3.219.9
typescript 5.4.3

ceifa · 2024-04-11T14:30:30Z

Fixed after downgrading to 1.42.0

bartlomieju · 2024-04-11T16:02:45Z

@ceifa do you only get these errors in Jupyter notebook or when editing regular files as well?

…3322) Fixes the regression described in #23293 (comment). This affected jupyter notebooks, as the LSP was passing in already denormalized specifiers, while the jupyter kernel was not. We need to denormalize the specifiers to evict the proper keys from our caches.

bartlomieju · 2024-04-11T20:06:53Z

@ceifa the problem should be fixed now and will be released in Deno v1.42.3.

ceifa · 2024-04-11T21:13:30Z

Thanks!

…3322) Fixes the regression described in #23293 (comment). This affected jupyter notebooks, as the LSP was passing in already denormalized specifiers, while the jupyter kernel was not. We need to denormalize the specifiers to evict the proper keys from our caches.

…enoland#23293) Currently we evict a lot of the caches on the JS side of things on every request, namely script versions, script file names, and compiler settings (as of denoland#23283, it's not quite every request but it's still unnecessarily often). This PR reports changes to the JS side, so that it can evict exactly the caches that it needs too. We might want to do some batching in the future so as not to do 1 request per change.

…noland#23322) Fixes the regression described in denoland#23293 (comment). This affected jupyter notebooks, as the LSP was passing in already denormalized specifiers, while the jupyter kernel was not. We need to denormalize the specifiers to evict the proper keys from our caches.

nathanwhit added 4 commits April 8, 2024 21:10

Only evict caches when things actually change

1d6c727

Appease linter

62aa347

Open is modification + accept ModuleSpecifier + fix test

9121211

Update test

7d33ea2

nathanwhit force-pushed the lsp-granular-js-cache-eviction branch from 1e8dc13 to 7d33ea2 Compare April 9, 2024 16:07

bartlomieju reviewed Apr 9, 2024

View reviewed changes

cli/tsc/99_main_compiler.js Show resolved Hide resolved

nathanwhit commented Apr 10, 2024

View reviewed changes

cli/lsp/diagnostics.rs Show resolved Hide resolved

nathanwhit commented Apr 10, 2024

View reviewed changes

cli/lsp/diagnostics.rs Show resolved Hide resolved

nathanwhit force-pushed the lsp-granular-js-cache-eviction branch from 1a2409a to 63f1eec Compare April 10, 2024 20:13

Fix race between did_change and diagnostics server

4588ca6

nathanwhit force-pushed the lsp-granular-js-cache-eviction branch from 63f1eec to 4588ca6 Compare April 10, 2024 20:16

bartlomieju reviewed Apr 10, 2024

View reviewed changes

cli/tsc/99_main_compiler.js Show resolved Hide resolved

bartlomieju approved these changes Apr 10, 2024

View reviewed changes

Don't evict name cache on change

69ffdf9

dsherret approved these changes Apr 11, 2024

View reviewed changes

nayeemrmn approved these changes Apr 11, 2024

View reviewed changes

nathanwhit merged commit 736f73b into denoland:main Apr 11, 2024
17 checks passed

nathanwhit deleted the lsp-granular-js-cache-eviction branch April 11, 2024 01:07

nathanwhit mentioned this pull request Apr 11, 2024

fix(lsp): Denormalize specifiers before calling $projectChanged #23322

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

perf(lsp): Only evict caches on JS side when things actually change #23293

perf(lsp): Only evict caches on JS side when things actually change #23293

nathanwhit commented Apr 9, 2024 •

edited

Loading

bartlomieju commented Apr 9, 2024 •

edited

Loading

bartlomieju left a comment

dsherret left a comment

nayeemrmn left a comment

ceifa commented Apr 11, 2024

ceifa commented Apr 11, 2024

bartlomieju commented Apr 11, 2024

bartlomieju commented Apr 11, 2024

ceifa commented Apr 11, 2024

perf(lsp): Only evict caches on JS side when things actually change #23293

perf(lsp): Only evict caches on JS side when things actually change #23293

Conversation

nathanwhit commented Apr 9, 2024 • edited Loading

bartlomieju commented Apr 9, 2024 • edited Loading

bartlomieju left a comment

Choose a reason for hiding this comment

dsherret left a comment

Choose a reason for hiding this comment

nayeemrmn left a comment

Choose a reason for hiding this comment

ceifa commented Apr 11, 2024

ceifa commented Apr 11, 2024

bartlomieju commented Apr 11, 2024

bartlomieju commented Apr 11, 2024

ceifa commented Apr 11, 2024

nathanwhit commented Apr 9, 2024 •

edited

Loading

bartlomieju commented Apr 9, 2024 •

edited

Loading