Don't wait for build graph generation when file is changed

Since we re-index source files if the build server sends us a `buildTarget/didChange` notification, we no longer need to wait for an up-to-date build graph when a file is modified. This resolves an issue that causes a `Scheduling tasks` progress to appear in the status bar whenever a file in the project is changed. Before swiftlang#1973, this happened fairly frequently during the initial indexing when a header file was written into the build directory. After that PR the `Scheduling Indexing` status appears a lot more frequently, namely every time the index’s database is modified, which happens quite all the time during indexing...

Author: ahoppen

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -175,7 +175,11 @@ package final actor SemanticIndexManager {
 
   /// The tasks to generate the build graph (resolving package dependencies, generating the build description,
   /// ...) and to schedule indexing of modified tasks.
-  private var scheduleIndexingTasks: [UUID: Task<Void, Never>] = [:]
+  private var buildGraphGenerationTasks: [UUID: Task<Void, Never>] = [:]
+
+  /// Tasks that were  created from `workspace/didChangeWatchedFiles` or `buildTarget/didChange` notifications. Needed
+  /// so that we can wait for these tasks to be handled in the poll index request.
+  private var fileOrBuildTargetChangedTasks: [UUID: Task<Void, Never>] = [:]
 
   private let preparationUpToDateTracker = UpToDateTracker<BuildTargetIdentifier>()
 
@@ -225,7 +229,7 @@ package final actor SemanticIndexManager {
     if inProgressPreparationTasks.values.contains(where: { $0.purpose == .forEditorFunctionality }) {
       return .preparingFileForEditorFunctionality
     }
-    if !scheduleIndexingTasks.isEmpty {
+    if !buildGraphGenerationTasks.isEmpty {
       return .schedulingIndexing
     }
     let preparationTasks = inProgressPreparationTasks.mapValues { inProgressTask in
@@ -270,46 +274,37 @@ package final actor SemanticIndexManager {
   /// Regenerate the build graph (also resolving package dependencies) and then index all the source files known to the
   /// 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
+  /// 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 {
+  package func scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(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()
-        if ensureAllUnitsRegisteredInIndex {
-          index.pollForUnitChangesAndWait()
-        }
+        // Polling for unit changes 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. This operation is worth the cost during initial indexing and during the manual re-index command.
+        index.pollForUnitChangesAndWait()
         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
         let filesToIndex: [DocumentURI] =
-          if let filesToIndex {
-            filesToIndex
-          } else {
-            await orLog("Getting files to index") {
-              try await self.buildSystemManager.buildableSourceFiles().sorted { $0.stringValue < $1.stringValue }
-            } ?? []
-          }
-        _ = await self.scheduleIndexing(
+          await orLog("Getting files to index") {
+            try await self.buildSystemManager.buildableSourceFiles().sorted { $0.stringValue < $1.stringValue }
+          } ?? []
+        await self.scheduleIndexing(
           of: filesToIndex,
+          waitForBuildGraphGenerationTasks: false,
           indexFilesWithUpToDateUnit: indexFilesWithUpToDateUnit,
           priority: .low
         )
-        scheduleIndexingTasks[taskId] = nil
+        buildGraphGenerationTasks[taskId] = nil
       }
     }
-    scheduleIndexingTasks[taskId] = generateBuildGraphTask
+    buildGraphGenerationTasks[taskId] = generateBuildGraphTask
     indexProgressStatusDidChange()
   }
 
@@ -318,15 +313,11 @@ package final actor SemanticIndexManager {
   package func scheduleReindex() async {
     await indexStoreUpToDateTracker.markAllKnownOutOfDate()
     await preparationUpToDateTracker.markAllKnownOutOfDate()
-    await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
-      filesToIndex: nil,
-      ensureAllUnitsRegisteredInIndex: false,
-      indexFilesWithUpToDateUnit: true
-    )
+    await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(indexFilesWithUpToDateUnit: true)
   }
 
   private func waitForBuildGraphGenerationTasks() async {
-    await scheduleIndexingTasks.values.concurrentForEach { await $0.value }
+    await buildGraphGenerationTasks.values.concurrentForEach { await $0.value }
   }
 
   /// Wait for all in-progress index tasks to finish.
@@ -336,6 +327,8 @@ package final actor SemanticIndexManager {
     // can await the index tasks below.
     await waitForBuildGraphGenerationTasks()
 
+    await fileOrBuildTargetChangedTasks.concurrentForEach { await $0.value.value }
+
     await inProgressIndexTasks.concurrentForEach { (_, inProgress) in
       switch inProgress.state {
       case .creatingIndexTask:
@@ -358,86 +351,102 @@ package final actor SemanticIndexManager {
     logger.info(
       "Waiting for up-to-date index for \(uris.map { $0.fileURL?.lastPathComponent ?? $0.stringValue }.joined(separator: ", "))"
     )
-    // If there's a build graph update in progress wait for that to finish so we can discover new files in the build
-    // system.
-    await waitForBuildGraphGenerationTasks()
 
     // Create a new index task for the files that aren't up-to-date. The newly scheduled index tasks will
     // - Wait for the existing index operations to finish if they have the same number of files.
     // - Reschedule the background index task in favor of an index task with fewer source files.
-    await self.scheduleIndexing(of: uris, indexFilesWithUpToDateUnit: false, priority: nil).value
+    await self.scheduleIndexing(
+      of: uris,
+      waitForBuildGraphGenerationTasks: true,
+      indexFilesWithUpToDateUnit: false,
+      priority: nil
+    ).value
     index.pollForUnitChangesAndWait()
     logger.debug("Done waiting for up-to-date index")
   }
 
-  package func filesDidChange(_ events: [FileEvent]) async {
-    // We only re-index the files that were changed and don't re-index any of their dependencies. See the
-    // `Documentation/Files_To_Reindex.md` file.
-    let changedFiles = events.map(\.uri)
-    await indexStoreUpToDateTracker.markOutOfDate(changedFiles)
-
-    // Preparation tracking should be per file. For now consider any non-known-language change as having to re-prepare
-    // the target itself so that we re-prepare potential input files to plugins.
-    // https://github.com/swiftlang/sourcekit-lsp/issues/1975
-    var outOfDateTargets = Set<BuildTargetIdentifier>()
-    for file in changedFiles {
-      let changedTargets = await buildSystemManager.targets(for: file)
-      if Language(inferredFromFileExtension: file) == nil {
-        outOfDateTargets.formUnion(changedTargets)
-      }
-
-      let dependentTargets = await buildSystemManager.targets(dependingOn: changedTargets)
-      outOfDateTargets.formUnion(dependentTargets)
-    }
-    if !outOfDateTargets.isEmpty {
-      logger.info(
-        """
-        Marking dependent targets as out-of-date: \
-        \(String(outOfDateTargets.map(\.uri.stringValue).joined(separator: ", ")))
-        """
-      )
-      await preparationUpToDateTracker.markOutOfDate(outOfDateTargets)
-    }
-
-    await scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
-      filesToIndex: changedFiles,
-      ensureAllUnitsRegisteredInIndex: false,
-      indexFilesWithUpToDateUnit: false
-    )
-  }
-
-  package func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async {
-    let targets = changes?.map(\.target)
+  package func filesDidChange(_ events: [FileEvent]) {
+    // We don't care about the order of file change events, so we can execute the changes in a task, allowing further
+    // file change handling to continue before we have scheduled the file re-indexing.
+    let uuid = UUID()
+    fileOrBuildTargetChangedTasks[uuid] = Task {
+      defer { fileOrBuildTargetChangedTasks[uuid] = nil }
+      // We only re-index the files that were changed and don't re-index any of their dependencies. See the
+      // `Documentation/Files_To_Reindex.md` file.
+      let changedFiles = events.map(\.uri)
+      await indexStoreUpToDateTracker.markOutOfDate(changedFiles)
+
+      // Preparation tracking should be per file. For now consider any non-known-language change as having to re-prepare
+      // the target itself so that we re-prepare potential input files to plugins.
+      // https://github.com/swiftlang/sourcekit-lsp/issues/1975
+      var outOfDateTargets = Set<BuildTargetIdentifier>()
+      for file in changedFiles {
+        let changedTargets = await buildSystemManager.targets(for: file)
+        if Language(inferredFromFileExtension: file) == nil {
+          outOfDateTargets.formUnion(changedTargets)
+        }
 
-    if let targets {
-      var targetsAndDependencies = targets
-      targetsAndDependencies += await buildSystemManager.targets(dependingOn: Set(targets))
-      if !targetsAndDependencies.isEmpty {
+        let dependentTargets = await buildSystemManager.targets(dependingOn: changedTargets)
+        outOfDateTargets.formUnion(dependentTargets)
+      }
+      if !outOfDateTargets.isEmpty {
         logger.info(
           """
           Marking dependent targets as out-of-date: \
-          \(String(targetsAndDependencies.map(\.uri.stringValue).joined(separator: ", ")))
+          \(String(outOfDateTargets.map(\.uri.stringValue).joined(separator: ", ")))
           """
         )
-        await preparationUpToDateTracker.markOutOfDate(targetsAndDependencies)
+        await preparationUpToDateTracker.markOutOfDate(outOfDateTargets)
       }
-    } else {
-      await preparationUpToDateTracker.markAllKnownOutOfDate()
-    }
 
-    await orLog("Scheduling re-indexing of changed targets") {
-      var sourceFiles = try await self.buildSystemManager.sourceFiles(includeNonBuildableFiles: false)
-      if let targets {
-        sourceFiles = sourceFiles.filter { !$0.value.targets.isDisjoint(with: targets) }
-      }
-      _ = await scheduleIndexing(
-        of: sourceFiles.keys,
+      await scheduleIndexing(
+        of: changedFiles,
+        waitForBuildGraphGenerationTasks: true,
         indexFilesWithUpToDateUnit: false,
         priority: .low
       )
     }
   }
 
+  package func buildTargetsChanged(_ changes: [BuildTargetEvent]?) async {
+    // We don't care about the order of target change events, so we can execute the changes in a task, allowing further
+    // target change handling to continue.
+    let uuid = UUID()
+    fileOrBuildTargetChangedTasks[uuid] = Task {
+      defer { fileOrBuildTargetChangedTasks[uuid] = nil }
+      let targets = changes?.map(\.target)
+
+      if let targets {
+        var targetsAndDependencies = targets
+        targetsAndDependencies += await buildSystemManager.targets(dependingOn: Set(targets))
+        if !targetsAndDependencies.isEmpty {
+          logger.info(
+            """
+            Marking dependent targets as out-of-date: \
+            \(String(targetsAndDependencies.map(\.uri.stringValue).joined(separator: ", ")))
+            """
+          )
+          await preparationUpToDateTracker.markOutOfDate(targetsAndDependencies)
+        }
+      } else {
+        await preparationUpToDateTracker.markAllKnownOutOfDate()
+      }
+
+      await orLog("Scheduling re-indexing of changed targets") {
+        var sourceFiles = try await self.buildSystemManager.sourceFiles(includeNonBuildableFiles: false)
+        if let targets {
+          sourceFiles = sourceFiles.filter { !$0.value.targets.isDisjoint(with: targets) }
+        }
+        await scheduleIndexing(
+          of: sourceFiles.keys,
+          waitForBuildGraphGenerationTasks: true,
+          indexFilesWithUpToDateUnit: false,
+          priority: .low
+        )
+      }
+    }
+  }
+
   /// Returns the files that should be indexed to get up-to-date index information for the given files.
   ///
   /// If `files` contains a header file, this will return a `FileToIndex` that re-indexes a main file which includes the
@@ -696,12 +705,23 @@ package final actor SemanticIndexManager {
   /// known to be up-to-date based on `indexStoreUpToDateTracker` or the unit timestamp will not be re-indexed unless
   /// `indexFilesWithUpToDateUnit` is `true`.
   ///
+  /// If `waitForBuildGraphGenerationTasks` is `true` and there are any tasks in progress that wait for an up-to-date
+  /// build graph, wait for those to finish. This is helpful so to avoid the following: We receive a build target change
+  /// notification before the initial background indexing has finished. If indexstore-db hasn't been initialized with
+  /// `pollForUnitChangesAndWait` yet, we might not know that the changed targets' files' index is actually up-to-date
+  /// and would thus schedule an unnecessary re-index of the file.
+  ///
   /// The returned task finishes when all files are indexed.
+  @discardableResult
   package func scheduleIndexing(
     of files: some Collection<DocumentURI> & Sendable,
+    waitForBuildGraphGenerationTasks: Bool,
     indexFilesWithUpToDateUnit: Bool,
     priority: TaskPriority?
   ) async -> Task<Void, Never> {
+    if waitForBuildGraphGenerationTasks {
+      await self.waitForBuildGraphGenerationTasks()
+    }
     // Perform a quick initial check to whether the files is up-to-date, in which case we don't need to schedule a
     // prepare and index operation at all.
     // We will check the up-to-date status again in `IndexTaskDescription.execute`. This ensures that if we schedule

Sources/SourceKitLSP/Workspace.swift

@@ -175,8 +175,6 @@ package final class Workspace: Sendable, BuildSystemManagerDelegate {
     }
     if let semanticIndexManager {
       await semanticIndexManager.scheduleBuildGraphGenerationAndBackgroundIndexAllFiles(
-        filesToIndex: nil,
-        ensureAllUnitsRegisteredInIndex: true,
         indexFilesWithUpToDateUnit: false
       )
     }