-
Notifications
You must be signed in to change notification settings - Fork 319
[DocC Live Preview] Cache on-disk snapshots opened in sourcekitd #2226
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
base: main
Are you sure you want to change the base?
[DocC Live Preview] Cache on-disk snapshots opened in sourcekitd #2226
Conversation
@swift-ci please test |
@swift-ci please test |
@swift-ci please test windows |
} | ||
|
||
let documentsToClose = openSnapshots.values | ||
Task { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Launching a task here means that the documents could be closed in parallel while another request is handled, which would be very hard to debug if this ends up creating some kind of data races in sourcekitd. I think I’d prefer a pattern where you have something like the following and then explicitly close the documents at end of the cache’s lifetime.
func withSymbolGraphCache<T>(_ body: (SymbolGraphCache) async throws -> T) async throws -> T
I recently wrote the following for another project, which might be helpful for such a with*
implementation (or generally something that I think might be useful to have in SwiftExtensions
).
/// Run `body` and always ensure that `cleanup` gets run, independently of whether `body` threw an error or returned a
/// value.
package func run<T>(
_ body: () async throws -> T,
cleanup: () async -> ()
) async throws -> T {
do {
let result = try await body()
await cleanup()
return result
} catch {
await cleanup()
throw error
}
}
/// - Parameter symbolLocation: The location of a symbol to find the symbol graph for. | ||
/// - Returns: The symbol graph for this location, if any. | ||
func fetchSymbolGraph(at symbolLocation: SymbolLocation) async throws -> String? { | ||
let swiftLanguageService = try await swiftLanguageService(for: symbolLocation.documentUri) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we’re never re-using an open document, are we? I don’t even think that we ever register open documents.
/// Used by `textDocument/doccDocumentation` requests to retrieve symbol graphs for files that are not currently | ||
/// open in the editor. This allows for retrieving multiple symbol graphs from the same file without having | ||
/// to re-open and parse the syntax tree every time. | ||
actor SymbolGraphCache: Sendable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would something like OnDiskDocumentManager
be a more appropriate name because this type neither really caches values (it manages document lifetimes), nor does it really cache symbol graphs but it keeps track of sourcekitd documents.
Removed
SwiftLanguageService.withSnapshotFromDiskOpenedInSourcekitd()
in favor of a newSymbolGraphCache
actor that will cache all snapshots opened for the duration of atextDocument/doccDocumentation
request. This will prevent the syntax tree from being parsed multiple times for the same document in a single request.Follow up to #2216