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 <[email protected]>
Run-TryBot: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: Hyang-Ah Hana Kim <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/check.go

@@ -323,6 +323,9 @@ type (
 //
 // 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()
 

gopls/internal/lsp/cache/snapshot.go

@@ -184,6 +184,18 @@ type snapshot struct {
 	// 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

gopls/internal/lsp/diagnostics.go

@@ -188,9 +188,27 @@ func (s *Server) diagnoseSnapshot(snapshot source.Snapshot, changedURIs []span.U
 		// 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():