internal/lsp/source: add an experimental new cache key for packages

We've recently noted that hashing the packages.Config into the cache key
for the type checked package is probably unnecessary, given that all the
other critical inputs into the typechecker are already included
(packageID, parsed files, and deps). Furthermore, when using a gopls
daemon this causes unnecessary cache misses, because the packages.Config
includes things like working directory that no longer matter once other
inputs to type checking have been computed.

Add an experiment flag that removes the packages.Config from the cache
key. An experiment is used to stage this change as comprehensively
testing the cache is ~impossible.

Change-Id: I7ba73daaa71a80ec996decaa9817ec515b5eeb6e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/260737
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Trust: Robert Findley <rfindley@google.com>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>

gopls/doc/settings.md

@@ -273,6 +273,17 @@ This option must be set to a valid duration string, for example `"250ms"`.
 
 
 Default: `0`.
+### **experimentalPackageCacheKey** *bool*
+experimentalPackageCacheKey controls whether to use a coarser cache key
+for package type information to increase cache hits. This setting removes
+the user's environment, build flags, and working directory from the cache
+key, which should be a safe change as all relevant inputs into the type
+checking pass are already hashed into the key. This is temporarily guarded
+by an experiment because caching behavior is subtle and difficult to
+comprehensively test.
+
+
+Default: `false`.
 <!-- END Experimental: DO NOT MANUALLY EDIT THIS SECTION -->
 
 ## Debugging

internal/lsp/cache/check.go

@@ -156,7 +156,8 @@ func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.Parse
 		deps[depHandle.m.pkgPath] = depHandle
 		depKeys = append(depKeys, depHandle.key)
 	}
-	ph.key = checkPackageKey(ctx, ph.m.id, compiledGoFiles, m.config, depKeys, mode)
+	experimentalKey := s.View().Options().ExperimentalPackageCacheKey
+	ph.key = checkPackageKey(ctx, ph.m.id, compiledGoFiles, m.config, depKeys, mode, experimentalKey)
 	return ph, deps, nil
 }
 
@@ -168,10 +169,16 @@ func (s *snapshot) workspaceParseMode(id packageID) source.ParseMode {
 	}
 }
 
-func checkPackageKey(ctx context.Context, id packageID, pghs []*parseGoHandle, cfg *packages.Config, deps []packageHandleKey, mode source.ParseMode) packageHandleKey {
+func checkPackageKey(ctx context.Context, id packageID, pghs []*parseGoHandle, cfg *packages.Config, deps []packageHandleKey, mode source.ParseMode, experimentalKey bool) packageHandleKey {
 	b := bytes.NewBuffer(nil)
 	b.WriteString(string(id))
-	b.WriteString(hashConfig(cfg))
+	if !experimentalKey {
+		// cfg was used to produce the other hashed inputs (package ID, parsed Go
+		// files, and deps). It should not otherwise affect the inputs to the type
+		// checker, so this experiment omits it. This should increase cache hits on
+		// the daemon as cfg contains the environment and working directory.
+		b.WriteString(hashConfig(cfg))
+	}
 	b.WriteByte(byte(mode))
 	for _, dep := range deps {
 		b.WriteString(string(dep))

internal/lsp/source/options.go

@@ -354,6 +354,15 @@ type ExperimentalOptions struct {
 	//
 	// This option must be set to a valid duration string, for example `"250ms"`.
 	ExperimentalDiagnosticsDelay time.Duration
+
+	// ExperimentalPackageCacheKey controls whether to use a coarser cache key
+	// for package type information to increase cache hits. This setting removes
+	// the user's environment, build flags, and working directory from the cache
+	// key, which should be a safe change as all relevant inputs into the type
+	// checking pass are already hashed into the key. This is temporarily guarded
+	// by an experiment because caching behavior is subtle and difficult to
+	// comprehensively test.
+	ExperimentalPackageCacheKey bool
 }
 
 // DebuggingOptions should not affect the logical execution of Gopls, but may
@@ -550,6 +559,7 @@ func (o *Options) AddStaticcheckAnalyzer(a *analysis.Analyzer) {
 func (o *Options) enableAllExperiments() {
 	o.ExperimentalDiagnosticsDelay = 200 * time.Millisecond
 	o.ExperimentalWorkspaceModule = true
+	o.ExperimentalPackageCacheKey = true
 	o.Staticcheck = true
 	o.Gofumpt = true
 	o.SymbolStyle = DynamicSymbols