Skip to content

Don’t poll the index for unit changes on every filesDidChange call #2003

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

Merged
merged 1 commit into from
Feb 26, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions Sources/SemanticIndex/SemanticIndexManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ private struct OpaqueQueuedIndexTask: Equatable {

private enum InProgressIndexStore {
/// We are waiting for preparation of the file's target to be scheduled. The next step is that we wait for
/// prepration to finish before we can update the index store for this file.
/// preparation to finish before we can update the index store for this file.
///
/// `preparationTaskID` identifies the preparation task so that we can transition a file's index state to
/// `updatingIndexStore` when its preparation task has finished.
Expand Down Expand Up @@ -273,18 +273,25 @@ package final actor SemanticIndexManager {
/// build system that don't currently have a unit with a timestamp that matches the mtime of the file.
///
/// If `filesToIndex` is `nil`, all files in the build system with out-of-date units are indexed.
///
/// If `ensureAllUnitsRegisteredInIndex` is `true`, ensure that all units are registered in the index before
/// triggering the indexing. This is a costly operation since it iterates through all the unit files on the file
/// system but if existing unit files are not known to the index, we might re-index those files even if they are
/// up-to-date. Generally this should be set to `true` during the initial indexing (in which case we might be need to
/// build the indexstore-db) and `false` for all subsequent indexing.
package func scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
filesToIndex: [DocumentURI]?,
ensureAllUnitsRegisteredInIndex: Bool,
indexFilesWithUpToDateUnit: Bool
) async {
let taskId = UUID()
let generateBuildGraphTask = Task(priority: .low) {
await withLoggingSubsystemAndScope(subsystem: indexLoggingSubsystem, scope: "build-graph-generation") {
await hooks.buildGraphGenerationDidStart?()
await self.buildSystemManager.waitForUpToDateBuildGraph()
// Ensure that we have an up-to-date indexstore-db. Waiting for the indexstore-db to be updated is cheaper than
// potentially not knowing about unit files, which causes the corresponding source files to be re-indexed.
index.pollForUnitChangesAndWait()
if ensureAllUnitsRegisteredInIndex {
index.pollForUnitChangesAndWait()
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add some additional documentation to pollForUnitChangesAndWait to describe what it actually does (and the fact it's costly) rather than just "for testing"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I’ll add some to indexstore-db

}
await hooks.buildGraphGenerationDidFinish?()
// TODO: Ideally this would be a type like any Collection<DocumentURI> & Sendable but that doesn't work due to
// https://github.com/swiftlang/swift/issues/75602
Expand Down Expand Up @@ -324,7 +331,11 @@ package final actor SemanticIndexManager {
package func scheduleReindex() async {
await indexStoreUpToDateTracker.markAllKnownOutOfDate()
await preparationUpToDateTracker.markAllKnownOutOfDate()
await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(filesToIndex: nil, indexFilesWithUpToDateUnit: true)
await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
filesToIndex: nil,
ensureAllUnitsRegisteredInIndex: false,
indexFilesWithUpToDateUnit: true
)
}

private func waitForBuildGraphGenerationTasks() async {
Expand Down Expand Up @@ -412,6 +423,7 @@ package final actor SemanticIndexManager {

await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
filesToIndex: changedFiles,
ensureAllUnitsRegisteredInIndex: false,
indexFilesWithUpToDateUnit: false
)
}
Expand Down
1 change: 1 addition & 0 deletions Sources/SourceKitLSP/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
if let semanticIndexManager {
await semanticIndexManager.scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
filesToIndex: nil,
ensureAllUnitsRegisteredInIndex: true,
indexFilesWithUpToDateUnit: false
)
}
Expand Down