Skip to content

Debounce filesBuildSettingsChanged calls #2020

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 28, 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
44 changes: 41 additions & 3 deletions Sources/BuildSystemIntegration/BuildSystemManager.swift
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,22 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
}
}

/// Debounces calls to `delegate.fileBuildSettingsChanged`.
///
/// This helps in the following situation: A build system takes 5s to return build settings for a file and we have 10
/// requests for those build settings coming in that time period. Once we get build settings, we get 10 calls to
/// `resultReceivedAfterTimeout` in `buildSettings(for:in:language:fallbackAfterTimeout:)`, all for the same document.
/// But calling `fileBuildSettingsChanged` once is totally sufficient.
///
/// Force-unwrapped optional because initializing it requires access to `self`.
private var filesBuildSettingsChangedDebouncer: Debouncer<Set<DocumentURI>>! = nil {
didSet {
// Must only be set once
precondition(oldValue == nil)
precondition(filesBuildSettingsChangedDebouncer != nil)
}
}

private var cachedAdjustedSourceKitOptions = RequestCache<TextDocumentSourceKitOptionsRequest>()

private var cachedBuildTargets = Cache<WorkspaceBuildTargetsRequest, [BuildTargetIdentifier: BuildTargetInfo]>()
Expand Down Expand Up @@ -421,6 +437,24 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
}
}

// We don't need a large debounce duration here. It just needs to be big enough to accumulate
// `resultReceivedAfterTimeout` calls for the same document (see comment on `filesBuildSettingsChangedDebouncer`).
// Since they should all come in at the same time, a couple of milliseconds should be sufficient here, an 20ms be
// plenty while still not causing a noticeable delay to the user.
self.filesBuildSettingsChangedDebouncer = Debouncer(
debounceDuration: .milliseconds(20),
combineResults: { $0.union($1) }
) {
[weak self] (filesWithChangedBuildSettings) in
guard let self, let delegate = await self.delegate else {
logger.fault("Not calling fileBuildSettingsChanged because no delegate exists in SwiftPMBuildSystem")
return
}
if !filesWithChangedBuildSettings.isEmpty {
await delegate.fileBuildSettingsChanged(filesWithChangedBuildSettings)
}
}

// TODO: Forward file watch patterns from this initialize request to the client
// (https://github.com/swiftlang/sourcekit-lsp/issues/1671)
initializeResult = Task { () -> InitializeBuildResponse? in
Expand Down Expand Up @@ -579,7 +613,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
self.cachedSourceFilesAndDirectories.clearAll(isolation: self)

await delegate?.buildTargetsChanged(notification.changes)
await delegate?.fileBuildSettingsChanged(Set(watchedFiles.keys))
await filesBuildSettingsChangedDebouncer.scheduleCall(Set(watchedFiles.keys))
}

private func logMessage(notification: BuildServerProtocol.OnBuildLogMessageNotification) async {
Expand Down Expand Up @@ -837,7 +871,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
try await withTimeout(options.buildSettingsTimeoutOrDefault) {
return try await self.buildSettingsFromBuildSystem(for: document, in: target, language: language)
} resultReceivedAfterTimeout: {
await self.delegate?.fileBuildSettingsChanged([document])
await self.filesBuildSettingsChangedDebouncer.scheduleCall([document])
}
} else {
try await self.buildSettingsFromBuildSystem(for: document, in: target, language: language)
Expand Down Expand Up @@ -895,7 +929,7 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
try await withTimeout(options.buildSettingsTimeoutOrDefault) {
await self.canonicalTarget(for: mainFile)
} resultReceivedAfterTimeout: {
await self.delegate?.fileBuildSettingsChanged([document])
await self.filesBuildSettingsChangedDebouncer.scheduleCall([document])
}
}
var languageForFile: Language
Expand Down Expand Up @@ -955,6 +989,10 @@ package actor BuildSystemManager: QueueBasedMessageHandler {
}
// Handle any messages the build system might have sent us while updating.
await messageHandlingQueue.async(metadata: .stateChange) {}.valuePropagatingCancellation

// Ensure that we send out all delegate calls so that everybody is informed about the changes.
await filesBuildSettingsChangedDebouncer.flush()
await filesDependenciesUpdatedDebouncer.flush()
}

/// The root targets of the project have depth of 0 and all target dependencies have a greater depth than the target
Expand Down
17 changes: 13 additions & 4 deletions Sources/SKUtilities/Debouncer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

/// Debounces calls to a function/closure. If multiple calls to the closure are made, it allows aggregating the
/// parameters.
package actor Debouncer<Parameter> {
package actor Debouncer<Parameter: Sendable> {
/// How long to wait for further `scheduleCall` calls before committing to actually calling `makeCall`.
private let debounceDuration: Duration

Expand All @@ -36,7 +36,7 @@ package actor Debouncer<Parameter> {
/// call can be debounced), the task that would commit the call (unless cancelled), the parameter with which this
/// call should be made and the time at which the call should be made. Keeping track of the time ensures that we don't
/// indefinitely debounce if a new `scheduleCall` is made every 0.4s but we debounce for 0.5s.
private var inProgressData: (Parameter, ContinuousClock.Instant, Task<Void, Never>)?
private var inProgressData: (parameter: Parameter, targetDate: ContinuousClock.Instant, task: Task<Void, Never>)?

package init(
debounceDuration: Duration,
Expand Down Expand Up @@ -66,11 +66,20 @@ package actor Debouncer<Parameter> {
} catch {
return
}
inProgressData = nil
await makeCall(parameter)
await self.flush()
}
inProgressData = (parameter, ContinuousClock.now + debounceDuration, task)
}

/// If any debounced calls are in progress, make them now, even if the debounce duration hasn't expired yet.
package func flush() async {
guard let inProgressDataValue = inProgressData else {
return
}
inProgressData = nil
let parameter = inProgressDataValue.parameter
await makeCall(parameter)
}
}

extension Debouncer<Void> {
Expand Down