Add cache to avoid requesting diagnostics from sourcekitd frequently. #965

joehsieh · 2023-11-09T13:31:37Z

I'm following the way we cached computations in SyntaxTreeManager.
#922 (comment)

joehsieh · 2023-11-09T13:33:33Z

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

+  /// The number of reports to keep
+  ///
+  /// - Note: This has been chosen without scientific measurements.
+  private let cacheSize = 5


i tentatively set the size of cache to 5 because i'm not sure if there is a better magic-number for it.

joehsieh · 2023-11-09T13:35:25Z

Sources/SourceKitLSP/Swift/SwiftLanguageServer.swift

  ) async throws -> RelatedFullDocumentDiagnosticReport {
    let snapshot = try documentManager.latestSnapshot(req.textDocument.uri)
+    if useCache, let report = report(for: snapshot.id) {
+      return report


i was trying to add a test for the code changes, but i didn't find a way to do it.
at least this change doesn't break any existing tests.

ahoppen

Instead of implicitly setting the cached reports in SwiftLanguageServer, I would prefer to have a similar design to SyntaxTreeManager. SyntaxTreeManager is an encapsulated class that has a single function

func diagnosticReport(for snapshot: DocumentSnapshot, buildSettings: SwiftCompileCommand) throws -> RelatedFullDocumentDiagnosticReport

And then the manager class will decide whether it has diagnostics cached already and, if not, sends a request to sourcekitd to compute them.

joehsieh · 2023-11-10T13:05:38Z

ah, right. in this way, we can make the code testable.

joehsieh · 2023-11-13T15:00:27Z

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

+    -> RelatedFullDocumentDiagnosticReport
+}
+
+actor DiagnosticReportManager {


note: I'll add tests for this new actor.

ahoppen · 2023-11-13T19:59:30Z

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

+import LSPLogging
+import LanguageServerProtocol
+
+protocol DiagnosticReportManagerDelegate: AnyObject {


I don’t think we need a delegate here. reportWithSnapshotReceived and fallbackReportWithSnapshotReceived should be implemented in the DiagnosticReportManager. I think it just means that DiagnosticReportManager needs to gain SourceKitD, DocumentManager and clientHasDiagnosticsCodeDescriptionSupport: Bool properties. That way, the logic is isolated and we’re playing less ping-pong between DiagnosticReportManager and SwiftLanguageServer.

i was considering the the way you mentioned above before, but regarding the testability of DiagnosticReportManager, i decided to have a delegate to make the testing easier.
it makes sense to remove the delegate to avoid playing ping-pong (let me update the code for it first), but i was wondering if there is a way to test it.

I think it’s fine to test it by just sending the diagnostic requests followed by a code action request (which I think we already have a test for). That should trigger the path that computes diagnostics and also the one that re-used cached diagnostics.

ah, okie. then, let me make this PR ready for review.

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

ahoppen · 2023-11-21T20:19:16Z

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

+    return try await reportTask.value
+  }
+
+  func resetCache() async {


Why do we ever need to reset the cache? The snapshot ID + build settings should be sufficient that we re-request diagnostic whenever they might have changed. Or am I missing something?

there are two tests will be failed without reseting cache.

Test Case '-[SourceKitLSPTests.SKTests testDependenciesUpdatedSwift]' failed (5.170 seconds). Test Case '-[SourceKitLSPTests.LocalSwiftTests testDiagnosticsReopen]' failed (1.400 seconds).

i think the cache for the file becomes dirty if the file is reopened with changed dependencies or content, so i reset the cache when open, reopen and close document for it.

Ah, I see. That makes sense. But we should not need to reset the entire cache, we should only need to clear the diagnostics for the document that we are opening/closing/reopening.

@ahoppen sorry for the late reply. 🙇 i've updated the code. PTAL. thanks!

No need to apologize., thank you for fixing this issue!

Tests/SourceKitLSPTests/LocalSwiftTests.swift

ahoppen

I’ve got two nitpicks, otherwise looks good to merge for me.

ahoppen · 2024-01-10T19:26:01Z

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

+  ) async throws -> LanguageServerProtocol.RelatedFullDocumentDiagnosticReport {
+    try Task.checkCancellation()
+
+    let keys = self.keys


I don’t think this is necessary. keys should already refer to self.keys everywhere below.

i checked the var keys the implementations is

private nonisolated var keys: sourcekitd_keys { return sourcekitd.keys }

i think the purpose is to ensure the value of keys won't change in func requestReport

the same pattern is happening in other files.
e.g. SwiftLanguageServer.swift, SemanticRefactoring.swift, etc.

Ah, OK. Fair enough.

ahoppen · 2024-01-10T19:28:23Z

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

+    dict[keys.diagnostics]?.forEach { _, diag in
+      if let diag = Diagnostic(
+        diag,
+        in: snapshot,
+        useEducationalNoteAsCode: self.clientHasDiagnosticsCodeDescriptionSupport
+      ) {
+        diagnostics.append(diag)
+      }
+      return true
+    }


I recently introduced map and compactMap on SKDResponseArray, which we could use here

Suggested change

dict[keys.diagnostics]?.forEach { _, diag in

if let diag = Diagnostic(

diag,

in: snapshot,

useEducationalNoteAsCode: self.clientHasDiagnosticsCodeDescriptionSupport

) {

diagnostics.append(diag)

}

return true

}

let diagnostics: [Diagnostic] = dict[keys.diagnostics]?.compactMap({ diag in

Diagnostic(

diag,

in: snapshot,

useEducationalNoteAsCode: self.clientHasDiagnosticsCodeDescriptionSupport

)

}) ?? []

ahoppen · 2024-01-10T19:33:21Z

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift

+    return try await reportTask.value
+  }
+
+  func resetCache() async {


No need to apologize., thank you for fixing this issue!

Move the responsibility of sending requests to get diagnostics from SwiftLanguageServer to DiagnosticReportManager Make buildSettings as part of the key to the cache Add ReportTask and refine the code Reset cache for reportTasks if needed Finetune the wordings and logics Solve conflicts Only remove necessary items from cache Refine the code

ahoppen · 2024-01-11T02:00:20Z

@swift-ci Please test

ahoppen · 2024-01-11T06:49:11Z

@swift-ci Please test Windows

joehsieh requested review from benlangmuir and ahoppen as code owners November 9, 2023 13:31

joehsieh commented Nov 9, 2023

View reviewed changes

ahoppen reviewed Nov 9, 2023

View reviewed changes

joehsieh force-pushed the cache-diagnostics branch from 0927ce1 to d10eef5 Compare November 13, 2023 14:58

joehsieh marked this pull request as draft November 13, 2023 14:59

joehsieh commented Nov 13, 2023

View reviewed changes

ahoppen reviewed Nov 13, 2023

View reviewed changes

joehsieh force-pushed the cache-diagnostics branch from d10eef5 to 9904dcb Compare November 14, 2023 13:31

joehsieh changed the title ~~Add cache for fullDocumentDiagnosticReport~~ Add cache to avoid requesting diagnostics from sourcekitd frequently. Nov 15, 2023

joehsieh marked this pull request as ready for review November 15, 2023 08:42

ahoppen reviewed Nov 15, 2023

View reviewed changes

ahoppen reviewed Nov 21, 2023

View reviewed changes

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift Outdated Show resolved Hide resolved

Sources/SourceKitLSP/Swift/DiagnosticReportManager.swift Outdated Show resolved Hide resolved

ahoppen reviewed Nov 21, 2023

View reviewed changes

joehsieh force-pushed the cache-diagnostics branch from 0181498 to d0e7d81 Compare December 3, 2023 13:15

joehsieh force-pushed the cache-diagnostics branch from d0e7d81 to 38fba69 Compare January 10, 2024 15:02

ahoppen reviewed Jan 10, 2024

View reviewed changes

joehsieh force-pushed the cache-diagnostics branch from 38fba69 to 8dcfb30 Compare January 11, 2024 01:36

ahoppen approved these changes Jan 11, 2024

View reviewed changes

ahoppen merged commit 3e432c4 into swiftlang:main Jan 12, 2024

Add cache to avoid requesting diagnostics from sourcekitd frequently. #965

Add cache to avoid requesting diagnostics from sourcekitd frequently. #965

Conversation

joehsieh commented Nov 9, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

joehsieh commented Nov 10, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joehsieh Dec 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ahoppen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahoppen commented Jan 11, 2024

Uh oh!

ahoppen commented Jan 11, 2024

Uh oh!

Uh oh!

joehsieh Dec 3, 2023 •

edited

Loading