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

WIP: Improve completion performance through caching #11269

Closed
wants to merge 17 commits into from

Conversation

danielhuang
Copy link

@danielhuang danielhuang commented Jan 13, 2022

Contains very poorly written code.

#7542

@danielhuang
Copy link
Author

The caching system mostly works with mainly method completions.

@danielhuang danielhuang marked this pull request as draft January 15, 2022 17:19
@danielhuang danielhuang force-pushed the completion branch 2 times, most recently from c5a8c54 to 5b4fe7f Compare February 18, 2022 21:23
@danielhuang danielhuang changed the title WIP: Improve completion performance through caching Improve completion performance through caching Feb 18, 2022
@danielhuang danielhuang marked this pull request as ready for review February 18, 2022 22:36
@danielhuang danielhuang changed the title Improve completion performance through caching WIP: Improve completion performance through caching Feb 19, 2022
@danielhuang
Copy link
Author

danielhuang commented Feb 19, 2022

The main reason tests are failing is the lack of automatic imports. The automatic import is still working in VSCode. scope is currently missing from ImportEdit because ImportEdit does not implement Send, and the only purpose it serves is to provide automatic import tests for completion.

@Veykril
Copy link
Member

Veykril commented Feb 21, 2022

The automatic import is still working in VSCode. scope is currently missing from ImportEdit because ImportEdit does not implement Send, and the only purpose it serves is to provide automatic import tests for completion.

TIL, I always thought that was required for completion resolving but turns out it isn't. I wonder if would make sense cfg(test) that(To make this Sendable one could lower the SyntaxNode inside of it to a GreenNode and reconstruct that on demand).

That aside, have you benchmarked this(via manual testing), that is has this improved out completion times noticably? This adds a lot of complexity and I believe this currently won't improve completion times too much given most of our issues come from salsa query revalidation. This will also behave rather oddly with flyimport completions given how they currently work I believe, we only kick these off at 3 characters typed as it is right now, and afterwards we can get new completions as we cap the maximum number of flyimport completions we generate, so this cache will limit our completion capabilities quite significantly in this regard.

So while we definitely do want to do some form of caching at some point, given how flyimport currently works for us I am not sure if this is feasible at the moment.
(I apologize for not getting into the matter earlier)

@danielhuang
Copy link
Author

danielhuang commented Feb 21, 2022

This implementation of caching does feel pretty hacky.

That aside, have you benchmarked this(via manual testing), that is has this improved out completion times noticably?

The main purpose of the caching is to prevent typing from causing the completion to "reset". For example, in VSCode, typing a single letter would cause the extension to "recalculate" the completions, which would cause VSCode to stop showing the completions after typing (since the second letter is typed before the calculation finishes). The caching would re-emit the previously calculated completions which would allow VSCode to continue showing the completions.

I've also tried Rust development in CLion (using IntelliJ Rust) and GNOME's Builder (using rust-analyzer). Both of these editors appear to perform this type of caching on the client side (the backend will only perform the calculation a single time after typing .).

@road2react
Copy link

what is Caching

@danielhuang danielhuang force-pushed the completion branch 2 times, most recently from 37b6d46 to d270152 Compare March 3, 2022 19:59
@danielhuang
Copy link
Author

I've been trying different attempts. I will close this for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants