Split up-to-date status tracking and index progress tracking #1322
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were mixing the up-to-date status and in-progress status of an index task in `SemanticIndexManager`. This meant that a single `QueuedTask` in the task scheduler could be needed for eg. both preparation for editor functionality in a file of that target and to re-index a file in that target. This dual ownership made it unclear, which caller would be entitled to cancel the task. Furthermore, we needed to duplicate some logic from the preparation task dependencies in `SemanticIndexManager.prepare`. To simplify things: - Split the up-to-date status and the in-progress status into two different data structures - Make the caller of `prepare` and `scheduleIndex` responsible for cancellation of the task it has scheduled. `TaskScheduler` might receive more scheduled tasks this way but the additional tasks should all be no-ops because the status is known to be up-to-date when they execute.
|
@swift-ci Please test |
|
@swift-ci Please test Linux |
|
@swift-ci please test Linux platform |
| } else { | ||
| // We couldn't determine which targets depend on the modified targets. Be conservative and assume all of them do. | ||
| preparationStatus = [:] | ||
| await indexStoreUpToDateStatus.markOutOfDate(changedFiles) |
There was a problem hiding this comment.
Isn't this already done a few lines above?
Comment on lines
+471
to
+474
| inProgressIndexTasks[file.sourceFile] = .waitingForPreparation( | ||
| preparationTaskID: preparationTaskID, | ||
| indexTask: indexTask | ||
| ) |
There was a problem hiding this comment.
Could we potentially run into a case?
- We kick off indexing for a single file
- We then schedule the indexing for a whole bunch of files that include that file
inProgressIndexTasksgets set to the new preparation task id for the large group of files- The indexing job for the single file gets completed, but that won't be reflected in
inProgressIndexTasks, soinProgressIndexFileswill still list it as running - We then have to wait for the large file job to complete before the file is marked as completed
And does it matter?
There was a problem hiding this comment.
Yes, we can run into that case but I don’t think it matters because the two uses of inProgressIndexFiles are:
- Index progress reporting: The effect of this will be that the numerator of the progress will only be incremented to reflect that the single file is up-to-date only when the batch is indexed, ie. it’s lower by 1 for a little while longer. Since we’re also waiting for other files to be indexed, I don’t think it actually matters
- Waiting for an up-to-date index: In that case we want to wait for all index tasks to finish anyway, including the batch.
ahoppen
added a commit
to ahoppen/sourcekit-lsp
that referenced
this pull request
May 22, 2024
bnbarham
reviewed
May 22, 2024
| import SKCore | ||
|
|
||
| /// Keeps track of whether an item (a target or file to index) is up-to-date. | ||
| actor IndexUpToDateStatusManager<Item: Hashable> { |
There was a problem hiding this comment.
Could drop the Index? Could be OperationStatusManager? Not a huge fan of Manager though 🤔. OperationStatusTracker? 🤷
There was a problem hiding this comment.
I think I’d like to stick with UpToDate in the name because the two states are upToDate and outOfDate. I’ll go with UpToDateTracker.
| await indexStoreUpToDateStatus.markOutOfDate(changedFiles) | ||
|
|
||
| await preparationUpToDateStatus.markAllOutOfDate() | ||
| // `markAllOutOfDate` only marks targets out-of-date that have been indexed before. Also mark all targets with |
ahoppen
added a commit
to ahoppen/sourcekit-lsp
that referenced
this pull request
May 24, 2024
ahoppen
added a commit
to ahoppen/sourcekit-lsp
that referenced
this pull request
May 24, 2024
ahoppen
added a commit
to ahoppen/sourcekit-lsp
that referenced
this pull request
May 25, 2024
ahoppen
added a commit
to ahoppen/sourcekit-lsp
that referenced
this pull request
May 25, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were mixing the up-to-date status and in-progress status of an index task in
SemanticIndexManager. This meant that a singleQueuedTaskin the task scheduler could be needed for eg. both preparation for editor functionality in a file of that target and to re-index a file in that target. This dual ownership made it unclear, which caller would be entitled to cancel the task. Furthermore, we needed to duplicate some logic from the preparation task dependencies inSemanticIndexManager.prepare.To simplify things:
prepareandscheduleIndexresponsible for cancellation of the task it has scheduled.TaskSchedulermight receive more scheduled tasks this way but the additional tasks should all be no-ops because the status is known to be up-to-date when they execute.