gopls/internal/lsp: avoid duplicate type checking following invalidation
Following a keystroke, it is common to compute both diagnostics and
completion results. For small packages, this sometimes results in
redundant work, but not enough to significantly affect benchmarks.
However, for very large packages where type checking takes >100ms, these
two operations always run in parallel recomputing the same shared state.
This is made clear in the oracle completion benchmark.
Fix this by guarding type checking with a mutex, and slightly delaying
initial diagnostics to yield to other operations (though because
diagnostics will also recompute shared, it doesn't matter too much which
operation acquires the mutex first).
For golang/go#61207
Change-Id: I761aef9c66ebdd54fab8c61605c42d82a8f412cc
Reviewed-on: https://go-review.googlesource.com/c/tools/+/511435
gopls-CI: kokoro <noreply+kokoro@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index 50ce895..1b9d08c 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -323,6 +323,9 @@
//
// Both pre and post may be called concurrently.
func (s *snapshot) forEachPackage(ctx context.Context, ids []PackageID, pre preTypeCheck, post postTypeCheck) error {
+ s.typeCheckMu.Lock()
+ defer s.typeCheckMu.Unlock()
+
ctx, done := event.Start(ctx, "cache.forEachPackage", tag.PackageCount.Of(len(ids)))
defer done()
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index 863e488..167c56d 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -184,6 +184,18 @@
// detect ignored files.
ignoreFilterOnce sync.Once
ignoreFilter *ignoreFilter
+
+ // typeCheckMu guards type checking.
+ //
+ // Only one type checking pass should be running at a given time, for two reasons:
+ // 1. type checking batches are optimized to use all available processors.
+ // Generally speaking, running two type checking batches serially is about
+ // as fast as running them in parallel.
+ // 2. type checking produces cached artifacts that may be re-used by the
+ // next type-checking batch: the shared import graph and the set of
+ // active packages. Running type checking batches in parallel after an
+ // invalidation can cause redundant calculation of this shared state.
+ typeCheckMu sync.Mutex
}
var globalSnapshotID uint64
diff --git a/gopls/internal/lsp/diagnostics.go b/gopls/internal/lsp/diagnostics.go
index 69c9aeb..dbc163a 100644
--- a/gopls/internal/lsp/diagnostics.go
+++ b/gopls/internal/lsp/diagnostics.go
@@ -188,9 +188,27 @@
// file modifications.
//
// The second phase runs after the delay, and does everything.
+ //
+ // We wait a brief delay before the first phase, to allow higher priority
+ // work such as autocompletion to acquire the type checking mutex (though
+ // typically both diagnosing changed files and performing autocompletion
+ // will be doing the same work: recomputing active packages).
+ const minDelay = 20 * time.Millisecond
+ select {
+ case <-time.After(minDelay):
+ case <-ctx.Done():
+ return
+ }
+
s.diagnoseChangedFiles(ctx, snapshot, changedURIs, onDisk)
s.publishDiagnostics(ctx, false, snapshot)
+ if delay < minDelay {
+ delay = 0
+ } else {
+ delay -= minDelay
+ }
+
select {
case <-time.After(delay):
case <-ctx.Done():
