From f394d451f85a030254df453bf84a450b228c4250 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Tue, 13 Jun 2023 13:50:51 -0400 Subject: [PATCH] gopls/internal/lsp/cache: compute xrefs and methodsets asynchronously No need to wait on xrefs or methodsets while performing latency-sensitive operations on packages. For golang/go#53992 Change-Id: I9ea65269a8c1e604fb99ed8b25e14db79f179576 Reviewed-on: https://go-review.googlesource.com/c/tools/+/503015 gopls-CI: kokoro TryBot-Result: Gopher Robot Run-TryBot: Robert Findley Reviewed-by: Alan Donovan --- gopls/internal/lsp/cache/check.go | 30 +++++++++++++--------------- gopls/internal/lsp/cache/pkg.go | 26 +++++++++++++++++++----- gopls/internal/lsp/cache/snapshot.go | 4 ++-- 3 files changed, 37 insertions(+), 23 deletions(-) diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go index 5f5400b9054..6cfabe3dbcb 100644 --- a/gopls/internal/lsp/cache/check.go +++ b/gopls/internal/lsp/cache/check.go @@ -26,9 +26,7 @@ import ( "golang.org/x/tools/gopls/internal/lsp/filecache" "golang.org/x/tools/gopls/internal/lsp/protocol" "golang.org/x/tools/gopls/internal/lsp/source" - "golang.org/x/tools/gopls/internal/lsp/source/methodsets" "golang.org/x/tools/gopls/internal/lsp/source/typerefs" - "golang.org/x/tools/gopls/internal/lsp/source/xrefs" "golang.org/x/tools/gopls/internal/span" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/event/tag" @@ -44,6 +42,8 @@ const ( preserveImportGraph = true // hold on to the import graph for open packages ) +type unit = struct{} + // A typeCheckBatch holds data for a logical type-checking operation, which may // type-check many unrelated packages. // @@ -56,7 +56,7 @@ type typeCheckBatch struct { handles map[PackageID]*packageHandle parseCache *parseCache fset *token.FileSet // describes all parsed or imported files - cpulimit chan struct{} // concurrency limiter for CPU-bound operations + cpulimit chan unit // concurrency limiter for CPU-bound operations mu sync.Mutex syntaxPackages map[PackageID]*futurePackage // results of processing a requested package; may hold (nil, nil) @@ -69,7 +69,7 @@ type typeCheckBatch struct { // The goroutine that creates the futurePackage is responsible for evaluating // its value, and closing the done channel. type futurePackage struct { - done chan struct{} + done chan unit v pkgOrErr } @@ -154,7 +154,7 @@ func (s *snapshot) getImportGraph(ctx context.Context) *importGraph { // for the work to be done. done := s.importGraphDone if done == nil { - done = make(chan struct{}) + done = make(chan unit) s.importGraphDone = done release := s.Acquire() // must acquire to use the snapshot asynchronously go func() { @@ -360,7 +360,7 @@ func (s *snapshot) forEachPackageInternal(ctx context.Context, importGraph *impo handles: handles, fset: fileSetWithBase(reservedForParsing), syntaxIndex: make(map[PackageID]int), - cpulimit: make(chan struct{}, runtime.GOMAXPROCS(0)), + cpulimit: make(chan unit, runtime.GOMAXPROCS(0)), syntaxPackages: make(map[PackageID]*futurePackage), importPackages: make(map[PackageID]*futurePackage), } @@ -369,7 +369,7 @@ func (s *snapshot) forEachPackageInternal(ctx context.Context, importGraph *impo // Clone the file set every time, to ensure we do not leak files. b.fset = tokeninternal.CloneFileSet(importGraph.fset) // Pre-populate future cache with 'done' futures. - done := make(chan struct{}) + done := make(chan unit) close(done) for id, res := range importGraph.imports { b.importPackages[id] = &futurePackage{done, res} @@ -427,7 +427,7 @@ func (b *typeCheckBatch) getImportPackage(ctx context.Context, id PackageID) (pk } } - f = &futurePackage{done: make(chan struct{})} + f = &futurePackage{done: make(chan unit)} b.importPackages[id] = f b.mu.Unlock() @@ -479,7 +479,7 @@ func (b *typeCheckBatch) handleSyntaxPackage(ctx context.Context, i int, id Pack return f.v.pkg, f.v.err } - f = &futurePackage{done: make(chan struct{})} + f = &futurePackage{done: make(chan unit)} b.syntaxPackages[id] = f b.mu.Unlock() defer func() { @@ -508,7 +508,7 @@ func (b *typeCheckBatch) handleSyntaxPackage(ctx context.Context, i int, id Pack select { case <-ctx.Done(): return nil, ctx.Err() - case b.cpulimit <- struct{}{}: + case b.cpulimit <- unit{}: defer func() { <-b.cpulimit // release CPU token }() @@ -637,8 +637,8 @@ func (b *typeCheckBatch) checkPackage(ctx context.Context, ph *packageHandle) (* // Write package data to disk asynchronously. go func() { toCache := map[string][]byte{ - xrefsKind: pkg.xrefs, - methodSetsKind: pkg.methodsets.Encode(), + xrefsKind: pkg.xrefs(), + methodSetsKind: pkg.methodsets().Encode(), diagnosticsKind: encodeDiagnostics(pkg.diagnostics), } @@ -763,13 +763,13 @@ func (s *snapshot) getPackageHandles(ctx context.Context, ids []PackageID) (map[ // Collect all reachable IDs, and create done channels. // TODO: opt: modify SortPostOrder to make this pre-traversal unnecessary. var allIDs []PackageID - dones := make(map[PackageID]chan struct{}) + dones := make(map[PackageID]chan unit) var walk func(PackageID) walk = func(id PackageID) { if _, ok := dones[id]; ok { return } - dones[id] = make(chan struct{}) + dones[id] = make(chan unit) allIDs = append(allIDs, id) m := meta.metadata[id] for _, depID := range m.DepsByPkgPath { @@ -1262,8 +1262,6 @@ func typeCheckImpl(ctx context.Context, b *typeCheckBatch, inputs typeCheckInput if err != nil { return nil, err } - pkg.methodsets = methodsets.NewIndex(pkg.fset, pkg.types) - pkg.xrefs = xrefs.Index(pkg.compiledGoFiles, pkg.types, pkg.typesInfo) // Our heuristic for whether to show type checking errors is: // + If any file was 'fixed', don't show type checking errors as we diff --git a/gopls/internal/lsp/cache/pkg.go b/gopls/internal/lsp/cache/pkg.go index dbbb4be11f6..32f63495849 100644 --- a/gopls/internal/lsp/cache/pkg.go +++ b/gopls/internal/lsp/cache/pkg.go @@ -11,9 +11,11 @@ import ( "go/scanner" "go/token" "go/types" + "sync" "golang.org/x/tools/gopls/internal/lsp/source" "golang.org/x/tools/gopls/internal/lsp/source/methodsets" + "golang.org/x/tools/gopls/internal/lsp/source/xrefs" "golang.org/x/tools/gopls/internal/span" ) @@ -53,11 +55,25 @@ type syntaxPackage struct { importMap map[PackagePath]*types.Package hasFixedFiles bool // if true, AST was sufficiently mangled that we should hide type errors - // TODO(rfindley): opt: xrefs and methodsets do not need to be pinned to the - // package, and perhaps should be computed asynchronously to package - // diagnostics. - xrefs []byte - methodsets *methodsets.Index + xrefsOnce sync.Once + _xrefs []byte // only used by the xrefs method + + methodsetsOnce sync.Once + _methodsets *methodsets.Index // only used by the methodsets method +} + +func (p *syntaxPackage) xrefs() []byte { + p.xrefsOnce.Do(func() { + p._xrefs = xrefs.Index(p.compiledGoFiles, p.types, p.typesInfo) + }) + return p._xrefs +} + +func (p *syntaxPackage) methodsets() *methodsets.Index { + p.methodsetsOnce.Do(func() { + p._methodsets = methodsets.NewIndex(p.fset, p.types) + }) + return p._methodsets } func (p *Package) String() string { return string(p.ph.m.ID) } diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index bff3dc17f63..6ff14fb6d34 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -688,7 +688,7 @@ func (s *snapshot) References(ctx context.Context, ids ...PackageID) ([]source.X return true } post := func(i int, pkg *Package) { - indexes[i] = XrefIndex{m: pkg.ph.m, data: pkg.pkg.xrefs} + indexes[i] = XrefIndex{m: pkg.ph.m, data: pkg.pkg.xrefs()} } return indexes, s.forEachPackage(ctx, ids, pre, post) } @@ -719,7 +719,7 @@ func (s *snapshot) MethodSets(ctx context.Context, ids ...PackageID) ([]*methods return true } post := func(i int, pkg *Package) { - indexes[i] = pkg.pkg.methodsets + indexes[i] = pkg.pkg.methodsets() } return indexes, s.forEachPackage(ctx, ids, pre, post) }