Index targets top-down instead of bottom up

We originally started preparing and indexing the low-level targets in the hope that we can unblock the files of those targets as early as possible for indexing and thus get best indexing performance. Revisiting this design decision, we can actually get a ~2x speed up (SourceKit-LSP gets indexed in 2:45 instead of 5:49 minutes on my machine) in initial indexing performance by preparing indexing top-level targets first. This speed-up is for two reasons:
- When preparing a top-level target, all of its dependencies are implicitly also prepared. That means that we don’t need to send any prepare requests to the build server for those targets, which is very advantages for SwiftPM projects, in which the constant cost in the order of 500ms (based on my memory) even for a null build.
- The underlying build server is able to prepare dependencies of the top-level target in parallel instead of implicitly serializing them by preparing all targets in the package in dependency order.

Author: ahoppen

Contributor Documentation/BSP Extensions.md

@@ -100,18 +100,29 @@ To do so, the build server should perform any work that is necessary to typechec
 The server communicates during the initialize handshake whether this method is supported or not by setting `prepareProvider: true` in `SourceKitInitializeBuildResponseData`.
 
 - method: `buildTarget/prepare`
-- params: `PrepareParams`
-- result: `void`
+- params: `BuildTargetPrepareRequest`
+- result: `BuildTargetPrepareResponse`
 
 ```ts
-export interface PrepareParams {
+export interface BuildTargetPrepareRequest {
   /** A list of build targets to prepare. */
   targets: BuildTargetIdentifier[];
 
   /** A unique identifier generated by the client to identify this request.
    * The server may include this id in triggered notifications or responses. **/
   originId?: OriginId;
 }
+
+export interface BuildTargetPrepareResponse {
+  /**
+   * Targets that were implicitly prepared by the prepare request.
+   *
+   * For example, a target may be implicitly prepared if one if its dependents gets prepared.
+   *
+   * When omitted, this is the same as an empty array.
+   */
+  implicitlyPreparedTargets?: BuildTargetIdentifier[]
+}
 ```
 
 ## `buildTarget/sources`

Sources/BuildServerIntegration/BuildServerManager.swift

@@ -1340,24 +1340,53 @@ package actor BuildServerManager: QueueBasedMessageHandler {
     return (depths, dependents)
   }
 
-  /// Sort the targets so that low-level targets occur before high-level targets.
-  ///
-  /// This sorting is best effort but allows the indexer to prepare and index low-level targets first, which allows
-  /// index data to be available earlier.
-  package func topologicalSort(of targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier] {
+  /// Sort the targets in the order they should be indexed in for best performance.
+  package func targetsSortedForIndexing(_ targets: [BuildTargetIdentifier]) async throws -> [BuildTargetIdentifier] {
     guard let buildTargets = await orLog("Getting build targets for topological sort", { try await buildTargets() })
     else {
       return targets.sorted { $0.uri.stringValue < $1.uri.stringValue }
     }
 
-    return targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in
-      let lhsDepth = buildTargets[lhs]?.depth ?? 0
-      let rhsDepth = buildTargets[rhs]?.depth ?? 0
+    // Generate a preliminary work list of targets to index in which we prefer top-level targets over low-level targets
+    // and targets of the root package over targets in dependencies.
+    // We want to index targets in the root package first because those are likely the files that the user is interested
+    // in editing. We want to index top-level targets first, because preparing those likely implies preparation of the
+    // low-level targets.
+    var workList = targets.sorted { (lhs: BuildTargetIdentifier, rhs: BuildTargetIdentifier) -> Bool in
+      let lhsTarget = buildTargets[lhs]
+      let rhsTarget = buildTargets[rhs]
+
+      switch (lhsTarget?.target.tags.contains(.dependency), rhsTarget?.target.tags.contains(.dependency)) {
+      case (true, false): return false
+      case (false, true): return true
+      default: break
+      }
+
+      let lhsDepth = lhsTarget?.depth ?? 0
+      let rhsDepth = rhsTarget?.depth ?? 0
       if lhsDepth != rhsDepth {
-        return lhsDepth > rhsDepth
+        return lhsDepth < rhsDepth
       }
+      // Use the target's name as a tie-breaker
       return lhs.uri.stringValue < rhs.uri.stringValue
     }
+
+    // Now walk through the list of targets in the work list. For each target from the work list, index all of its
+    // transitive dependencies next. We do this because preparing a top-level target likely also prepared all of its
+    // dependencies, so we should be able to index all files in the target's dependencies without needing to perform any
+    // target preparation.
+    var sorted: [BuildTargetIdentifier] = []
+    while !workList.isEmpty {
+      let target = workList.removeFirst()
+      sorted.append(target)
+
+      let transitiveDependencies = transitiveClosure(of: [target]) { Set(buildTargets[$0]?.target.dependencies ?? []) }
+      let dependenciesInWorkList = workList.filter { transitiveDependencies.contains($0) }
+      sorted += dependenciesInWorkList
+      workList.removeAll(where: { dependenciesInWorkList.contains($0) })
+    }
+
+    return sorted
   }
 
   /// Returns the list of targets that might depend on the given target and that need to be re-prepared when a file in
@@ -1373,14 +1402,18 @@ package actor BuildServerManager: QueueBasedMessageHandler {
       .sorted { $0.uri.stringValue < $1.uri.stringValue }
   }
 
-  package func prepare(targets: Set<BuildTargetIdentifier>) async throws {
-    let _: VoidResponse? = try await buildServerAdapterAfterInitialized?.send(
+  package func prepare(targets: Set<BuildTargetIdentifier>) async throws -> BuildTargetPrepareResponse {
+    guard let buildServerAdapterAfterInitialized = try await buildServerAdapterAfterInitialized else {
+      throw ResponseError.unknown("No connection to build server")
+    }
+    let response = try await buildServerAdapterAfterInitialized.send(
       BuildTargetPrepareRequest(targets: targets.sorted { $0.uri.stringValue < $1.uri.stringValue })
     )
     await orLog("Calling fileDependenciesUpdated") {
       let filesInPreparedTargets = try await self.sourceFiles(in: targets).flatMap(\.sources).map(\.uri)
       await filesDependenciesUpdatedDebouncer.scheduleCall(Set(filesInPreparedTargets))
     }
+    return response
   }
 
   package func registerForChangeNotifications(for uri: DocumentURI, language: Language) async {

Sources/BuildServerIntegration/BuiltInBuildServer.swift

@@ -51,7 +51,7 @@ package protocol BuiltInBuildServer: AnyObject, Sendable {
 
   /// Prepare the given targets for indexing and semantic functionality. This should build all swift modules of target
   /// dependencies.
-  func prepare(request: BuildTargetPrepareRequest) async throws -> VoidResponse
+  func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse
 
   /// Retrieve build settings for the given document.
   ///

Sources/BuildServerIntegration/FixedCompilationDatabaseBuildServer.swift

@@ -111,7 +111,7 @@ package actor FixedCompilationDatabaseBuildServer: BuiltInBuildServer {
     }
   }
 
-  package func prepare(request: BuildTargetPrepareRequest) async throws -> VoidResponse {
+  package func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
     throw ResponseError.methodNotFound(BuildTargetPrepareRequest.method)
   }
 

Sources/BuildServerIntegration/JSONCompilationDatabaseBuildServer.swift

@@ -176,7 +176,7 @@ package actor JSONCompilationDatabaseBuildServer: BuiltInBuildServer {
     }
   }
 
-  package func prepare(request: BuildTargetPrepareRequest) async throws -> VoidResponse {
+  package func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
     throw ResponseError.methodNotFound(BuildTargetPrepareRequest.method)
   }
 

Sources/BuildServerIntegration/LegacyBuildServer.swift

@@ -169,7 +169,7 @@ actor LegacyBuildServer: MessageHandler, BuiltInBuildServer {
 
   package func didChangeWatchedFiles(notification: OnWatchedFilesDidChangeNotification) {}
 
-  package func prepare(request: BuildTargetPrepareRequest) async throws -> VoidResponse {
+  package func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
     throw ResponseError.methodNotFound(BuildTargetPrepareRequest.method)
   }
 

Sources/BuildServerIntegration/SwiftPMBuildServer.swift

@@ -714,12 +714,17 @@ package actor SwiftPMBuildServer: BuiltInBuildServer {
     return VoidResponse()
   }
 
-  package func prepare(request: BuildTargetPrepareRequest) async throws -> VoidResponse {
+  package func prepare(request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
     // TODO: Support preparation of multiple targets at once. (https://github.com/swiftlang/sourcekit-lsp/issues/1262)
+    var implicitlyPreparedTargets: [BuildTargetIdentifier] = []
     for target in request.targets {
-      await orLog("Preparing") { try await prepare(singleTarget: target) }
+      await orLog("Preparing") {
+        try await prepare(singleTarget: target)
+        implicitlyPreparedTargets += transitiveClosure(of: [target], successors: { targetDependencies[$0] ?? [] })
+          .filter { !request.targets.contains($0) }
+      }
     }
-    return VoidResponse()
+    return BuildTargetPrepareResponse(implicitlyPreparedTargets: implicitlyPreparedTargets)
   }
 
   private func prepare(singleTarget target: BuildTargetIdentifier) async throws {

Sources/SKTestSupport/CustomBuildServerTestProject.swift

@@ -92,7 +92,7 @@ package protocol CustomBuildServer: MessageHandler {
   func textDocumentSourceKitOptionsRequest(
     _ request: TextDocumentSourceKitOptionsRequest
   ) async throws -> TextDocumentSourceKitOptionsResponse?
-  func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> VoidResponse
+  func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse
   func waitForBuildSystemUpdates(request: WorkspaceWaitForBuildSystemUpdatesRequest) async -> VoidResponse
   nonisolated func onWatchedFilesDidChange(_ notification: OnWatchedFilesDidChangeNotification) throws
   func workspaceWaitForBuildSystemUpdatesRequest(
@@ -243,8 +243,8 @@ package extension CustomBuildServer {
     ])
   }
 
-  func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> VoidResponse {
-    return VoidResponse()
+  func prepareTarget(_ request: BuildTargetPrepareRequest) async throws -> BuildTargetPrepareResponse {
+    return BuildTargetPrepareResponse()
   }
 
   func waitForBuildSystemUpdates(request: WorkspaceWaitForBuildSystemUpdatesRequest) async -> VoidResponse {

Sources/SemanticIndex/PreparationTaskDescription.swift

@@ -110,14 +110,19 @@ package struct PreparationTaskDescription: IndexTaskDescription {
         )
         signposter.endInterval("Preparing", state)
       }
+      let prepareResponse: BuildTargetPrepareResponse?
       do {
-        try await buildServerManager.prepare(targets: Set(targetsToPrepare))
+        prepareResponse = try await buildServerManager.prepare(targets: Set(targetsToPrepare))
       } catch {
         logger.error("Preparation failed: \(error.forLogging)")
+        prepareResponse = nil
       }
       await hooks.preparationTaskDidFinish?(self)
       if !Task.isCancelled {
-        await preparationUpToDateTracker.markUpToDate(targetsToPrepare, updateOperationStartDate: startDate)
+        await preparationUpToDateTracker.markUpToDate(
+          targetsToPrepare + (prepareResponse?.implicitlyPreparedTargets ?? []),
+          updateOperationStartDate: startDate
+        )
       }
     }
   }

Sources/SemanticIndex/SemanticIndexManager.swift

@@ -909,7 +909,9 @@ package final actor SemanticIndexManager {
     // The targets sorted in reverse topological order, low-level targets before high-level targets. If topological
     // sorting fails, sorted in another deterministic way where the actual order doesn't matter.
     var sortedTargets: [BuildTargetIdentifier] =
-      await orLog("Sorting targets") { try await buildServerManager.topologicalSort(of: Array(filesByTarget.keys)) }
+      await orLog("Sorting targets") {
+        try await buildServerManager.targetsSortedForIndexing(Array(filesByTarget.keys))
+      }
       ?? Array(filesByTarget.keys).sorted { $0.uri.stringValue < $1.uri.stringValue }
 
     if Set(sortedTargets) != Set(filesByTarget.keys) {