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

Cache Symbol Index #489

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Cache Symbol Index #489

wants to merge 1 commit into from

Conversation

daplf
Copy link
Contributor

@daplf daplf commented Sep 6, 2023

Related to #328

Adds caching to the Symbol Index using the recently introduced on disk DB.

Since we are now caching the symbol index, the only way to rebuild it is by changing the dependencies (triggered when we change the dependency files such as pom.xml or build.gradle). Note that this was not the case before. Changing the dependencies previously did not update the symbol index.

I am not sure whether this is the right way to go though. We are currently updating dependencies whenever we save the dependencies file. If a user abuses this, the server could have issues. This was already true before, but it would now be more severe because each change could lead to an entire symbol index refresh.

I think we should probably try to build a more robust mechanism to update dependencies (e.g, the current one would re-run maven/gradle even if you only make a meaningless change, such as adding a space somewhere). Ideally, we would only update dependencies (and the symbol index) if the dependencies actually changed. Moreover, in such a scenario, we should only update the relevant symbol index entries and not all of them. E.g., if we add a new dependency, we should only add the new symbols to the index, keeping the other ones untouched.

Would appreciate some ideas/discussion around this.

@daplf daplf marked this pull request as ready for review September 6, 2023 12:07
@themkat themkat added the enhancement New feature or request label Oct 11, 2023
@themkat themkat mentioned this pull request Oct 23, 2023
@kohnish
Copy link

kohnish commented Oct 23, 2023

This patch works well on a small android project with a single additional file.
A few minutes startup time gets reduced to instant. I'll try on a bigger project later.

With GNU Emacs you need something like this to use persistent cache feature. (not sure if multi projects would be supported this way.)

(add-to-list 'eglot-server-programs '(kotlin-mode  . ("kotlin-language-server" :initializationOptions (:storagePath "/tmp"))))

Thanks a lot for the patch.

Copy link
Collaborator

@themkat themkat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to work well! good job 😄 Sorry for taking so long to review... 🙁

The problem about not updating dependencies redundantly (e.g, just adding a space to pom.xml) can be a bit complex. A few non-perfect ideas to get the discussion starting (both needs heavy modification if we use a variant of them):

Run the dependency file (e.g, pom.xml) through a "formatter" internally

This is a very stupid solution, but we avoid whitespace changes. If we want to compare the old vs new, we can for example calculate a checksum. Most checksums take whitespace into account if I don't remember wrong, so that is the reason for the formatter step.

Have a book-keeping mechanism of the currently used dependencies

When creating the index the first time, we fetch a list of dependencies using Maven/Gradle/other. We currently fetch EVERYTHING, including transitive dependencies. To do the book-keeping, that is probably way too much (though it can be a starting point I guess), but creating another set of logic around Maven or Gradle will probably slow the server down even further.

When the pom.xml/build.gradle/potatofile changes, we can then check if the list of dependencies has changed.

This might be slow for bigger projects.

@daplf
Copy link
Contributor Author

daplf commented Nov 23, 2023

Thank you both for testing this :) And thanks for the suggestions (I'd like to get some sort of discussion going).

Have a book-keeping mechanism of the currently used dependencies

This sounds like a pretty interesting solution to me tbh. And it could help us with some other things. Right now, we re-index the entire project when the file changes. If we rely on the list of dependencies, we can technically just add (or remove) the symbols relevant to that dependency. This would actually improve re-indexing performance quite a bit. In any case, this is something we can always iterate on as well.

We currently fetch EVERYTHING, including transitive dependencies.

I also agree we may be fetching too much stuff at the moment and could probably optimize it. Unfortunately, the current mechanism heavily relies on the compiler APIs, which are under-documented, which means improving this may not be that straightforward.

I'll give this some more thought and maybe do some experiments if I have time. In the meantime, if anyone has any other ideas and/or thoughts, feel free to contribute to the discussion!

@themkat
Copy link
Collaborator

themkat commented Nov 29, 2023

Have a book-keeping mechanism of the currently used dependencies

This sounds like a pretty interesting solution to me tbh. And it could help us with some other things. Right now, we re-index the entire project when the file changes. If we rely on the list of dependencies, we can technically just add (or remove) the symbols relevant to that dependency. This would actually improve re-indexing performance quite a bit. In any case, this is something we can always iterate on as well.

One added benefit of this is that it might help us create other analysis methods and similar. The Eclipse Java language server (jdtls) currently has a custom methods that lists out dependencies in a project, which makes for nice dropdown menus of jar files as seen used in packages like lsp-treemacs (for Emacs):

(shamelessly stolen from: https://github.com/emacs-lsp/lsp-treemacs#lsp-treemacs-deps-list )

If we have a book keeping list of jar files, it could easily help us implement functionality like that, and maybe also other things, in the future 🙂

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

Successfully merging this pull request may close these issues.

3 participants