From e295a4e95a2014183f49c421f4f155f550f54230 Mon Sep 17 00:00:00 2001 From: Alex Hoppen Date: Mon, 20 May 2024 21:01:40 -0700 Subject: [PATCH] Split up-to-date status tracking and index progress tracking 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. --- Sources/SKCore/TaskScheduler.swift | 46 ++- Sources/SKSupport/Sequence+AsyncMap.swift | 15 + Sources/SemanticIndex/CMakeLists.txt | 1 + .../SemanticIndex/IndexStatusManager.swift | 70 ++++ .../PreparationTaskDescription.swift | 17 +- .../SemanticIndex/SemanticIndexManager.swift | 311 +++++++++--------- .../UpdateIndexStoreTaskDescription.swift | 10 + .../SourceKitLSP/IndexProgressManager.swift | 2 +- Tests/SKCoreTests/TaskSchedulerTests.swift | 4 +- .../BackgroundIndexingTests.swift | 3 +- 10 files changed, 300 insertions(+), 179 deletions(-) create mode 100644 Sources/SemanticIndex/IndexStatusManager.swift diff --git a/Sources/SKCore/TaskScheduler.swift b/Sources/SKCore/TaskScheduler.swift index 77e0f7ac2..23be76912 100644 --- a/Sources/SKCore/TaskScheduler.swift +++ b/Sources/SKCore/TaskScheduler.swift @@ -87,7 +87,7 @@ public enum TaskExecutionState { case finished } -fileprivate actor QueuedTask { +public actor QueuedTask { /// Result of `executionTask` / the tasks in `executionTaskCreatedContinuation`. /// See doc comment on `executionTask`. enum ExecutionTaskFinishStatus { @@ -147,14 +147,38 @@ fileprivate actor QueuedTask { /// Gets reset every time `executionTask` finishes. nonisolated(unsafe) private var cancelledToBeRescheduled: AtomicBool = .init(initialValue: false) + private nonisolated(unsafe) var _isExecuting: AtomicBool = .init(initialValue: false) + + /// Whether the task is currently executing or still queued to be executed later. + public nonisolated var isExecuting: Bool { + return _isExecuting.value + } + + /// Wait for the task to finish. + /// + /// If the tasks that waits for this queued task to finished is cancelled, the QueuedTask will still continue + /// executing. + public func waitToFinish() async { + return await resultTask.value + } + + /// Wait for the task to finish. + /// + /// If the tasks that waits for this queued task to finished is cancelled, the QueuedTask will also be cancelled. + /// This assumes that the caller of this method has unique control over the task and is the only one interested in its + /// value. + public func waitToFinishPropagatingCancellation() async { + return await resultTask.valuePropagatingCancellation + } + /// A callback that will be called when the task starts executing, is cancelled to be rescheduled, or when it finishes /// execution. - private let executionStateChangedCallback: (@Sendable (TaskExecutionState) async -> Void)? + private let executionStateChangedCallback: (@Sendable (QueuedTask, TaskExecutionState) async -> Void)? init( priority: TaskPriority? = nil, description: TaskDescription, - executionStateChangedCallback: (@Sendable (TaskExecutionState) async -> Void)? + executionStateChangedCallback: (@Sendable (QueuedTask, TaskExecutionState) async -> Void)? ) async { self._priority = .init(initialValue: priority?.rawValue ?? Task.currentPriority.rawValue) self.description = description @@ -214,19 +238,21 @@ fileprivate actor QueuedTask { } executionTask = task executionTaskCreatedContinuation.yield(task) - await executionStateChangedCallback?(.executing) + _isExecuting.value = true + await executionStateChangedCallback?(self, .executing) return await task.value } /// Implementation detail of `execute` that is called after `self.description.execute()` finishes. private func finalizeExecution() async -> ExecutionTaskFinishStatus { self.executionTask = nil + _isExecuting.value = false if Task.isCancelled && self.cancelledToBeRescheduled.value { - await executionStateChangedCallback?(.cancelledToBeRescheduled) + await executionStateChangedCallback?(self, .cancelledToBeRescheduled) self.cancelledToBeRescheduled.value = false return ExecutionTaskFinishStatus.cancelledToBeRescheduled } else { - await executionStateChangedCallback?(.finished) + await executionStateChangedCallback?(self, .finished) return ExecutionTaskFinishStatus.terminated } } @@ -327,8 +353,10 @@ public actor TaskScheduler { public func schedule( priority: TaskPriority? = nil, _ taskDescription: TaskDescription, - @_inheritActorContext executionStateChangedCallback: (@Sendable (TaskExecutionState) async -> Void)? = nil - ) async -> Task { + @_inheritActorContext executionStateChangedCallback: ( + @Sendable (QueuedTask, TaskExecutionState) async -> Void + )? = nil + ) async -> QueuedTask { let queuedTask = await QueuedTask( priority: priority, description: taskDescription, @@ -341,7 +369,7 @@ public actor TaskScheduler { // queued task. await self.poke() } - return queuedTask.resultTask + return queuedTask } /// Trigger all queued tasks to update their priority. diff --git a/Sources/SKSupport/Sequence+AsyncMap.swift b/Sources/SKSupport/Sequence+AsyncMap.swift index e6cb2f360..97d6817db 100644 --- a/Sources/SKSupport/Sequence+AsyncMap.swift +++ b/Sources/SKSupport/Sequence+AsyncMap.swift @@ -39,4 +39,19 @@ extension Sequence { return result } + + /// Just like `Sequence.map` but allows an `async` transform function. + public func asyncFilter( + @_inheritActorContext _ predicate: @Sendable (Element) async throws -> Bool + ) async rethrows -> [Element] { + var result: [Element] = [] + + for element in self { + if try await predicate(element) { + result.append(element) + } + } + + return result + } } diff --git a/Sources/SemanticIndex/CMakeLists.txt b/Sources/SemanticIndex/CMakeLists.txt index 85372d49e..b087edaeb 100644 --- a/Sources/SemanticIndex/CMakeLists.txt +++ b/Sources/SemanticIndex/CMakeLists.txt @@ -2,6 +2,7 @@ add_library(SemanticIndex STATIC CheckedIndex.swift CompilerCommandLineOption.swift + IndexStatusManager.swift IndexTaskDescription.swift PreparationTaskDescription.swift SemanticIndexManager.swift diff --git a/Sources/SemanticIndex/IndexStatusManager.swift b/Sources/SemanticIndex/IndexStatusManager.swift new file mode 100644 index 000000000..016243bf8 --- /dev/null +++ b/Sources/SemanticIndex/IndexStatusManager.swift @@ -0,0 +1,70 @@ +//===----------------------------------------------------------------------===// +// +// This source file is part of the Swift.org open source project +// +// Copyright (c) 2014 - 2024 Apple Inc. and the Swift project authors +// Licensed under Apache License v2.0 with Runtime Library Exception +// +// See https://swift.org/LICENSE.txt for license information +// See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors +// +//===----------------------------------------------------------------------===// + +import Foundation +import SKCore + +/// Keeps track of whether an item (a target or file to index) is up-to-date. +actor IndexUpToDateStatusManager { + private enum Status { + /// The item is up-to-date. + case upToDate + + /// The target or file has been marked out-of-date at the given date. + /// + /// Keeping track of the date is necessary so that we don't mark a target as up-to-date if we have the following + /// ordering of events: + /// - Preparation started + /// - Target marked out of date + /// - Preparation finished + case outOfDate(Date) + } + + private var status: [Item: Status] = [:] + + /// Mark the target or file as up-to-date from a preparation/update-indexstore operation started at + /// `updateOperationStartDate`. + /// + /// See comment on `Status.outOfDate` why `updateOperationStartDate` needs to be passed. + func markUpToDate(_ items: [Item], updateOperationStartDate: Date) { + for item in items { + switch status[item] { + case .upToDate: + break + case .outOfDate(let markedOutOfDate): + if markedOutOfDate < updateOperationStartDate { + status[item] = .upToDate + } + case nil: + status[item] = .upToDate + } + } + } + + func markOutOfDate(_ items: some Collection) { + let date = Date() + for item in items { + status[item] = .outOfDate(date) + } + } + + func markAllOutOfDate() { + markOutOfDate(status.keys) + } + + func isUpToDate(_ item: Item) -> Bool { + if case .upToDate = status[item] { + return true + } + return false + } +} diff --git a/Sources/SemanticIndex/PreparationTaskDescription.swift b/Sources/SemanticIndex/PreparationTaskDescription.swift index 08263d9ea..049ce896c 100644 --- a/Sources/SemanticIndex/PreparationTaskDescription.swift +++ b/Sources/SemanticIndex/PreparationTaskDescription.swift @@ -35,6 +35,8 @@ public struct PreparationTaskDescription: IndexTaskDescription { /// The build system manager that is used to get the toolchain and build settings for the files to index. private let buildSystemManager: BuildSystemManager + private let preparationUpToDateStatus: IndexUpToDateStatusManager + /// Test hooks that should be called when the preparation task finishes. private let testHooks: IndexTestHooks @@ -54,10 +56,12 @@ public struct PreparationTaskDescription: IndexTaskDescription { init( targetsToPrepare: [ConfiguredTarget], buildSystemManager: BuildSystemManager, + preparationUpToDateStatus: IndexUpToDateStatusManager, testHooks: IndexTestHooks ) { self.targetsToPrepare = targetsToPrepare self.buildSystemManager = buildSystemManager + self.preparationUpToDateStatus = preparationUpToDateStatus self.testHooks = testHooks } @@ -66,10 +70,15 @@ public struct PreparationTaskDescription: IndexTaskDescription { // See comment in `withLoggingScope`. // The last 2 digits should be sufficient to differentiate between multiple concurrently running preparation operations await withLoggingScope("preparation-\(id % 100)") { - let startDate = Date() - let targetsToPrepare = targetsToPrepare.sorted(by: { + let targetsToPrepare = await targetsToPrepare.asyncFilter { + await !preparationUpToDateStatus.isUpToDate($0) + }.sorted(by: { ($0.targetID, $0.runDestinationID) < ($1.targetID, $1.runDestinationID) }) + if targetsToPrepare.isEmpty { + return + } + let targetsToPrepareDescription = targetsToPrepare .map { "\($0.targetID)-\($0.runDestinationID)" } @@ -77,6 +86,7 @@ public struct PreparationTaskDescription: IndexTaskDescription { logger.log( "Starting preparation with priority \(Task.currentPriority.rawValue, privacy: .public): \(targetsToPrepareDescription)" ) + let startDate = Date() do { try await buildSystemManager.prepare(targets: targetsToPrepare) } catch { @@ -85,6 +95,9 @@ public struct PreparationTaskDescription: IndexTaskDescription { ) } await testHooks.preparationTaskDidFinish?(self) + if !Task.isCancelled { + await preparationUpToDateStatus.markUpToDate(targetsToPrepare, updateOperationStartDate: startDate) + } logger.log( "Finished preparation in \(Date().timeIntervalSince(startDate) * 1000, privacy: .public)ms: \(targetsToPrepareDescription)" ) diff --git a/Sources/SemanticIndex/SemanticIndexManager.swift b/Sources/SemanticIndex/SemanticIndexManager.swift index 2f304ba70..6ce61adfc 100644 --- a/Sources/SemanticIndex/SemanticIndexManager.swift +++ b/Sources/SemanticIndex/SemanticIndexManager.swift @@ -15,40 +15,44 @@ import LSPLogging import LanguageServerProtocol import SKCore -/// Describes the state of indexing for a single source file -private enum IndexStatus { - /// The index is up-to-date. - case upToDate - /// The file or target is not up to date. We have scheduled a task to update the index store for the file / prepare - /// the target, but that index operation hasn't been started yet. - case scheduled(T) - /// We are currently actively updating the index store for the file / preparing the target, ie. we are running a - /// subprocess that updates the index store / prepares a target. - case executing(T) - - var description: String { - switch self { - case .upToDate: - return "upToDate" - case .scheduled: - return "scheduled" - case .executing: - return "executing" - } +/// A wrapper around `QueuedTask` that only allows equality comparison and inspection whether the `QueuedTask` is +/// currently executing. +/// +/// This way we can store `QueuedTask` in the `inProgress` dictionaries while guaranteeing that whoever created the +/// queued task still has exclusive ownership of the task and can thus control the task's cancellation. +private struct OpaqueQueuedIndexTask: Equatable { + private let task: QueuedTask + + var isExecuting: Bool { + task.isExecuting } -} -/// See `SemanticIndexManager.preparationStatus` -private struct PreparationTaskStatusData { - /// A UUID to track the task. This is used to ensure that status updates from this task don't update - /// `preparationStatus` for targets that are tracked by a different task. - let taskID: UUID + init(_ task: QueuedTask) { + self.task = task + } - /// The list of targets that are being prepared in a joint preparation operation. - let targets: [ConfiguredTarget] + static func == (lhs: OpaqueQueuedIndexTask, rhs: OpaqueQueuedIndexTask) -> Bool { + return lhs.task === rhs.task + } +} - /// The task that prepares the target - let task: Task +private enum InProgressIndexStore { + /// We are waiting for preparation of the file's target to finish before we can index it. + /// + /// `preparationTaskID` identifies the preparation task so that we can transition a file's index state to + /// `updatingIndexStore` when its preparation task has finished. + /// + /// `indexTask` is a task that finishes after both preparation and index store update are done. Whoever owns the index + /// task is still the sole owner of it and responsible for its cancellation. + case waitingForPreparation(preparationTaskID: UUID, indexTask: Task) + + /// The file's target has been prepared and we are updating the file's index store. + /// + /// `updateIndexStoreTask` is the task that updates the index store itself. + /// + /// `indexTask` is a task that finishes after both preparation and index store update are done. Whoever owns the index + /// task is still the sole owner of it and responsible for its cancellation. + case updatingIndexStore(updateIndexStoreTask: OpaqueQueuedIndexTask, indexTask: Task) } /// Schedules index tasks and keeps track of the index status of files. @@ -66,20 +70,21 @@ public final actor SemanticIndexManager { /// ...). `nil` if no build graph is currently being generated. private var generateBuildGraphTask: Task? - /// The preparation status of the targets that the `SemanticIndexManager` has started preparation for. - /// - /// Targets will be removed from this dictionary when they are no longer known to be up-to-date. - private var preparationStatus: [ConfiguredTarget: IndexStatus] = [:] + private let preparationUpToDateStatus = IndexUpToDateStatusManager() + + private let indexStoreUpToDateStatus = IndexUpToDateStatusManager() - /// The index status of the source files that the `SemanticIndexManager` knows about. + /// The preparation tasks that have been started and are either scheduled in the task scheduler or currently + /// executing. /// - /// Files will be removed from this dictionary if their index is no longer up-to-date. + /// After a preparation task finishes, it is removed from this dictionary. + private var inProgressPreparationTasks: [ConfiguredTarget: OpaqueQueuedIndexTask] = [:] + + /// The files that are currently being index, either waiting for their target to be prepared, waiting for the index + /// store update task to be scheduled in the task scheduler or which currently have an index store update running. /// - /// The associated values of the `IndexStatus` are: - /// - A UUID to track the task. This is used to ensure that status updates from this task don't update - /// `preparationStatus` for targets that are tracked by a different task. - /// - The task that prepares the target - private var indexStatus: [DocumentURI: IndexStatus<(UUID, Task)>] = [:] + /// After the file is indexed, it is removed from this dictionary. + private var inProgressIndexTasks: [DocumentURI: InProgressIndexStore] = [:] /// The `TaskScheduler` that manages the scheduling of index tasks. This is shared among all `SemanticIndexManager`s /// in the process, to ensure that we don't schedule more index operations than processor cores from multiple @@ -103,21 +108,30 @@ public final actor SemanticIndexManager { /// The files that still need to be indexed. /// - /// See `FileIndexStatus` for the distinction between `scheduled` and `executing`. - public var inProgressIndexTasks: (scheduled: [DocumentURI], executing: [DocumentURI]) { - let scheduled = indexStatus.compactMap { (uri: DocumentURI, status: IndexStatus) in - if case .scheduled = status { - return uri + /// Scheduled tasks are files that are waiting for their target to be prepared or whose index store update task is + /// waiting to be scheduled by the task scheduler. + /// + /// `executing` are the files that currently have an active index store update task running. + public var inProgressIndexFiles: (scheduled: [DocumentURI], executing: [DocumentURI]) { + var scheduled: [DocumentURI] = [] + var executing: [DocumentURI] = [] + for (uri, status) in inProgressIndexTasks { + let isExecuting: Bool + switch status { + case .waitingForPreparation: + isExecuting = false + case .updatingIndexStore(updateIndexStoreTask: let updateIndexStoreTask, indexTask: _): + isExecuting = updateIndexStoreTask.isExecuting } - return nil - } - let inProgress = indexStatus.compactMap { (uri: DocumentURI, status: IndexStatus) in - if case .executing = status { - return uri + + if isExecuting { + executing.append(uri) + } else { + scheduled.append(uri) } - return nil } - return (scheduled, inProgress) + + return (scheduled, executing) } public init( @@ -176,14 +190,14 @@ public final actor SemanticIndexManager { await generateBuildGraphTask?.value await withTaskGroup(of: Void.self) { taskGroup in - for (_, status) in indexStatus { + for (_, status) in inProgressIndexTasks { switch status { - case .scheduled((_, let task)), .executing((_, let task)): + case .waitingForPreparation(preparationTaskID: _, indexTask: let indexTask), + .updatingIndexStore(updateIndexStoreTask: _, indexTask: let indexTask): taskGroup.addTask { - await task.value + await indexTask.value } - case .upToDate: - break + } } await taskGroup.waitForAll() @@ -216,21 +230,26 @@ public final actor SemanticIndexManager { // 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) - // Reset the index status for these files so they get re-indexed by `index(files:priority:)` - for uri in changedFiles { - indexStatus[uri] = nil - } + await indexStoreUpToDateStatus.markOutOfDate(changedFiles) + // Note that configured targets are the right abstraction layer here (instead of a non-configured target) because a // build system might have targets that include different source files. Hence a source file might be in target T // configured for macOS but not in target T configured for iOS. let targets = await changedFiles.asyncMap { await buildSystemManager.configuredTargets(for: $0) }.flatMap { $0 } if let dependentTargets = await buildSystemManager.targets(dependingOn: targets) { - for dependentTarget in dependentTargets { - preparationStatus[dependentTarget] = nil - } + await preparationUpToDateStatus.markOutOfDate(dependentTargets) } 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) + + await preparationUpToDateStatus.markAllOutOfDate() + // `markAllOutOfDate` only marks targets out-of-date that have been indexed before. Also mark all targets with + // in-progress preparation out of date. So we don't get into the following situation, which would result in an + // incorrect up-to-date status of a target + // - Target preparation starts for the first time + // - Files changed + // - Target preparation finishes. + await preparationUpToDateStatus.markOutOfDate(inProgressPreparationTasks.keys) } await scheduleBackgroundIndex(files: changedFiles) @@ -283,124 +302,83 @@ public final actor SemanticIndexManager { /// Prepare the given targets for indexing private func prepare(targets: [ConfiguredTarget], priority: TaskPriority?) async { - var targetsToPrepare: [ConfiguredTarget] = [] - var preparationTasksToAwait: [Task] = [] - for target in targets { - switch preparationStatus[target] { - case .upToDate: - break - case .scheduled(let existingTaskData), .executing(let existingTaskData): - // If we already have a task scheduled that prepares fewer targets, await that instead of overriding the - // target's preparation status with a longer-running task. The key benefit here is that when we get many - // preparation requests for the same target (eg. one for every text document request sent to a file), we don't - // re-create new `PreparationTaskDescription`s for every preparation request. Instead, all the preparation - // requests await the same task. At the same time, if we have a multi-file preparation request and then get a - // single-file preparation request, we will override the preparation of that target with the single-file - // preparation task, ensuring that the task gets prepared as quickly as possible. - if existingTaskData.targets.count <= targets.count { - preparationTasksToAwait.append(existingTaskData.task) - } else { - targetsToPrepare.append(target) - } - case nil: - targetsToPrepare.append(target) - } + // Perform a quick initial check whether the target is up-to-date, in which case we don't need to schedule a + // preparation operation at all. + // We will check the up-to-date status again in `PreparationTaskDescription.execute`. This ensures that if we + // schedule two preparations of the same target in quick succession, only the first one actually performs a prepare + // and the second one will be a no-op once it runs. + let targetsToPrepare = await targets.asyncFilter { + await !preparationUpToDateStatus.isUpToDate($0) + } + + guard !targetsToPrepare.isEmpty else { + return } let taskDescription = AnyIndexTaskDescription( PreparationTaskDescription( targetsToPrepare: targetsToPrepare, buildSystemManager: self.buildSystemManager, + preparationUpToDateStatus: preparationUpToDateStatus, testHooks: testHooks ) ) - if !targetsToPrepare.isEmpty { - // A UUID that is used to identify the task. This ensures that status updates from this task don't update - // `preparationStatus` for targets that are tracked by a different task, eg. because this task is a multi-target - // preparation task and the target's status is now tracked by a single-file preparation task. - let taskID = UUID() - let preparationTask = await self.indexTaskScheduler.schedule(priority: priority, taskDescription) { newState in - switch newState { - case .executing: - for target in targetsToPrepare { - if case .scheduled(let existingTaskData) = self.preparationStatus[target], existingTaskData.taskID == taskID - { - self.preparationStatus[target] = .executing(existingTaskData) - } - } - case .cancelledToBeRescheduled: - for target in targetsToPrepare { - if case .executing(let existingTaskData) = self.preparationStatus[target], existingTaskData.taskID == taskID - { - self.preparationStatus[target] = .scheduled(existingTaskData) - } - } - case .finished: - for target in targetsToPrepare { - switch self.preparationStatus[target] { - case .executing(let existingTaskData) where existingTaskData.taskID == taskID: - self.preparationStatus[target] = .upToDate - default: - break - } - } - self.indexTaskDidFinish() - } + let preparationTask = await indexTaskScheduler.schedule(priority: priority, taskDescription) { task, newState in + guard case .finished = newState else { + return } for target in targetsToPrepare { - preparationStatus[target] = .scheduled( - PreparationTaskStatusData(taskID: taskID, targets: targetsToPrepare, task: preparationTask) - ) - } - preparationTasksToAwait.append(preparationTask) - } - await withTaskGroup(of: Void.self) { taskGroup in - for task in preparationTasksToAwait { - taskGroup.addTask { - await task.valuePropagatingCancellation + if self.inProgressPreparationTasks[target] == OpaqueQueuedIndexTask(task) { + self.inProgressPreparationTasks[target] = nil } } - await taskGroup.waitForAll() + self.indexTaskDidFinish() + } + for target in targetsToPrepare { + inProgressPreparationTasks[target] = OpaqueQueuedIndexTask(preparationTask) } + return await preparationTask.waitToFinishPropagatingCancellation() } /// Update the index store for the given files, assuming that their targets have already been prepared. - private func updateIndexStore(for filesAndTargets: [FileAndTarget], taskID: UUID, priority: TaskPriority?) async { + private func updateIndexStore( + for filesAndTargets: [FileAndTarget], + preparationTaskID: UUID, + priority: TaskPriority? + ) async { let taskDescription = AnyIndexTaskDescription( UpdateIndexStoreTaskDescription( filesToIndex: filesAndTargets, buildSystemManager: self.buildSystemManager, index: index, + indexStoreUpToDateStatus: indexStoreUpToDateStatus, testHooks: testHooks ) ) - let updateIndexStoreTask = await self.indexTaskScheduler.schedule(priority: priority, taskDescription) { newState in - switch newState { - case .executing: - for fileAndTarget in filesAndTargets { - if case .scheduled((taskID, let task)) = self.indexStatus[fileAndTarget.file.sourceFile] { - self.indexStatus[fileAndTarget.file.sourceFile] = .executing((taskID, task)) - } - } - case .cancelledToBeRescheduled: - for fileAndTarget in filesAndTargets { - if case .executing((taskID, let task)) = self.indexStatus[fileAndTarget.file.sourceFile] { - self.indexStatus[fileAndTarget.file.sourceFile] = .scheduled((taskID, task)) - } - } - case .finished: - for fileAndTarget in filesAndTargets { - switch self.indexStatus[fileAndTarget.file.sourceFile] { - case .executing((taskID, _)): - self.indexStatus[fileAndTarget.file.sourceFile] = .upToDate - default: - break - } + let updateIndexTask = await indexTaskScheduler.schedule(priority: priority, taskDescription) { task, newState in + guard case .finished = newState else { + return + } + for fileAndTarget in filesAndTargets { + if case .updatingIndexStore(OpaqueQueuedIndexTask(task), _) = self.inProgressIndexTasks[ + fileAndTarget.file.sourceFile + ] { + self.inProgressIndexTasks[fileAndTarget.file.sourceFile] = nil } - self.indexTaskDidFinish() + } + self.indexTaskDidFinish() + } + for fileAndTarget in filesAndTargets { + if case .waitingForPreparation(preparationTaskID, let indexTask) = inProgressIndexTasks[ + fileAndTarget.file.sourceFile + ] { + inProgressIndexTasks[fileAndTarget.file.sourceFile] = .updatingIndexStore( + updateIndexStoreTask: OpaqueQueuedIndexTask(updateIndexTask), + indexTask: indexTask + ) } } - await updateIndexStoreTask.value + return await updateIndexTask.waitToFinishPropagatingCancellation() } /// Index the given set of files at the given priority, preparing their targets beforehand, if needed. @@ -410,11 +388,13 @@ public final actor SemanticIndexManager { of files: some Collection, priority: TaskPriority? ) async -> Task { - let outOfDateFiles = await filesToIndex(toCover: files).filter { - if case .upToDate = indexStatus[$0.sourceFile] { - return false - } - return true + // 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 + // schedule two indexing jobs for the same file in quick succession, only the first one actually updates the index + // store and the second one will be a no-op once it runs. + let outOfDateFiles = await filesToIndex(toCover: files).asyncFilter { + return await !indexStoreUpToDateStatus.isUpToDate($0.sourceFile) } // sort files to get deterministic indexing order .sorted(by: { $0.sourceFile.stringValue < $1.sourceFile.stringValue }) @@ -456,7 +436,7 @@ public final actor SemanticIndexManager { // processor count, so we can get parallelism during preparation. // https://github.com/apple/sourcekit-lsp/issues/1262 for targetsBatch in sortedTargets.partition(intoBatchesOfSize: 1) { - let taskID = UUID() + let preparationTaskID = UUID() let indexTask = Task(priority: priority) { // First prepare the targets. await prepare(targets: targetsBatch, priority: priority) @@ -471,7 +451,7 @@ public final actor SemanticIndexManager { taskGroup.addTask { await self.updateIndexStore( for: fileBatch.map { FileAndTarget(file: $0, target: target) }, - taskID: taskID, + preparationTaskID: preparationTaskID, priority: priority ) } @@ -488,7 +468,10 @@ public final actor SemanticIndexManager { // setting it to `.scheduled` because we don't have an `await` call between the creation of `indexTask` and // this loop, so we still have exclusive access to the `SemanticIndexManager` actor and hence `updateIndexStore` // can't execute until we have set all index statuses to `.scheduled`. - indexStatus[file.sourceFile] = .scheduled((taskID, indexTask)) + inProgressIndexTasks[file.sourceFile] = .waitingForPreparation( + preparationTaskID: preparationTaskID, + indexTask: indexTask + ) } indexTasksWereScheduled(filesToIndex.count) } diff --git a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift index 0c0e8ad4c..3863a27ec 100644 --- a/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift +++ b/Sources/SemanticIndex/UpdateIndexStoreTaskDescription.swift @@ -89,6 +89,8 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { /// The build system manager that is used to get the toolchain and build settings for the files to index. private let buildSystemManager: BuildSystemManager + private let indexStoreUpToDateStatus: IndexUpToDateStatusManager + /// A reference to the underlying index store. Used to check if the index is already up-to-date for a file, in which /// case we don't need to index it again. private let index: UncheckedIndex @@ -113,11 +115,13 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { filesToIndex: [FileAndTarget], buildSystemManager: BuildSystemManager, index: UncheckedIndex, + indexStoreUpToDateStatus: IndexUpToDateStatusManager, testHooks: IndexTestHooks ) { self.filesToIndex = filesToIndex self.buildSystemManager = buildSystemManager self.index = index + self.indexStoreUpToDateStatus = indexStoreUpToDateStatus self.testHooks = testHooks } @@ -176,6 +180,10 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { } private func updateIndexStore(forSingleFile file: FileToIndex, in target: ConfiguredTarget) async { + guard await !indexStoreUpToDateStatus.isUpToDate(file.sourceFile) else { + // If we know that the file is up-to-date without having ot hit the index, do that because it's fastest. + return + } guard let sourceFileUrl = file.sourceFile.fileURL else { // The URI is not a file, so there's nothing we can index. return @@ -205,6 +213,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { ) return } + let startDate = Date() switch language { case .swift: do { @@ -233,6 +242,7 @@ public struct UpdateIndexStoreTaskDescription: IndexTaskDescription { "Not updating index store for \(file) because it is a language that is not supported by background indexing" ) } + await indexStoreUpToDateStatus.markUpToDate([file.sourceFile], updateOperationStartDate: startDate) } private func updateIndexStore( diff --git a/Sources/SourceKitLSP/IndexProgressManager.swift b/Sources/SourceKitLSP/IndexProgressManager.swift index fdc745fa3..fb758af73 100644 --- a/Sources/SourceKitLSP/IndexProgressManager.swift +++ b/Sources/SourceKitLSP/IndexProgressManager.swift @@ -73,7 +73,7 @@ actor IndexProgressManager { var scheduled: [DocumentURI] = [] var executing: [DocumentURI] = [] for indexManager in await sourceKitLSPServer.workspaces.compactMap({ $0.semanticIndexManager }) { - let inProgress = await indexManager.inProgressIndexTasks + let inProgress = await indexManager.inProgressIndexFiles scheduled += inProgress.scheduled executing += inProgress.executing } diff --git a/Tests/SKCoreTests/TaskSchedulerTests.swift b/Tests/SKCoreTests/TaskSchedulerTests.swift index 9ca7a1bd0..90f07fe6c 100644 --- a/Tests/SKCoreTests/TaskSchedulerTests.swift +++ b/Tests/SKCoreTests/TaskSchedulerTests.swift @@ -348,7 +348,9 @@ fileprivate extension TaskScheduler { body, dependencies: dependencies ) - return await self.schedule(priority: priority, taskDescription) + return Task(priority: priority) { + await self.schedule(priority: priority, taskDescription).waitToFinishPropagatingCancellation() + } } } diff --git a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift index 7f0a20812..8efb8b041 100644 --- a/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift +++ b/Tests/SourceKitLSPTests/BackgroundIndexingTests.swift @@ -558,8 +558,7 @@ final class BackgroundIndexingTests: XCTestCase { ConfiguredTarget(targetID: "LibB", runDestinationID: "dummy"), ], [ - ConfiguredTarget(targetID: "LibA", runDestinationID: "dummy"), - ConfiguredTarget(targetID: "LibB", runDestinationID: "dummy"), + ConfiguredTarget(targetID: "LibB", runDestinationID: "dummy") ], ]) serverOptions.indexTestHooks = expectedPreparationTracker.testHooks