-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
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
1e8dc13
to
7d33ea2
Compare
I can no longer see
|
1a2409a
to
63f1eec
Compare
63f1eec
to
4588ca6
Compare
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.
LGTM, I profiled this again and can definitely see an improvement 👍
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.
LGTM. Great work!
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.
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.
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) |
Fixed after downgrading to 1.42.0 |
@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.
@ceifa the problem should be fixed now and will be released in Deno v1.42.3. |
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.
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.