gopls/internal/lsp/cache: move purgeFuncBodies into the parse cache
When working in a package we must repeatedly re-build package handles,
which requires parsing with purged func bodies. Although purging func
bodies leads to faster parsing, it is still expensive enough to warrant
caching.
Move the 'purgeFuncBodies' field into the parse cache.
For golang/go#61207
Change-Id: I90575e5b6be7181743e8376c24312115a1029188
Reviewed-on: https://go-review.googlesource.com/c/tools/+/503440
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
diff --git a/gopls/internal/lsp/cache/analysis.go b/gopls/internal/lsp/cache/analysis.go
index 7116009..7efb4ed 100644
--- a/gopls/internal/lsp/cache/analysis.go
+++ b/gopls/internal/lsp/cache/analysis.go
@@ -754,7 +754,7 @@
// as cached ASTs require the global FileSet.
// ast.Object resolution is unfortunately an implied part of the
// go/analysis contract.
- pgf, err := parseGoImpl(ctx, an.fset, fh, source.ParseFull&^source.SkipObjectResolution)
+ pgf, err := parseGoImpl(ctx, an.fset, fh, source.ParseFull&^source.SkipObjectResolution, false)
parsed[i] = pgf
return err
})
diff --git a/gopls/internal/lsp/cache/check.go b/gopls/internal/lsp/cache/check.go
index fa1b2c4..02de896 100644
--- a/gopls/internal/lsp/cache/check.go
+++ b/gopls/internal/lsp/cache/check.go
@@ -22,7 +22,6 @@
"golang.org/x/mod/module"
"golang.org/x/sync/errgroup"
"golang.org/x/tools/go/ast/astutil"
- goplsastutil "golang.org/x/tools/gopls/internal/astutil"
"golang.org/x/tools/gopls/internal/bug"
"golang.org/x/tools/gopls/internal/lsp/filecache"
"golang.org/x/tools/gopls/internal/lsp/protocol"
@@ -611,7 +610,7 @@
for i, fh := range ph.localInputs.compiledGoFiles {
i, fh := i, fh
group.Go(func() error {
- pgf, err := parseGoImpl(ctx, b.fset, fh, parser.SkipObjectResolution)
+ pgf, err := parseGoImpl(ctx, b.fset, fh, parser.SkipObjectResolution, false)
pgfs[i] = pgf
return err
})
@@ -1233,14 +1232,9 @@
bug.Reportf("internal error reading typerefs data: %v", err)
}
- pgfs := make([]*source.ParsedGoFile, len(cgfs))
- for i, fh := range cgfs {
- content, err := fh.Content()
- if err != nil {
- return nil, err
- }
- content = goplsastutil.PurgeFuncBodies(content)
- pgfs[i], _ = ParseGoSrc(ctx, token.NewFileSet(), fh.URI(), content, source.ParseFull&^parser.ParseComments)
+ pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), source.ParseFull&^parser.ParseComments, true, cgfs...)
+ if err != nil {
+ return nil, err
}
data := typerefs.Encode(pgfs, id, imports)
@@ -1495,11 +1489,11 @@
// Collect parsed files from the type check pass, capturing parse errors from
// compiled files.
var err error
- pkg.goFiles, err = b.parseCache.parseFiles(ctx, b.fset, source.ParseFull, inputs.goFiles...)
+ pkg.goFiles, err = b.parseCache.parseFiles(ctx, b.fset, source.ParseFull, false, inputs.goFiles...)
if err != nil {
return nil, err
}
- pkg.compiledGoFiles, err = b.parseCache.parseFiles(ctx, b.fset, source.ParseFull, inputs.compiledGoFiles...)
+ pkg.compiledGoFiles, err = b.parseCache.parseFiles(ctx, b.fset, source.ParseFull, false, inputs.compiledGoFiles...)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/errors.go b/gopls/internal/lsp/cache/errors.go
index f85a73c..3b752b4 100644
--- a/gopls/internal/lsp/cache/errors.go
+++ b/gopls/internal/lsp/cache/errors.go
@@ -518,12 +518,14 @@
// to use in a list of file of a package, for example.
//
// It returns an error if the file could not be read.
+//
+// TODO(rfindley): eliminate this helper.
func parseGoURI(ctx context.Context, fs source.FileSource, uri span.URI, mode parser.Mode) (*source.ParsedGoFile, error) {
fh, err := fs.ReadFile(ctx, uri)
if err != nil {
return nil, err
}
- return parseGoImpl(ctx, token.NewFileSet(), fh, mode)
+ return parseGoImpl(ctx, token.NewFileSet(), fh, mode, false)
}
// parseModURI is a helper to parse the Mod file at the given URI from the file
diff --git a/gopls/internal/lsp/cache/mod_tidy.go b/gopls/internal/lsp/cache/mod_tidy.go
index 3f89db9..a96793b 100644
--- a/gopls/internal/lsp/cache/mod_tidy.go
+++ b/gopls/internal/lsp/cache/mod_tidy.go
@@ -486,7 +486,7 @@
//
// TODO(rfindley): this should key off source.ImportPath.
func parseImports(ctx context.Context, s *snapshot, files []source.FileHandle) (map[string]bool, error) {
- pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), source.ParseHeader, files...)
+ pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), source.ParseHeader, false, files...)
if err != nil { // e.g. context cancellation
return nil, err
}
diff --git a/gopls/internal/lsp/cache/parse.go b/gopls/internal/lsp/cache/parse.go
index fa650ae..7afa5d9 100644
--- a/gopls/internal/lsp/cache/parse.go
+++ b/gopls/internal/lsp/cache/parse.go
@@ -15,6 +15,7 @@
"path/filepath"
"reflect"
+ goplsastutil "golang.org/x/tools/gopls/internal/astutil"
"golang.org/x/tools/gopls/internal/lsp/protocol"
"golang.org/x/tools/gopls/internal/lsp/safetoken"
"golang.org/x/tools/gopls/internal/lsp/source"
@@ -27,7 +28,7 @@
// ParseGo parses the file whose contents are provided by fh, using a cache.
// The resulting tree may have beeen fixed up.
func (s *snapshot) ParseGo(ctx context.Context, fh source.FileHandle, mode parser.Mode) (*source.ParsedGoFile, error) {
- pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), mode, fh)
+ pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), mode, false, fh)
if err != nil {
return nil, err
}
@@ -35,7 +36,7 @@
}
// parseGoImpl parses the Go source file whose content is provided by fh.
-func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mode parser.Mode) (*source.ParsedGoFile, error) {
+func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle, mode parser.Mode, purgeFuncBodies bool) (*source.ParsedGoFile, error) {
ext := filepath.Ext(fh.URI().Filename())
if ext != ".go" && ext != "" { // files generated by cgo have no extension
return nil, fmt.Errorf("cannot parse non-Go file %s", fh.URI())
@@ -48,14 +49,17 @@
if ctx.Err() != nil {
return nil, ctx.Err()
}
- pgf, _ := ParseGoSrc(ctx, fset, fh.URI(), content, mode)
+ pgf, _ := ParseGoSrc(ctx, fset, fh.URI(), content, mode, purgeFuncBodies)
return pgf, nil
}
// ParseGoSrc parses a buffer of Go source, repairing the tree if necessary.
//
// The provided ctx is used only for logging.
-func ParseGoSrc(ctx context.Context, fset *token.FileSet, uri span.URI, src []byte, mode parser.Mode) (res *source.ParsedGoFile, fixes []fixType) {
+func ParseGoSrc(ctx context.Context, fset *token.FileSet, uri span.URI, src []byte, mode parser.Mode, purgeFuncBodies bool) (res *source.ParsedGoFile, fixes []fixType) {
+ if purgeFuncBodies {
+ src = goplsastutil.PurgeFuncBodies(src)
+ }
ctx, done := event.Start(ctx, "cache.ParseGoSrc", tag.File.Of(uri.Filename()))
defer done()
diff --git a/gopls/internal/lsp/cache/parse_cache.go b/gopls/internal/lsp/cache/parse_cache.go
index 7e0298e..e900ecb 100644
--- a/gopls/internal/lsp/cache/parse_cache.go
+++ b/gopls/internal/lsp/cache/parse_cache.go
@@ -126,8 +126,9 @@
// parseKey uniquely identifies a parsed Go file.
type parseKey struct {
- uri span.URI
- mode parser.Mode
+ uri span.URI
+ mode parser.Mode
+ purgeFuncBodies bool
}
type parseCacheEntry struct {
@@ -145,7 +146,7 @@
// The resulting slice has an entry for every given file handle, though some
// entries may be nil if there was an error reading the file (in which case the
// resulting error will be non-nil).
-func (c *parseCache) startParse(mode parser.Mode, fhs ...source.FileHandle) ([]*memoize.Promise, error) {
+func (c *parseCache) startParse(mode parser.Mode, purgeFuncBodies bool, fhs ...source.FileHandle) ([]*memoize.Promise, error) {
c.mu.Lock()
defer c.mu.Unlock()
@@ -173,8 +174,9 @@
data[i] = content
key := parseKey{
- uri: fh.URI(),
- mode: mode,
+ uri: fh.URI(),
+ mode: mode,
+ purgeFuncBodies: purgeFuncBodies,
}
if e, ok := c.m[key]; ok {
@@ -197,7 +199,7 @@
// inside of parseGoSrc without exceeding the allocated space.
base, nextBase := c.allocateSpace(2*len(content) + parsePadding)
- pgf, fixes1 := ParseGoSrc(ctx, fileSetWithBase(base), uri, content, mode)
+ pgf, fixes1 := ParseGoSrc(ctx, fileSetWithBase(base), uri, content, mode, purgeFuncBodies)
file := pgf.Tok
if file.Base()+file.Size()+1 > nextBase {
// The parsed file exceeds its allocated space, likely due to multiple
@@ -209,7 +211,7 @@
// there, as parseGoSrc will repeat them.
actual := file.Base() + file.Size() - base // actual size consumed, after re-parsing
base2, nextBase2 := c.allocateSpace(actual)
- pgf2, fixes2 := ParseGoSrc(ctx, fileSetWithBase(base2), uri, content, mode)
+ pgf2, fixes2 := ParseGoSrc(ctx, fileSetWithBase(base2), uri, content, mode, purgeFuncBodies)
// In golang/go#59097 we observed that this panic condition was hit.
// One bug was found and fixed, but record more information here in
@@ -314,7 +316,7 @@
//
// If parseFiles returns an error, it still returns a slice,
// but with a nil entry for each file that could not be parsed.
-func (c *parseCache) parseFiles(ctx context.Context, fset *token.FileSet, mode parser.Mode, fhs ...source.FileHandle) ([]*source.ParsedGoFile, error) {
+func (c *parseCache) parseFiles(ctx context.Context, fset *token.FileSet, mode parser.Mode, purgeFuncBodies bool, fhs ...source.FileHandle) ([]*source.ParsedGoFile, error) {
pgfs := make([]*source.ParsedGoFile, len(fhs))
// Temporary fall-back for 32-bit systems, where reservedForParsing is too
@@ -324,7 +326,7 @@
if bits.UintSize == 32 {
for i, fh := range fhs {
var err error
- pgfs[i], err = parseGoImpl(ctx, fset, fh, mode)
+ pgfs[i], err = parseGoImpl(ctx, fset, fh, mode, purgeFuncBodies)
if err != nil {
return pgfs, err
}
@@ -332,7 +334,7 @@
return pgfs, nil
}
- promises, firstErr := c.startParse(mode, fhs...)
+ promises, firstErr := c.startParse(mode, purgeFuncBodies, fhs...)
// Await all parsing.
var g errgroup.Group
diff --git a/gopls/internal/lsp/cache/parse_cache_test.go b/gopls/internal/lsp/cache/parse_cache_test.go
index de48777..01c3b3f 100644
--- a/gopls/internal/lsp/cache/parse_cache_test.go
+++ b/gopls/internal/lsp/cache/parse_cache_test.go
@@ -31,12 +31,12 @@
fset := token.NewFileSet()
cache := newParseCache(0)
- pgfs1, err := cache.parseFiles(ctx, fset, source.ParseFull, fh)
+ pgfs1, err := cache.parseFiles(ctx, fset, source.ParseFull, false, fh)
if err != nil {
t.Fatal(err)
}
pgf1 := pgfs1[0]
- pgfs2, err := cache.parseFiles(ctx, fset, source.ParseFull, fh)
+ pgfs2, err := cache.parseFiles(ctx, fset, source.ParseFull, false, fh)
pgf2 := pgfs2[0]
if err != nil {
t.Fatal(err)
@@ -50,7 +50,7 @@
files := []source.FileHandle{fh}
files = append(files, dummyFileHandles(parseCacheMinFiles-1)...)
- pgfs3, err := cache.parseFiles(ctx, fset, source.ParseFull, files...)
+ pgfs3, err := cache.parseFiles(ctx, fset, source.ParseFull, false, files...)
pgf3 := pgfs3[0]
if pgf3 != pgf1 {
t.Errorf("parseFiles(%q, ...): unexpected cache miss", uri)
@@ -65,13 +65,13 @@
// Now overwrite the cache, after which we should get new results.
cache.gcOnce()
files = dummyFileHandles(parseCacheMinFiles)
- _, err = cache.parseFiles(ctx, fset, source.ParseFull, files...)
+ _, err = cache.parseFiles(ctx, fset, source.ParseFull, false, files...)
if err != nil {
t.Fatal(err)
}
// force a GC, which should collect the recently parsed files
cache.gcOnce()
- pgfs4, err := cache.parseFiles(ctx, fset, source.ParseFull, fh)
+ pgfs4, err := cache.parseFiles(ctx, fset, source.ParseFull, false, fh)
if err != nil {
t.Fatal(err)
}
@@ -95,7 +95,7 @@
// Parsing should succeed even though we overflow the padding.
cache := newParseCache(0)
- _, err := cache.parseFiles(context.Background(), token.NewFileSet(), source.ParseFull, files...)
+ _, err := cache.parseFiles(context.Background(), token.NewFileSet(), source.ParseFull, false, files...)
if err != nil {
t.Fatal(err)
}
@@ -115,7 +115,7 @@
// Parsing should succeed even though we overflow the padding.
cache := newParseCache(0)
- _, err := cache.parseFiles(context.Background(), token.NewFileSet(), source.ParseFull, files...)
+ _, err := cache.parseFiles(context.Background(), token.NewFileSet(), source.ParseFull, false, files...)
if err != nil {
t.Fatal(err)
}
@@ -133,19 +133,19 @@
cache := newParseCache(gcDuration)
cache.stop() // we'll manage GC manually, for testing.
- pgfs0, err := cache.parseFiles(ctx, fset, source.ParseFull, fh, fh)
+ pgfs0, err := cache.parseFiles(ctx, fset, source.ParseFull, false, fh, fh)
if err != nil {
t.Fatal(err)
}
files := dummyFileHandles(parseCacheMinFiles)
- _, err = cache.parseFiles(ctx, fset, source.ParseFull, files...)
+ _, err = cache.parseFiles(ctx, fset, source.ParseFull, false, files...)
if err != nil {
t.Fatal(err)
}
// Even after filling up the 'min' files, we get a cache hit for our original file.
- pgfs1, err := cache.parseFiles(ctx, fset, source.ParseFull, fh, fh)
+ pgfs1, err := cache.parseFiles(ctx, fset, source.ParseFull, false, fh, fh)
if err != nil {
t.Fatal(err)
}
@@ -155,14 +155,14 @@
}
// But after GC, we get a cache miss.
- _, err = cache.parseFiles(ctx, fset, source.ParseFull, files...) // mark dummy files as newer
+ _, err = cache.parseFiles(ctx, fset, source.ParseFull, false, files...) // mark dummy files as newer
if err != nil {
t.Fatal(err)
}
time.Sleep(gcDuration)
cache.gcOnce()
- pgfs2, err := cache.parseFiles(ctx, fset, source.ParseFull, fh, fh)
+ pgfs2, err := cache.parseFiles(ctx, fset, source.ParseFull, false, fh, fh)
if err != nil {
t.Fatal(err)
}
@@ -180,7 +180,7 @@
fh := makeFakeFileHandle(uri, []byte("package p\n\nconst _ = \"foo\""))
cache := newParseCache(0)
- pgfs, err := cache.parseFiles(ctx, token.NewFileSet(), source.ParseFull, fh, fh)
+ pgfs, err := cache.parseFiles(ctx, token.NewFileSet(), source.ParseFull, false, fh, fh)
if err != nil {
t.Fatal(err)
}
diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go
index f2471c2..12cbc99 100644
--- a/gopls/internal/lsp/cache/snapshot.go
+++ b/gopls/internal/lsp/cache/snapshot.go
@@ -2432,8 +2432,8 @@
fset := token.NewFileSet()
// Parse headers to compare package names and imports.
- oldHeads, oldErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseHeader, oldFH)
- newHeads, newErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseHeader, newFH)
+ oldHeads, oldErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseHeader, false, oldFH)
+ newHeads, newErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseHeader, false, newFH)
if oldErr != nil || newErr != nil {
// TODO(rfindley): we can get here if newFH does not exist. There is
@@ -2484,8 +2484,8 @@
// Note: if this affects performance we can probably avoid parsing in the
// common case by first scanning the source for potential comments.
if !invalidate {
- origFulls, oldErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseFull, oldFH)
- newFulls, newErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseFull, newFH)
+ origFulls, oldErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseFull, false, oldFH)
+ newFulls, newErr := lockedSnapshot.view.parseCache.parseFiles(ctx, fset, source.ParseFull, false, newFH)
if oldErr == nil && newErr == nil {
invalidate = magicCommentsChanged(origFulls[0].File, newFulls[0].File)
} else {
@@ -2570,7 +2570,7 @@
// For the builtin file only, we need syntactic object resolution
// (since we can't type check).
mode := source.ParseFull &^ source.SkipObjectResolution
- pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), mode, fh)
+ pgfs, err := s.view.parseCache.parseFiles(ctx, token.NewFileSet(), mode, false, fh)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/cache/symbols.go b/gopls/internal/lsp/cache/symbols.go
index 14874f1..466d9dc 100644
--- a/gopls/internal/lsp/cache/symbols.go
+++ b/gopls/internal/lsp/cache/symbols.go
@@ -60,10 +60,8 @@
}
// symbolizeImpl reads and parses a file and extracts symbols from it.
-// It may use a parsed file already present in the cache but
-// otherwise does not populate the cache.
func symbolizeImpl(ctx context.Context, snapshot *snapshot, fh source.FileHandle) ([]source.Symbol, error) {
- pgfs, err := snapshot.view.parseCache.parseFiles(ctx, token.NewFileSet(), source.ParseFull, fh)
+ pgfs, err := snapshot.view.parseCache.parseFiles(ctx, token.NewFileSet(), source.ParseFull, false, fh)
if err != nil {
return nil, err
}
diff --git a/gopls/internal/lsp/source/typerefs/pkgrefs_test.go b/gopls/internal/lsp/source/typerefs/pkgrefs_test.go
index 40e2c88..d752055 100644
--- a/gopls/internal/lsp/source/typerefs/pkgrefs_test.go
+++ b/gopls/internal/lsp/source/typerefs/pkgrefs_test.go
@@ -295,7 +295,7 @@
return nil, err
}
content = astutil.PurgeFuncBodies(content)
- pgf, _ := cache.ParseGoSrc(ctx, token.NewFileSet(), uri, content, source.ParseFull)
+ pgf, _ := cache.ParseGoSrc(ctx, token.NewFileSet(), uri, content, source.ParseFull, false)
return pgf, nil
}
diff --git a/gopls/internal/lsp/source/typerefs/refs_test.go b/gopls/internal/lsp/source/typerefs/refs_test.go
index b83c781..388dced 100644
--- a/gopls/internal/lsp/source/typerefs/refs_test.go
+++ b/gopls/internal/lsp/source/typerefs/refs_test.go
@@ -516,7 +516,7 @@
var pgfs []*source.ParsedGoFile
for i, src := range test.srcs {
uri := span.URI(fmt.Sprintf("file:///%d.go", i))
- pgf, _ := cache.ParseGoSrc(ctx, token.NewFileSet(), uri, []byte(src), source.ParseFull)
+ pgf, _ := cache.ParseGoSrc(ctx, token.NewFileSet(), uri, []byte(src), source.ParseFull, false)
if !test.allowErrs && pgf.ParseErr != nil {
t.Fatalf("ParseGoSrc(...) returned parse errors: %v", pgf.ParseErr)
}
diff --git a/gopls/internal/lsp/source/types_format.go b/gopls/internal/lsp/source/types_format.go
index d6fdfe2..3c37171 100644
--- a/gopls/internal/lsp/source/types_format.go
+++ b/gopls/internal/lsp/source/types_format.go
@@ -289,6 +289,8 @@
return types.TypeString(obj.Type(), qf), nil
}
+ // TODO(rfindley): parsing to produce candidates can be costly; consider
+ // using faster methods.
targetpgf, pos, err := parseFull(ctx, snapshot, srcpkg.FileSet(), obj.Pos())
if err != nil {
return "", err // e.g. ctx cancelled
