-
Notifications
You must be signed in to change notification settings - Fork 131
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
LS state & performance refactoring #1667
Conversation
b0ad1b4
to
c8d820d
Compare
914ac8b
to
0c5e3d6
Compare
0c5e3d6
to
dcea7ba
Compare
9a174bf
to
dd2dd71
Compare
b326916
to
d4706f9
Compare
3d0d8f9
to
67551b4
Compare
7fc8086
to
c9759bc
Compare
c9759bc
to
3b8df0a
Compare
The EventBus has a fixed list of topics that anyone can subscribe to. Whenever an event is published on a topic, all subscribers are notified.
This moves all logic related to Terraform root modules into a feature. The feature keeps track of installed providers and modules.
This moves all logic related to module files into a feature. The feature contains all state, jobs, hooks, and decoder related files.
3b8df0a
to
99bb90f
Compare
Document synchronization handlers will now fire an event for every incoming request. Features can subscribe to these events. Language feature handlers will now wait for running jobs for a directory before attempting to complete the request and return data.
99bb90f
to
9e72a75
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// TODO? Can features contribute commands, so we don't have to import | ||
// the features here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to create issues for this somewhere? (I'm fine with leaving these comments as is)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that we might need to update our docs (e.g. |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
SchemaMerger
interface terraform-schema#335TL;DR: This PR tries to solve two main problems:
I would suggest that you review this PR commit by commit. First, I introduce an event bus to tie everything together. Then I introduce the concept of features to the codebase along with three new features:
.terraform
,.terraform.lock.hcl
)*.tf
,*.tf.json
)*.tfvars
,*.tfvars.json
)Each feature decouples and isolates all functionality related to handling these files (or directories). It has its own internal state, its own set of jobs, its own subscription to specific event topics, and its own decoder (if applicable).
It's intended to have more features in the future: a
stacks
feature, atests
feature, and some for other Terraform-related languages/things.Now that the features are in place, we can remove everything from the existing packages that belongs in a feature. Mainly we can clean up the global state, the terraform package and the walker & indexer. Only structures that are relevant to the language server as a whole (job scheduler, schema store) or that are intended to be used in conjunction with multiple features (change batches) should live at the global package level.
I also refactored the walker and removed the scheduling of jobs when walking a workspace after opening it. We still detect and collect all directories relevant to terraform-ls, but we don't index them. Only when a user opens a file do we schedule jobs to parse the modules, variables, and root module files if applicable. This results in faster startup times and overall less CPU and memory usage.
Also, the text synchronization handlers now don't block on incoming requests. Previously, we would wait for all jobs to finish processing before accepting the next
didChange
request. Now, instead, we wait in the language feature handlers (e.g.complete
orhover
) until all jobs for a directory have been processed before returning a result. This should result in a better user experience and better job deduplication in the queue.Known issues:
Missing decoding of installed modules fixed in #1760
I plan to address this in a separate PR. Since we don't index the whole
.terraform
directory at startup, we need to introduce another way to detect installed modules. We can extend the existing local module detection on open to detect installed modules as well. The only missing piece is a mapping from a module's source to its installation location on disk.The global workspace symbol LSP method is now less accurate
Since we only keep open modules in memory, the handler will only report symbols for all modules that have been opened at least once. This is a tradeoff for not indexing the entire workspace at startup. We can still look at a low resource background index job or kick off indexing the workspace when a sends a workspace symbol request. At that point, we can report progress in the UI.
Flaky tests?
Keep monitoring
Rough performance estimates:
A
a workspace with ~50k lines of Terraform codeB
a workspace with ~600 lines of Terraform codeon an Intel i9 2,4Ghz 8 Core MacBook Pro
main
new
Closes: