Skip to content

Commit 43b02ea

Browse files
committed
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 <[email protected]> Run-TryBot: Robert Findley <[email protected]> Reviewed-by: Alan Donovan <[email protected]>
1 parent 5919673 commit 43b02ea

File tree

1 file changed

+55
-4
lines changed

1 file changed

+55
-4
lines changed

gopls/internal/lsp/cache/snapshot.go

Lines changed: 55 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -2083,10 +2083,36 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
20832083
// Invalidate the previous modTidyHandle if any of the files have been
20842084
// saved or if any of the metadata has been invalidated.
20852085
if invalidateMetadata || fileWasSaved(originalFH, change.fileHandle) {
2086-
// TODO(maybe): Only delete mod handles for
2087-
// which the withoutURI is relevant.
2088-
// Requires reverse-engineering the go command. (!)
2089-
result.modTidyHandles.Clear()
2086+
// Only invalidate mod tidy results for the most relevant modfile in the
2087+
// workspace. This is a potentially lossy optimization for workspaces
2088+
// with many modules (such as google-cloud-go, which has 145 modules as
2089+
// of writing).
2090+
//
2091+
// While it is theoretically possible that a change in workspace module A
2092+
// could affect the mod-tidiness of workspace module B (if B transitively
2093+
// requires A), such changes are probably unlikely and not worth the
2094+
// penalty of re-running go mod tidy for everything. Note that mod tidy
2095+
// ignores GOWORK, so the two modules would have to be related by a chain
2096+
// of replace directives.
2097+
//
2098+
// We could improve accuracy by inspecting replace directives, using
2099+
// overlays in go mod tidy, and/or checking for metadata changes from the
2100+
// on-disk content.
2101+
//
2102+
// Note that we iterate the modTidyHandles map here, rather than e.g.
2103+
// using nearestModFile, because we don't have access to an accurate
2104+
// FileSource at this point in the snapshot clone.
2105+
const onlyInvalidateMostRelevant = true
2106+
if onlyInvalidateMostRelevant {
2107+
deleteMostRelevantModFile(result.modTidyHandles, uri)
2108+
} else {
2109+
result.modTidyHandles.Clear()
2110+
}
2111+
2112+
// TODO(rfindley): should we apply the above heuristic to mod vuln
2113+
// or mod handles as well?
2114+
//
2115+
// TODO(rfindley): no tests fail if I delete the below line.
20902116
result.modWhyHandles.Clear()
20912117
result.modVulnHandles.Clear()
20922118
}
@@ -2277,6 +2303,31 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
22772303
return result, release
22782304
}
22792305

2306+
// deleteMostRelevantModFile deletes the mod file most likely to be the mod
2307+
// file for the changed URI, if it exists.
2308+
//
2309+
// Specifically, this is the longest mod file path in a directory containing
2310+
// changed. This might not be accurate if there is another mod file closer to
2311+
// changed that happens not to be present in the map, but that's OK: the goal
2312+
// of this function is to guarantee that IF the nearest mod file is present in
2313+
// the map, it is invalidated.
2314+
func deleteMostRelevantModFile(m *persistent.Map, changed span.URI) {
2315+
var mostRelevant span.URI
2316+
changedFile := changed.Filename()
2317+
2318+
m.Range(func(key, value interface{}) {
2319+
modURI := key.(span.URI)
2320+
if len(modURI) > len(mostRelevant) {
2321+
if source.InDir(filepath.Dir(modURI.Filename()), changedFile) {
2322+
mostRelevant = modURI
2323+
}
2324+
}
2325+
})
2326+
if mostRelevant != "" {
2327+
m.Delete(mostRelevant)
2328+
}
2329+
}
2330+
22802331
// invalidatedPackageIDs returns all packages invalidated by a change to uri.
22812332
// If we haven't seen this URI before, we guess based on files in the same
22822333
// directory. This is of course incorrect in build systems where packages are

0 commit comments

Comments
 (0)