Skip to content

Commit ae05609

Browse files
findleyrfindleyr
committed
internal/lsp/cache: add an LRU parse cache
As work proceeds on incremental type-checking, two observations have emerged from benchmarking: - Using a global FileSet is impossible, as IImportShallow allocates a large number of new token.Files (in early experiments 75%+ of in-use memory was consumed by the FileSet!) - Several benchmarks regressed with incremental type-checking due to re-parsing package files following a change. Ideally after a single file changes we would be able to re-typecheck packages containing that file after only re-parsing the single file that changed. These observations are in tension: because type-checking requires that parsed ast.Files live in the same token.FileSet as the type-checked package, we cannot naively save the results of parsing and still use a package-scoped FileSet. This CL seeks to address both observations, by introducing a new mechanism for caching parsed files (a parseCache) that parses files in a standalone FileSet offset to avoid collision with other parsed files. This cache exposes a batch API to parse multiple files and return a FileSet describing all of them. Benchmarking indicates that this partially mitigates performance regressions without sacrificing the memory improvement we by avoiding a global cache of parsed files. In this CL the parse cache is not yet integrated with type-checking, but replaces certain call-sites where we previously tried to avoid parsing through the cache. For golang/go#57987 Change-Id: I840cf003db835a40721f086abcc7bf00486b8108 Reviewed-on: https://go-review.googlesource.com/c/tools/+/469858 Reviewed-by: Alan Donovan <[email protected]> TryBot-Result: Gopher Robot <[email protected]> gopls-CI: kokoro <[email protected]> Run-TryBot: Robert Findley <[email protected]>
1 parent de54582 commit ae05609

File tree

14 files changed

+509
-119
lines changed

14 files changed

+509
-119
lines changed

‎gopls/internal/lsp/cache/check.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -677,7 +677,7 @@ func depsErrors(ctx context.Context, m *source.Metadata, meta *metadataGraph, fs
677677
if err != nil {
678678
return nil, err
679679
}
680-
fset := source.SingletonFileSet(pgf.Tok)
680+
fset := source.FileSetFor(pgf.Tok)
681681
// TODO(adonovan): modify Imports() to accept a single token.File (cgf.Tok).
682682
for _, group := range astutil.Imports(fset, pgf.File) {
683683
for _, imp := range group {

‎gopls/internal/lsp/cache/mod_tidy.go‎

Lines changed: 16 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,11 @@ func modTidyDiagnostics(ctx context.Context, snapshot *snapshot, pm *source.Pars
203203
// If -mod=readonly is not set we may have successfully imported
204204
// packages from missing modules. Otherwise they'll be in
205205
// MissingDependencies. Combine both.
206-
for imp := range parseImports(ctx, snapshot, goFiles) {
206+
imps, err := parseImports(ctx, snapshot, goFiles)
207+
if err != nil {
208+
return nil, err
209+
}
210+
for imp := range imps {
207211
if req, ok := missing[imp]; ok {
208212
missingImports[imp] = req
209213
break
@@ -446,19 +450,20 @@ func missingModuleForImport(pgf *source.ParsedGoFile, imp *ast.ImportSpec, req *
446450
//
447451
// (We can't simply use Metadata.Imports because it is based on
448452
// CompiledGoFiles, after cgo processing.)
449-
func parseImports(ctx context.Context, s *snapshot, files []source.FileHandle) map[string]bool {
450-
s.mu.Lock() // peekOrParse requires a locked snapshot (!)
451-
defer s.mu.Unlock()
453+
//
454+
// TODO(rfindley): this should key off source.ImportPath.
455+
func parseImports(ctx context.Context, s *snapshot, files []source.FileHandle) (map[string]bool, error) {
456+
pgfs, _, err := s.parseCache.parseFiles(ctx, source.ParseHeader, files...)
457+
if err != nil { // e.g. context cancellation
458+
return nil, err
459+
}
460+
452461
seen := make(map[string]bool)
453-
for _, file := range files {
454-
f, err := peekOrParse(ctx, s, file, source.ParseHeader)
455-
if err != nil {
456-
continue
457-
}
458-
for _, spec := range f.File.Imports {
462+
for _, pgf := range pgfs {
463+
for _, spec := range pgf.File.Imports {
459464
path, _ := strconv.Unquote(spec.Path.Value)
460465
seen[path] = true
461466
}
462467
}
463-
return seen
468+
return seen, nil
464469
}

‎gopls/internal/lsp/cache/parse.go‎

Lines changed: 11 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,13 @@ import (
2121
"golang.org/x/tools/gopls/internal/lsp/protocol"
2222
"golang.org/x/tools/gopls/internal/lsp/safetoken"
2323
"golang.org/x/tools/gopls/internal/lsp/source"
24+
"golang.org/x/tools/gopls/internal/span"
2425
"golang.org/x/tools/internal/diff"
2526
"golang.org/x/tools/internal/event"
2627
"golang.org/x/tools/internal/event/tag"
2728
"golang.org/x/tools/internal/memoize"
2829
)
2930

30-
// parseKey uniquely identifies a parsed Go file.
31-
type parseKey struct {
32-
file source.FileIdentity
33-
mode source.ParseMode
34-
}
35-
3631
// ParseGo parses the file whose contents are provided by fh, using a cache.
3732
// The resulting tree may have be fixed up.
3833
//
@@ -111,22 +106,6 @@ func (s *snapshot) ParseGo(ctx context.Context, fh source.FileHandle, mode sourc
111106
return res.parsed, res.err
112107
}
113108

114-
// peekParseGoLocked peeks at the cache used by ParseGo but does not
115-
// populate it or wait for other threads to do so. On cache hit, it returns
116-
// the cache result of parseGoImpl; otherwise it returns (nil, nil).
117-
func (s *snapshot) peekParseGoLocked(fh source.FileHandle, mode source.ParseMode) (*source.ParsedGoFile, error) {
118-
entry, hit := s.parsedGoFiles.Get(parseKey{fh.FileIdentity(), mode})
119-
if !hit {
120-
return nil, nil // no-one has requested this file
121-
}
122-
v := entry.(*memoize.Promise).Cached()
123-
if v == nil {
124-
return nil, nil // parsing is still in progress
125-
}
126-
res := v.(parseGoResult)
127-
return res.parsed, res.err
128-
}
129-
130109
// parseGoResult holds the result of a call to parseGoImpl.
131110
type parseGoResult struct {
132111
parsed *source.ParsedGoFile
@@ -146,13 +125,17 @@ func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle,
146125
if err != nil {
147126
return nil, err
148127
}
128+
return parseGoSrc(ctx, fset, fh.URI(), src, mode), nil
129+
}
149130

131+
// parseGoSrc parses a buffer of Go source, repairing the tree if necessary.
132+
func parseGoSrc(ctx context.Context, fset *token.FileSet, uri span.URI, src []byte, mode source.ParseMode) (res *source.ParsedGoFile) {
150133
parserMode := parser.AllErrors | parser.ParseComments
151134
if mode == source.ParseHeader {
152135
parserMode = parser.ImportsOnly | parser.ParseComments
153136
}
154137

155-
file, err := parser.ParseFile(fset, fh.URI().Filename(), src, parserMode)
138+
file, err := parser.ParseFile(fset, uri.Filename(), src, parserMode)
156139
var parseErr scanner.ErrorList
157140
if err != nil {
158141
// We passed a byte slice, so the only possible error is a parse error.
@@ -164,7 +147,7 @@ func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle,
164147
// file.Pos is the location of the package declaration (issue #53202). If there was
165148
// none, we can't find the token.File that ParseFile created, and we
166149
// have no choice but to recreate it.
167-
tok = fset.AddFile(fh.URI().Filename(), -1, len(src))
150+
tok = fset.AddFile(uri.Filename(), -1, len(src))
168151
tok.SetLinesForContent(src)
169152
}
170153

@@ -189,7 +172,7 @@ func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle,
189172
event.Log(ctx, fmt.Sprintf("fixSrc loop - last diff:\n%v", unified), tag.File.Of(tok.Name()))
190173
}
191174

192-
newFile, _ := parser.ParseFile(fset, fh.URI().Filename(), newSrc, parserMode)
175+
newFile, _ := parser.ParseFile(fset, uri.Filename(), newSrc, parserMode)
193176
if newFile != nil {
194177
// Maintain the original parseError so we don't try formatting the doctored file.
195178
file = newFile
@@ -202,15 +185,15 @@ func parseGoImpl(ctx context.Context, fset *token.FileSet, fh source.FileHandle,
202185
}
203186

204187
return &source.ParsedGoFile{
205-
URI: fh.URI(),
188+
URI: uri,
206189
Mode: mode,
207190
Src: src,
208191
Fixed: fixed,
209192
File: file,
210193
Tok: tok,
211-
Mapper: protocol.NewMapper(fh.URI(), src),
194+
Mapper: protocol.NewMapper(uri, src),
212195
ParseErr: parseErr,
213-
}, nil
196+
}
214197
}
215198

216199
// An unexportedFilter removes as much unexported AST from a set of Files as possible.

0 commit comments

Comments
 (0)