gopls/internal/lsp/cache: reduce importing in analysis

This CL is a substantial reorganization of the analysis driver to
ensure that export data is imported at most once per batch of packages
that are analyzed, instead of once per import edge. This greatly
reduces the amount of allocation and computation done during analysis.

In cache/analysis.go, Snapshot.Analyze (which now takes a set of
PackageIDs, instead of being called singly in a loop) constructs an
ephemeral DAG that mirrors the package graph, and then works in
parallel postorder over this graph doing analysis. It uses a single
FileSet for the whole batch of packages it creates. The subgraph
rooted at each node is effectively a types.Importer for that node,
as it represents the mapping from PackagePath to *types.Package.

We no longer bother with promises or invalidation. We rely on the fact
that the graph is relatively cheap to construct, cache hits are cheap
to process, and the whole process only occurs after an idle delay of
about a second.

Also:

- In internal/facts, optimize the fact decoder by using a callback.
  Previously, it was spending a lot of time traversing the API of all
  imports of a package to build a PackagePath-to-types.Package
  mapping. For many packages in terraform-provider-aws this visits
  over 1M objects (!!). But of course this is trivially computed from
  the new representation.

- In internal/gcimporter, IImportShallow now uses a single callback to
  get all the types.Package symbols from the client, potentially in
  parallel (and that's what gopls does). The previous separation of
  "create" and "populate" has gone away.

  The analysis driver additionally exploits the getPackages callback to
  efficiently read the package manifest of an export data file,
  then abort with an error before proceeding to actually decode
  the rest of the file.

With this change, we can process the internal/provider package of the
terraform-provider-aws repo in 20s cold, 4s hot. (Before, it would run
out of memory.)

$ go test -bench=InitialWorkspaceLoad/hashiform ./gopls/internal/regtest/bench
BenchmarkInitialWorkspaceLoad/hashiform-8         	       1	4014521793 ns/op	 349570384 alloc_bytes	 439230464 in_use_bytes	 668992216 total_alloc_bytes
PASS

Fixes golang/go#60621

Change-Id: Iadeb02f57eb19dcccb639857053b897a60e0a90e
Reviewed-on: https://go-review.googlesource.com/c/tools/+/503195
Reviewed-by: Robert Findley <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Run-TryBot: Alan Donovan <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>

Author: adonovan

gopls/internal/lsp/cache/analysis.go

@@ -531,29 +531,19 @@ func (b *typeCheckBatch) importPackage(ctx context.Context, m *source.Metadata,
 
 	impMap := b.importMap(m.ID)
 
-	var firstErr error // TODO(rfindley): unused: revisit or remove.
 	thisPackage := types.NewPackage(string(m.PkgPath), string(m.Name))
-	getPackage := func(path, name string) *types.Package {
-		if path == string(m.PkgPath) {
-			return thisPackage
-		}
-
-		id := impMap[path]
-		imp, err := b.getImportPackage(ctx, id)
-		if err == nil {
-			return imp
-		}
-		// inv: err != nil
-		if firstErr == nil {
-			firstErr = err
+	getPackages := func(items []gcimporter.GetPackagesItem) error {
+		for i, item := range items {
+			if item.Path == string(m.PkgPath) {
+				items[i].Pkg = thisPackage
+			} else {
+				pkg, err := b.getImportPackage(ctx, impMap[item.Path])
+				if err != nil {
+					return err
+				}
+				items[i].Pkg = pkg
+			}
 		}
-
-		// Context cancellation, or a very bad error such as a file permission
-		// error.
-		//
-		// Returning nil here will cause the import to fail (and panic if
-		// gcimporter.debug is set), but that is preferable to the confusing errors
-		// produced when shallow import encounters an empty package.
 		return nil
 	}
 
@@ -563,9 +553,9 @@ func (b *typeCheckBatch) importPackage(ctx context.Context, m *source.Metadata,
 		return nil, ctx.Err()
 	}
 
-	// TODO(rfindley): collect "deep" hashes here using the provided
+	// TODO(rfindley): collect "deep" hashes here using the getPackages
 	// callback, for precise pruning.
-	imported, err := gcimporter.IImportShallow(b.fset, getPackage, data, string(m.PkgPath), func(*types.Package, string) {})
+	imported, err := gcimporter.IImportShallow(b.fset, getPackages, data, string(m.PkgPath))
 	if err != nil {
 		return nil, fmt.Errorf("import failed for %q: %v", m.ID, err)
 	}

gopls/internal/lsp/cache/check.go

@@ -5,8 +5,6 @@
 package cache
 
 import (
-	"strings"
-
 	"golang.org/x/tools/gopls/internal/lsp/source"
 	"golang.org/x/tools/gopls/internal/span"
 	"golang.org/x/tools/internal/persistent"
@@ -136,13 +134,3 @@ func (s knownDirsSet) Insert(key span.URI) {
 func (s knownDirsSet) Remove(key span.URI) {
 	s.impl.Delete(key)
 }
-
-// analysisKeyLessInterface is the less-than relation for analysisKey
-// values wrapped in an interface.
-func analysisKeyLessInterface(a, b interface{}) bool {
-	x, y := a.(analysisKey), b.(analysisKey)
-	if cmp := strings.Compare(x.analyzerNames, y.analyzerNames); cmp != 0 {
-		return cmp < 0
-	}
-	return x.pkgid < y.pkgid
-}

gopls/internal/lsp/cache/maps.go

@@ -171,7 +171,6 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI,
 		parseCache:           new(parseCache),
 		activePackages:       persistent.NewMap(packageIDLessInterface),
 		symbolizeHandles:     persistent.NewMap(uriLessInterface),
-		analyses:             persistent.NewMap(analysisKeyLessInterface),
 		workspacePackages:    make(map[PackageID]PackagePath),
 		unloadableFiles:      make(map[span.URI]struct{}),
 		parseModHandles:      persistent.NewMap(uriLessInterface),

gopls/internal/lsp/cache/session.go

@@ -122,11 +122,6 @@ type snapshot struct {
 	// IDs not contained in the map are not known to be open or not open.
 	activePackages *persistent.Map // from packageID to *Package
 
-	// analyses maps an analysisKey (which identifies a package
-	// and a set of analyzers) to the handle for the future result
-	// of loading the package and analyzing it.
-	analyses *persistent.Map // from analysisKey to analysisPromise
-
 	// workspacePackages contains the workspace's packages, which are loaded
 	// when the view is created. It contains no intermediate test variants.
 	workspacePackages map[PackageID]PackagePath
@@ -268,7 +263,6 @@ func (s *snapshot) destroy(destroyedBy string) {
 
 	s.packages.Destroy()
 	s.activePackages.Destroy()
-	s.analyses.Destroy()
 	s.files.Destroy()
 	s.knownSubdirs.Destroy()
 	s.symbolizeHandles.Destroy()
@@ -2014,7 +2008,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		initializedErr:       s.initializedErr,
 		packages:             s.packages.Clone(),
 		activePackages:       s.activePackages.Clone(),
-		analyses:             s.analyses.Clone(),
 		files:                s.files.Clone(),
 		parseCache:           s.parseCache,
 		symbolizeHandles:     s.symbolizeHandles.Clone(),
@@ -2242,18 +2235,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC
 		result.activePackages.Delete(id)
 	}
 
-	// Delete invalidated analysis actions.
-	var actionsToDelete []analysisKey
-	result.analyses.Range(func(k, _ interface{}) {
-		key := k.(analysisKey)
-		if _, ok := idsToInvalidate[key.pkgid]; ok {
-			actionsToDelete = append(actionsToDelete, key)
-		}
-	})
-	for _, key := range actionsToDelete {
-		result.analyses.Delete(key)
-	}
-
 	// If a file has been deleted, we must delete metadata for all packages
 	// containing that file.
 	//

gopls/internal/lsp/cache/snapshot.go

@@ -203,7 +203,7 @@ func (s *Server) codeAction(ctx context.Context, params *protocol.CodeActionPara
 		if err != nil {
 			return nil, err
 		}
-		analysisDiags, err := source.Analyze(ctx, snapshot, pkg.Metadata().ID, true)
+		analysisDiags, err := source.Analyze(ctx, snapshot, map[source.PackageID]unit{pkg.Metadata().ID: {}}, true)
 		if err != nil {
 			return nil, err
 		}
@@ -575,3 +575,5 @@ func goTest(ctx context.Context, snapshot source.Snapshot, uri span.URI, rng pro
 		Command: &cmd,
 	}}, nil
 }
+
+type unit = struct{}

gopls/internal/lsp/code_action.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"sort"
 	"strings"
 	"sync"
 	"time"
@@ -362,7 +363,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, analyze
 	var (
 		seen       = map[span.URI]struct{}{}
 		toDiagnose = make(map[source.PackageID]*source.Metadata)
-		toAnalyze  = make(map[source.PackageID]*source.Metadata)
+		toAnalyze  = make(map[source.PackageID]unit)
 	)
 	for _, m := range workspace {
 		var hasNonIgnored, hasOpenFile bool
@@ -378,7 +379,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, analyze
 		if hasNonIgnored {
 			toDiagnose[m.ID] = m
 			if analyze == analyzeEverything || analyze == analyzeOpenPackages && hasOpenFile {
-				toAnalyze[m.ID] = m
+				toAnalyze[m.ID] = unit{}
 			}
 		}
 	}
@@ -416,7 +417,7 @@ func (s *Server) diagnose(ctx context.Context, snapshot source.Snapshot, analyze
 // of concurrent dispatch: as of writing we concurrently run TidyDiagnostics
 // and diagnosePkgs, and diagnosePkgs concurrently runs PackageDiagnostics and
 // analysis.
-func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, toDiagnose, toAnalyze map[source.PackageID]*source.Metadata) {
+func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, toDiagnose map[source.PackageID]*source.Metadata, toAnalyze map[source.PackageID]unit) {
 	ctx, done := event.Start(ctx, "Server.diagnosePkgs", source.SnapshotLabels(snapshot)...)
 	defer done()
 
@@ -425,7 +426,6 @@ func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, toD
 	var (
 		wg            sync.WaitGroup
 		pkgDiags      map[span.URI][]*source.Diagnostic
-		analysisMu    sync.Mutex
 		analysisDiags = make(map[span.URI][]*source.Diagnostic)
 	)
 
@@ -446,28 +446,30 @@ func (s *Server) diagnosePkgs(ctx context.Context, snapshot source.Snapshot, toD
 
 	// Get diagnostics from analysis framework.
 	// This includes type-error analyzers, which suggest fixes to compiler errors.
-	//
-	// TODO(adonovan): in many cases we will be analyze multiple open variants of
-	// an open package, which have significantly overlapping import graphs.
-	// It may make sense to change the Analyze API to accept a slice of IDs, or
-	// merge analysis with type-checking.
-	for _, m := range toAnalyze {
-		m := m
-		wg.Add(1)
-		go func() {
-			defer wg.Done()
-			diags, err := source.Analyze(ctx, snapshot, m.ID, false)
-			if err != nil {
-				event.Error(ctx, "warning: analyzing package", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(string(m.ID)))...)
-				return
-			}
-			analysisMu.Lock()
-			for uri, diags := range diags {
-				analysisDiags[uri] = append(analysisDiags[uri], diags...)
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		diags, err := source.Analyze(ctx, snapshot, toAnalyze, false)
+		if err != nil {
+			var tagStr string // sorted comma-separated list of package IDs
+			{
+				// TODO(adonovan): replace with a generic map[S]any -> string
+				// function in the tag package, and use  maps.Keys + slices.Sort.
+				keys := make([]string, 0, len(toDiagnose))
+				for id := range toDiagnose {
+					keys = append(keys, string(id))
+				}
+				sort.Strings(keys)
+				tagStr = strings.Join(keys, ",")
 			}
-			analysisMu.Unlock()
-		}()
-	}
+			event.Error(ctx, "warning: analyzing package", err, append(source.SnapshotLabels(snapshot), tag.Package.Of(tagStr))...)
+			return
+		}
+		for uri, diags := range diags {
+			analysisDiags[uri] = append(analysisDiags[uri], diags...)
+		}
+	}()
+
 	wg.Wait()
 
 	// TODO(rfindley): remove the guards against snapshot.IsBuiltin, after the

gopls/internal/lsp/diagnostics.go

@@ -21,7 +21,7 @@ type SuggestedFix struct {
 }
 
 // Analyze reports go/analysis-framework diagnostics in the specified package.
-func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeConvenience bool) (map[span.URI][]*Diagnostic, error) {
+func Analyze(ctx context.Context, snapshot Snapshot, pkgIDs map[PackageID]unit, includeConvenience bool) (map[span.URI][]*Diagnostic, error) {
 	// Exit early if the context has been canceled. This also protects us
 	// from a race on Options, see golang/go#36699.
 	if ctx.Err() != nil {
@@ -45,7 +45,7 @@ func Analyze(ctx context.Context, snapshot Snapshot, pkgid PackageID, includeCon
 		}
 	}
 
-	analysisDiagnostics, err := snapshot.Analyze(ctx, pkgid, analyzers)
+	analysisDiagnostics, err := snapshot.Analyze(ctx, pkgIDs, analyzers)
 	if err != nil {
 		return nil, err
 	}
@@ -81,7 +81,7 @@ func FileDiagnostics(ctx context.Context, snapshot Snapshot, uri span.URI) (File
 	if err != nil {
 		return nil, nil, err
 	}
-	adiags, err := Analyze(ctx, snapshot, pkg.Metadata().ID, false)
+	adiags, err := Analyze(ctx, snapshot, map[PackageID]unit{pkg.Metadata().ID: {}}, false)
 	if err != nil {
 		return nil, nil, err
 	}

gopls/internal/lsp/source/diagnostics.go

@@ -21,7 +21,7 @@ package source
 //   - special cases: embedded fields, interfaces, test variants,
 //     function-local things with uppercase names;
 //     packages with type errors (currently 'satisfy' rejects them),
-//     pakage with missing imports;
+//     package with missing imports;
 //
 // - measure performance in k8s.
 //

gopls/internal/lsp/source/rename.go

@@ -92,8 +92,8 @@ type Snapshot interface {
 	// Position information is added to FileSet().
 	ParseGo(ctx context.Context, fh FileHandle, mode parser.Mode) (*ParsedGoFile, error)
 
-	// Analyze runs the specified analyzers on the given package at this snapshot.
-	Analyze(ctx context.Context, id PackageID, analyzers []*Analyzer) ([]*Diagnostic, error)
+	// Analyze runs the specified analyzers on the given packages at this snapshot.
+	Analyze(ctx context.Context, pkgIDs map[PackageID]unit, analyzers []*Analyzer) ([]*Diagnostic, error)
 
 	// RunGoCommandPiped runs the given `go` command, writing its output
 	// to stdout and stderr. Verb, Args, and WorkingDir must be specified.

gopls/internal/lsp/source/view.go

@@ -27,6 +27,7 @@ func BenchmarkInitialWorkspaceLoad(b *testing.B) {
 		{"pkgsite", "internal/frontend/server.go"},
 		{"starlark", "starlark/eval.go"},
 		{"tools", "internal/lsp/cache/snapshot.go"},
+		{"hashiform", "internal/provider/provider.go"},
 	}
 
 	for _, test := range tests {

gopls/internal/regtest/bench/iwl_test.go

@@ -87,6 +87,15 @@ var repos = map[string]*repo{
 		short:  true,
 		inDir:  flag.String("tools_dir", "", "if set, reuse this directory as x/[email protected]"),
 	},
+
+	// A repo of similar size to kubernetes, but with substantially more
+	// complex types that led to a serious performance regression (issue #60621).
+	"hashiform": {
+		name:   "hashiform",
+		url:    "https://github.com/hashicorp/terraform-provider-aws",
+		commit: "ac55de2b1950972d93feaa250d7505d9ed829c7c",
+		inDir:  flag.String("hashiform_dir", "", "if set, reuse this directory as hashiform@ac55de2"),
+	},
 }
 
 // getRepo gets the requested repo, and skips the test if -short is set and

gopls/internal/regtest/bench/repo_test.go

@@ -19,7 +19,7 @@ var (
 	File          = keys.NewString("file", "")
 	Directory     = keys.New("directory", "")
 	URI           = keys.New("URI", "")
-	Package       = keys.NewString("package", "") // Package ID
+	Package       = keys.NewString("package", "") // sorted comma-separated list of Package IDs
 	PackagePath   = keys.NewString("package_path", "")
 	Query         = keys.New("query", "")
 	Snapshot      = keys.NewUInt64("snapshot", "")

internal/event/tag/tag.go

@@ -158,24 +158,52 @@ type gobFact struct {
 // for the same package. Each call to Decode returns an independent
 // fact set.
 type Decoder struct {
-	pkg      *types.Package
-	packages map[string]*types.Package
+	pkg        *types.Package
+	getPackage GetPackageFunc
 }
 
 // NewDecoder returns a fact decoder for the specified package.
+//
+// It uses a brute-force recursive approach to enumerate all objects
+// defined by dependencies of pkg, so that it can learn the set of
+// package paths that may be mentioned in the fact encoding. This does
+// not scale well; use [NewDecoderFunc] where possible.
 func NewDecoder(pkg *types.Package) *Decoder {
 	// Compute the import map for this package.
 	// See the package doc comment.
-	return &Decoder{pkg, importMap(pkg.Imports())}
+	m := importMap(pkg.Imports())
+	getPackageFunc := func(path string) *types.Package { return m[path] }
+	return NewDecoderFunc(pkg, getPackageFunc)
+}
+
+// NewDecoderFunc returns a fact decoder for the specified package.
+//
+// It calls the getPackage function for the package path string of
+// each dependency (perhaps indirect) that it encounters in the
+// encoding. If the function returns nil, the fact is discarded.
+//
+// This function is preferred over [NewDecoder] when the client is
+// capable of efficient look-up of packages by package path.
+func NewDecoderFunc(pkg *types.Package, getPackage GetPackageFunc) *Decoder {
+	return &Decoder{
+		pkg:        pkg,
+		getPackage: getPackage,
+	}
 }
 
-// Decode decodes all the facts relevant to the analysis of package pkg.
-// The read function reads serialized fact data from an external source
-// for one of of pkg's direct imports. The empty file is a valid
-// encoding of an empty fact set.
+// A GetPackageFunc function returns the package denoted by a package path.
+type GetPackageFunc = func(pkgPath string) *types.Package
+
+// Decode decodes all the facts relevant to the analysis of package
+// pkg. The read function reads serialized fact data from an external
+// source for one of pkg's direct imports, identified by package path.
+// The empty file is a valid encoding of an empty fact set.
 //
 // It is the caller's responsibility to call gob.Register on all
 // necessary fact types.
+//
+// Concurrent calls to Decode are safe, so long as the
+// [GetPackageFunc] (if any) is also concurrency-safe.
 func (d *Decoder) Decode(read func(*types.Package) ([]byte, error)) (*Set, error) {
 	// Read facts from imported packages.
 	// Facts may describe indirectly imported packages, or their objects.
@@ -202,13 +230,11 @@ func (d *Decoder) Decode(read func(*types.Package) ([]byte, error)) (*Set, error
 		if err := gob.NewDecoder(bytes.NewReader(data)).Decode(&gobFacts); err != nil {
 			return nil, fmt.Errorf("decoding facts for %q: %v", imp.Path(), err)
 		}
-		if debug {
-			logf("decoded %d facts: %v", len(gobFacts), gobFacts)
-		}
+		logf("decoded %d facts: %v", len(gobFacts), gobFacts)
 
 		// Parse each one into a key and a Fact.
 		for _, f := range gobFacts {
-			factPkg := d.packages[f.PkgPath]
+			factPkg := d.getPackage(f.PkgPath) // possibly an indirect dependency
 			if factPkg == nil {
 				// Fact relates to a dependency that was
 				// unused in this translation unit. Skip.