gopls/internal/lsp/cache: only delete the most relevant mod tidy handle
For workspaces with a lot of modules, deleting every mod tidy handle on
every save is too expensive. Approximate the correct behavior by
deleting only the most relevant mod file. See the comments in the code
for an explanation of why this is an approximation, and why is is
probably acceptable.
This decreases the DiagnoseSave benchmark for google-cloud-go to 550ms
(from 1.8s).
For golang/go#60089
Change-Id: I94bea0b00b13468f73f921db789292cfa2b8d3e9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496595
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index bf22164..e0a8113 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -2083,10 +2083,36 @@
// Invalidate the previous modTidyHandle if any of the files have been
// saved or if any of the metadata has been invalidated.
if invalidateMetadata || fileWasSaved(originalFH, change.fileHandle) {
- // TODO(maybe): Only delete mod handles for
- // which the withoutURI is relevant.
- // Requires reverse-engineering the go command. (!)
- result.modTidyHandles.Clear()
+ // Only invalidate mod tidy results for the most relevant modfile in the
+ // workspace. This is a potentially lossy optimization for workspaces
+ // with many modules (such as google-cloud-go, which has 145 modules as
+ // of writing).
+ //
+ // While it is theoretically possible that a change in workspace module A
+ // could affect the mod-tidiness of workspace module B (if B transitively
+ // requires A), such changes are probably unlikely and not worth the
+ // penalty of re-running go mod tidy for everything. Note that mod tidy
+ // ignores GOWORK, so the two modules would have to be related by a chain
+ // of replace directives.
+ //
+ // We could improve accuracy by inspecting replace directives, using
+ // overlays in go mod tidy, and/or checking for metadata changes from the
+ // on-disk content.
+ //
+ // Note that we iterate the modTidyHandles map here, rather than e.g.
+ // using nearestModFile, because we don't have access to an accurate
+ // FileSource at this point in the snapshot clone.
+ const onlyInvalidateMostRelevant = true
+ if onlyInvalidateMostRelevant {
+ deleteMostRelevantModFile(result.modTidyHandles, uri)
+ } else {
+ result.modTidyHandles.Clear()
+ }
+
+ // TODO(rfindley): should we apply the above heuristic to mod vuln
+ // or mod handles as well?
+ //
+ // TODO(rfindley): no tests fail if I delete the below line.
result.modWhyHandles.Clear()
result.modVulnHandles.Clear()
}
@@ -2277,6 +2303,31 @@
return result, release
}
+// deleteMostRelevantModFile deletes the mod file most likely to be the mod
+// file for the changed URI, if it exists.
+//
+// Specifically, this is the longest mod file path in a directory containing
+// changed. This might not be accurate if there is another mod file closer to
+// changed that happens not to be present in the map, but that's OK: the goal
+// of this function is to guarantee that IF the nearest mod file is present in
+// the map, it is invalidated.
+func deleteMostRelevantModFile(m *persistent.Map, changed span.URI) {
+ var mostRelevant span.URI
+ changedFile := changed.Filename()
+
+ m.Range(func(key, value interface{}) {
+ modURI := key.(span.URI)
+ if len(modURI) > len(mostRelevant) {
+ if source.InDir(filepath.Dir(modURI.Filename()), changedFile) {
+ mostRelevant = modURI
+ }
+ }
+ })
+ if mostRelevant != "" {
+ m.Delete(mostRelevant)
+ }
+}
+
// invalidatedPackageIDs returns all packages invalidated by a change to uri.
// If we haven't seen this URI before, we guess based on files in the same
// directory. This is of course incorrect in build systems where packages are
