Skip to content
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

lsp: replace code lens navigation tree with swc "collector" #11032

Open
kitsonk opened this issue Jun 18, 2021 · 4 comments
Open

lsp: replace code lens navigation tree with swc "collector" #11032

kitsonk opened this issue Jun 18, 2021 · 4 comments
Labels
lsp related to the language server refactor

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Jun 18, 2021

Currently, the TypeScript code lenses for implementations and references use the TypeScript "navigation tree", which is a light-weight AST, to identify positions in the code that might be "interesting" to lazily ask the TypeScript language server about at a future point.

Getting a navigation tree is one of the slower processes with the language server, and is several orders of magnitude slower than visiting an swc parsed AST. In fact, we actually don't currently cache/reuse an swc parsed AST for a file across the language server, reparsing a file a few times depending on the types of requests to a language server.

I recently refactored the code for the code lenses to make them a little bit more self contained. The logic for walking the "navigation tree" AST is in lsp::code_lens::collect_tsc and I implemented with test code lens using the "collector" pattern in lsp::code_lens::collect_test which is in my opinion the way we should refactor the TypeScript code lenses.

Also, caching the swc parse of the module in the document info would be a good thing I think. That might take a bit more structuring of code. It is used in dependency analysis of the document as well as the test code lens and also use by the linting (though I don't know how easy it would be to send it to deno_lint without a bigger refactor of one or the other).

This is the refactor I had in my mind @dsherret that I mentioned to you. Let me know if you want to work on it, if not, we can always add it to my backlog. 😄

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 7, 2021

@dsherret if you don't feel too strongly about this one, I would be interested in taking this one back up. I think it could dramatically improve the performance of the LSP.

@dsherret
Copy link
Member

dsherret commented Nov 7, 2021

@kitsonk I was thinking about working on it in January/Feb after the bulk of the node compatibility improvements. Feel free to take a look sooner if you'd like.

@dsherret
Copy link
Member

I've been looking at the LSP code more over the past few days and eliminating the NavigationTree would be very nice. Right now there's a race condition if we were to make the code concurrent in that an out of date navigation tree might end up being stored in a document that it wasn't created from. This could be eliminated by implementing this feature and it would reduce our mutation points.

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 10, 2021

To fix that race condition, we need to supply the LSP version on the call and have it come back in the response and abandon it. But the bigger win would be cancelling in flight requests, but that is a bit of a challenge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lsp related to the language server refactor
Projects
None yet
Development

No branches or pull requests

2 participants