Reduce language server latency #1003
Conversation
|
cc @cometkim |
|
Cc @cristianoc |
There was a problem hiding this comment.
What's the perf difference in practice?
In case we're worried about things changing infrequently such as compiler version, one possibility would be to refresh the data from time to time (e.g. based on an interval). Which should have no impact on latency.
Anything else to pay attention to in this PR?
| rescriptVersion: string | undefined; | ||
| bscBinaryLocation: string | null; | ||
| namespaceName: string | null; |
There was a problem hiding this comment.
I guess these 3 are the added cached values.
There was a problem hiding this comment.
Exactly, and rescriptVersion was already cached for incremental compilation, just moved that cache to a more central place.
It seems to vary wildly depending on how the system handles frequently spawning child processes. For me, there's no noticeable difference, but for for example @cometkim and @JonoPrest, there's a pretty massive difference at times.
Yeah this is a good approach. I think what I want us to do soon is what you mentioned in another thread - figure out and write down all of the inputs that might change which the LS needs to react to, and then figure out which of those are worth caring about, and what then to do about them.
Not really no, this should be fairly straight forward. |
|
I certainly experience delays. I've increased my lsp timeout to 5 seconds and still often hit timeouts with rescript ls. It could be a configuration or setup issue but I don't get these delays on other lsps. Rust analyzer for eg also has to index a project before it can be used with the editor (which can take minutes) but it doesn't freeze the editor or hit any timeouts. |
|
@JonoPrest did you try with the VSIX of this PR? |
I did yes 👍🏼 we've been discussing it on discord. I think there was an improvement but I haven't been able to try the project cache configuration yet. |
| // First look in project files | ||
| let foundRootFromProjectFiles: string | null = null; | ||
|
|
||
| for (const rootPath of projectsFiles.keys()) { |
There was a problem hiding this comment.
If I understand correctly here, the most common operation is to find the project info associated with the current open file.
I see you make a { [project root path]: [project info] } index above, but it seems like it's missing the { [open file]: [project root path] } or { [open file]: [project info] } inverted index.
I think we don't need this loop if we're using an index for it in the same way
export const projectRoots = new Map<p.DocumentUri, string>();
export const projects = new Map<string, projectFiles>();There was a problem hiding this comment.
We could do that, but this feels like a good trade off to me. That loop is cheap and it'll also be resilient to if roots change for some reason while the file is open (unlikely, but still).
* avoid recomputing project roots and ReScript versions as much as possible * more precomputation * changelog
This reduces latency in the language server by caching a few project config related things, so that we most often don't need to do any potentially expensive lookups before we call the analysis, or invoke incremental typechecking.
Known issues with this approach: We need to recompute the ReScript version if it changes. I however don't believe we're watching anything already that can help us figure out when to recompute it. Will need to think a bit about how to do that. It's not a big problem though, because changing the ReScript version in a project does not happen often. And a restart of the language server (or closing all project files) will fix it.