-
Notifications
You must be signed in to change notification settings - Fork 128
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
indexer/walker: Avoid running jobs where not needed #1006
Conversation
7cf644d
to
044747d
Compare
e27d3d0
to
e774671
Compare
e774671
to
d425b9a
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.
Nice work and thanks for the detailed PR description! 👍
I've found just two minor things
@dbanck I found a slightly different solution, although the difference is really minor given how rarely (if ever) scheduling of the job would fail - this would have something to do with memdb or the underlying indexes. So if/when that happens, it's more likely we'd be unable to schedule any/all jobs, so most of this conditional logic will likely never come into play. PTAL |
(not meant as part of this PR) Maybe we could avoid all the checks when creating jobs and terraform-ls/internal/state/jobs.go Lines 66 to 75 in a2fa3a0
|
Hmm, I like that idea! 🤔 |
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. |
Closes #1002
Depends on #1027
Background
The removal of de-duplication upon job enqueuing is necessary to avoid various bugs and race conditions where we'd deduplicate jobs which appear to be same, but have different
Defer
orDependsOn
, i.e. we'd be overly aggressive in the deduplication efforts.It is worth clarifying here that this PR doesn't affect jobs which run as part of text synchronization - i.e.
textDocument/didOpen
,textDocument/didChange
orworkspace/didChangeWatchedFiles
. It only aims to reduce the duplicated work as part of the walker (which is triggered byinitialize
), which would occur if the user has deep workspace which takes a while to index via walker, and opens a few (yet unindexed) modules, which get indexed as a priority.For
textDocument/didChange
andworkspace/didChangeWatchedFiles
it is expected that the jobs do need to run since we know something has changed. We could attempt to do some de-duplication there as well, but it's probably going to have low impact.For
textDocument/didOpen
- we treat any opened document as a change currently, because we have no way to tell whether it matches the contents on disk. This is IMO a common case, where user would start without any open files, walker indexes everything, and then they open any module and we re-index the whole module again, and we do it again for every single file they open. Reducing duplicated work here is just little more involved, so I filed a separate ticket for that: #1031Benchmarks
I am (finally) updating the benchmarks as part of the PR, which may suggest that this PR has negative performance impact, but it's little more complicated as we're really reflecting a few PRs, each contributing to those numbers in slightly different way:
terraform providers schema -json
when it's not necessary. Previously we'd run it on every indexed & initialized module.required_version
constraint #1027 removed 1 job (terraform version
) entirely from the walker indexing. It now runs only on the text synchronization methods.