Setup incremental indexing on file changes #323
Conversation
fwcd
added
code completion
|
Wow, many thanks for digging into this, looks really good already! Very excited to take a closer look at this, I hope to find some time to do so soon. |
| var progressFactory: Progress.Factory = object: Progress.Factory { | ||
| override fun create(label: String): CompletableFuture<Progress> = CompletableFuture.supplyAsync { Progress.None } | ||
| } |
There was a problem hiding this comment.
Any reason for not using Progress.Factory.None? If it's important that the future completes asynchronously, we may want to update Progress.Factory.None itself since I don't think we use it elsewhere.
There was a problem hiding this comment.
Yeah, Progress.Factory.None makes more sense 👍
| @@ -125,6 +124,9 @@ class SourcePath( | |||
| if (isTemporary) (all().asSequence() + sequenceOf(parsed!!)).toList() | |||
| else all() | |||
| } | |||
|
|
|||
| // Creates a shallow copy | |||
| fun clone(): SourceFile = SourceFile(uri, content, path, parsed, compiledFile, compiledContext, compiledContainer, language, isTemporary) | |||
There was a problem hiding this comment.
Just curious, could we make SourceFile a data class to get this implementation for free? Or isn't that possible because it is an inner class?
There was a problem hiding this comment.
Yeah, data classes cannot be inner at the same time.
| private const val MAX_SHORT_NAME_LENGTH = 80 | ||
| private const val MAX_URI_LENGTH = 511 | ||
|
|
||
| private object Symbols : IntIdTable() { |
There was a problem hiding this comment.
Just to understand, we are now using integers (instead of fully-qualified names) to identify symbols. Can we still be sure that uniqueness is maintained (perhaps by using .uniqueIndex() instead of .index())?
There was a problem hiding this comment.
Right, I didn't use a unique index because I think it would complicate things when a user creates multiple classes with the same fully qualified name. Even though it's illegal (and will lead to compilation errors), it's still technically possible for the user to do this. Therefore, I chose to include the duplicate entries in the index.
We could certainly make it unique, but I think we would need to add some more logic to prevent duplicates. And if we ever start using the index for other things (like definitions, for example) duplicated symbols would be ignored from that.
| } | ||
| } | ||
|
|
||
| private fun Transaction.addDeclarations(declarations: Sequence<DeclarationDescriptor>) = |
There was a problem hiding this comment.
Do we use anything from Transaction's this in this block?
|
Also yes, we should probably move to compiling all files on startup, the question would mostly be how to do so in a in a performant manner (the indexing already blocks many LSP operations despite being asynchronous, so I assume the compiler holds some locks). |
Cool, I can look at this later and introduce it in a following PR if I find a performant way to do it. |
|
@fwcd Hey. I dived a bit into it and changed some things. Now all files are compiled on startup and we only index the dependencies on startup. This solves the issue I initially highlighted. The dependency indexing is also more isolated now. This should make #273 easier to implement as well. Let me know what you think |
|
Looks good, thanks! |
Fixes #272
This seems to work reasonably well for most cases. I only identified a couple of issues so far. I'll describe those at the end.
Up until now, indexing was done when the first file was compiled. The indexing covered the entire project including all the dependencies. After that, no more indexing was performed.
With this new approach, the first time a file is compiled, we index every symbol (including dependencies). The difference now is that on subsequent compilations, the files' declarations are re-indexed. This process is currently removing all the declarations of the changed files and adding the new ones. This might not be the most performant approach (especially since most declarations will be removed just to be added again right after), but I think it'll do for now (I didn't test with very large files).
I initially planned to include the locations of the declarations in the index. Unfortunately, performance can take a big hit when indexing the dependencies because we're fetching locations for all the declarations available. We can probably still have them, but we'll need to come up with a better approach to populate them. I nonetheless kept the DB tables for the locations so we can use them in the future. I can remove these tables if you'd like.
As mentioned in the beginning, I only found two issues so far:
Note that this only re-indexes declarations on file changes. If a new dependency is added, we're not re-indexing yet. This can be done in a later PR.