diff --git a/internal/lsp/cache/analysis.go b/internal/lsp/cache/analysis.go index a5e34b406d..b277324a0a 100644 --- a/internal/lsp/cache/analysis.go +++ b/internal/lsp/cache/analysis.go @@ -58,6 +58,11 @@ func (s *snapshot) Analyze(ctx context.Context, id string, analyzers []*source.A return results, nil } +type actionKey struct { + pkg packageKey + analyzer *analysis.Analyzer +} + type actionHandleKey source.Hash // An action represents one unit of analysis work: the application of @@ -90,6 +95,20 @@ type packageFactKey struct { } func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.Analyzer) (*actionHandle, error) { + const mode = source.ParseFull + key := actionKey{ + pkg: packageKey{id: id, mode: mode}, + analyzer: a, + } + + s.mu.Lock() + entry, hit := s.actions.Get(key) + s.mu.Unlock() + + if hit { + return entry.(*actionHandle), nil + } + // TODO(adonovan): opt: this block of code sequentially loads a package // (and all its dependencies), then sequentially creates action handles // for the direct dependencies (whose packages have by then been loaded @@ -100,15 +119,6 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A // use of concurrency would lead to an exponential amount of duplicated // work. We should instead use an atomically updated future cache // and a parallel graph traversal. - - // TODO(adonovan): in the code below, follow the structure of - // the other handle-map accessors. - - const mode = source.ParseFull - if act := s.getActionHandle(id, mode, a); act != nil { - return act, nil - } - ph, err := s.buildPackageHandle(ctx, id, mode) if err != nil { return nil, err @@ -157,13 +167,24 @@ func (s *snapshot) actionHandle(ctx context.Context, id PackageID, a *analysis.A return runAnalysis(ctx, snapshot, a, pkg, results) }) - act := &actionHandle{ + ah := &actionHandle{ analyzer: a, pkg: pkg, handle: handle, } - act = s.addActionHandle(act, release) - return act, nil + + s.mu.Lock() + defer s.mu.Unlock() + + // Check cache again in case another thread got there first. + if result, ok := s.actions.Get(key); ok { + release() + return result.(*actionHandle), nil + } + + s.actions.Set(key, ah, func(_, _ interface{}) { release() }) + + return ah, nil } func (act *actionHandle) analyze(ctx context.Context, snapshot *snapshot) ([]*source.Diagnostic, interface{}, error) { diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go index 8ac8b851a2..a59c8908d5 100644 --- a/internal/lsp/cache/cache.go +++ b/internal/lsp/cache/cache.go @@ -199,7 +199,7 @@ func (c *Cache) PackageStats(withNames bool) template.HTML { c.store.DebugOnlyIterate(func(k, v interface{}) { switch k.(type) { case packageHandleKey: - v := v.(*packageData) + v := v.(typeCheckResult) if v.pkg == nil { break } diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go index 366a7afe13..e51f86e5d9 100644 --- a/internal/lsp/cache/check.go +++ b/internal/lsp/cache/check.go @@ -22,6 +22,7 @@ import ( "golang.org/x/tools/go/ast/astutil" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/event" + "golang.org/x/tools/internal/lsp/bug" "golang.org/x/tools/internal/lsp/debug/tag" "golang.org/x/tools/internal/lsp/protocol" "golang.org/x/tools/internal/lsp/source" @@ -43,10 +44,7 @@ type packageHandleKey source.Hash // A packageHandle is a handle to the future result of type-checking a package. // The resulting package is obtained from the check() method. type packageHandle struct { - handle *memoize.Handle - - // mode is the mode the files will be parsed in. - mode source.ParseMode + handle *memoize.Handle // [typeCheckResult] // m is the metadata associated with the package. m *KnownMetadata @@ -61,15 +59,9 @@ type packageHandle struct { key packageHandleKey } -func (ph *packageHandle) packageKey() packageKey { - return packageKey{ - id: ph.m.ID, - mode: ph.mode, - } -} - -// packageData contains the data produced by type-checking a package. -type packageData struct { +// typeCheckResult contains the result of a call to +// typeCheck, which type-checks a package. +type typeCheckResult struct { pkg *pkg err error } @@ -80,15 +72,21 @@ type packageData struct { // attempt to reload missing or invalid metadata. The caller must reload // metadata if needed. func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode source.ParseMode) (*packageHandle, error) { - if ph := s.getPackage(id, mode); ph != nil { - return ph, nil - } + packageKey := packageKey{id: id, mode: mode} + + s.mu.Lock() + entry, hit := s.packages.Get(packageKey) + m := s.meta.metadata[id] + s.mu.Unlock() - m := s.getMetadata(id) if m == nil { return nil, fmt.Errorf("no metadata for %s", id) } + if hit { + return entry.(*packageHandle), nil + } + // Begin computing the key by getting the depKeys for all dependencies. // This requires reading the transitive closure of dependencies' source files. // @@ -140,10 +138,10 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so // All the file reading has now been done. // Create a handle for the result of type checking. experimentalKey := s.View().Options().ExperimentalPackageCacheKey - key := computePackageKey(m.ID, compiledGoFiles, m, depKeys, mode, experimentalKey) + phKey := computePackageKey(m.ID, compiledGoFiles, m, depKeys, mode, experimentalKey) // TODO(adonovan): extract lambda into a standalone function to // avoid implicit lexical dependencies. - handle, release := s.store.Handle(key, func(ctx context.Context, arg interface{}) interface{} { + handle, release := s.store.Handle(phKey, func(ctx context.Context, arg interface{}) interface{} { snapshot := arg.(*snapshot) // Start type checking of direct dependencies, @@ -167,21 +165,43 @@ func (s *snapshot) buildPackageHandle(ctx context.Context, id PackageID, mode so defer wg.Wait() pkg, err := typeCheck(ctx, snapshot, goFiles, compiledGoFiles, m.Metadata, mode, deps) - return &packageData{pkg, err} + return typeCheckResult{pkg, err} }) ph := &packageHandle{ handle: handle, - mode: mode, m: m, - key: key, + key: phKey, } - // Cache the handle in the snapshot. If a package handle has already - // been cached, addPackage will return the cached value. This is fine, - // since the original package handle above will have no references and be - // garbage collected. - return s.addPackageHandle(ph, release) + s.mu.Lock() + defer s.mu.Unlock() + + // Check that the metadata has not changed + // (which should invalidate this handle). + // + // (In future, handles should form a graph with edges from a + // packageHandle to the handles for parsing its files and the + // handles for type-checking its immediate deps, at which + // point there will be no need to even access s.meta.) + if s.meta.metadata[ph.m.ID].Metadata != ph.m.Metadata { + return nil, fmt.Errorf("stale metadata for %s", ph.m.ID) + } + + // Check cache again in case another thread got there first. + if prev, ok := s.packages.Get(packageKey); ok { + prevPH := prev.(*packageHandle) + release() + if prevPH.m.Metadata != ph.m.Metadata { + return nil, bug.Errorf("existing package handle does not match for %s", ph.m.ID) + } + return prevPH, nil + } + + // Update the map. + s.packages.Set(packageKey, ph, func(_, _ interface{}) { release() }) + + return ph, nil } // readGoFiles reads the content of Metadata.GoFiles and @@ -273,7 +293,7 @@ func (ph *packageHandle) check(ctx context.Context, s *snapshot) (*pkg, error) { if err != nil { return nil, err } - data := v.(*packageData) + data := v.(typeCheckResult) return data.pkg, data.err } @@ -290,7 +310,7 @@ func (ph *packageHandle) cached() (*pkg, error) { if v == nil { return nil, fmt.Errorf("no cached type information for %s", ph.m.PkgPath) } - data := v.(*packageData) + data := v.(typeCheckResult) return data.pkg, data.err } diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 1ba945cf0e..9f26b1e59c 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -30,7 +30,6 @@ import ( "golang.org/x/mod/module" "golang.org/x/mod/semver" "golang.org/x/sync/errgroup" - "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/packages" "golang.org/x/tools/internal/event" "golang.org/x/tools/internal/gocommand" @@ -175,11 +174,6 @@ func (s *snapshot) awaitHandle(ctx context.Context, h *memoize.Handle) (interfac return h.Get(ctx, s) } -type actionKey struct { - pkg packageKey - analyzer *analysis.Analyzer -} - // destroy waits for all leases on the snapshot to expire then releases // any resources (reference counts and files) associated with it. // Snapshots being destroyed can be awaited using v.destroyWG. @@ -771,34 +765,6 @@ func (s *snapshot) getImportedBy(id PackageID) []PackageID { return s.meta.importedBy[id] } -// addPackageHandle stores ph in the snapshot, or returns a pre-existing handle -// for the given package key, if it exists. -// -// An error is returned if the metadata used to build ph is no longer relevant. -// -// TODO(adonovan): inline sole use in buildPackageHandle. -func (s *snapshot) addPackageHandle(ph *packageHandle, release func()) (*packageHandle, error) { - s.mu.Lock() - defer s.mu.Unlock() - - if s.meta.metadata[ph.m.ID].Metadata != ph.m.Metadata { - return nil, fmt.Errorf("stale metadata for %s", ph.m.ID) - } - - // If the package handle has already been cached, - // return the cached handle instead of overriding it. - if v, ok := s.packages.Get(ph.packageKey()); ok { - result := v.(*packageHandle) - release() - if result.m.Metadata != ph.m.Metadata { - return nil, bug.Errorf("existing package handle does not match for %s", ph.m.ID) - } - return result, nil - } - s.packages.Set(ph.packageKey(), ph, func(_, _ interface{}) { release() }) - return ph, nil -} - func (s *snapshot) workspacePackageIDs() (ids []PackageID) { s.mu.Lock() defer s.mu.Unlock() @@ -1202,63 +1168,6 @@ func moduleForURI(modFiles map[span.URI]struct{}, uri span.URI) span.URI { return match } -// TODO(adonovan): inline sole use in buildPackageHandle. -func (s *snapshot) getPackage(id PackageID, mode source.ParseMode) *packageHandle { - s.mu.Lock() - defer s.mu.Unlock() - - key := packageKey{ - id: id, - mode: mode, - } - v, ok := s.packages.Get(key) - if !ok { - return nil - } - return v.(*packageHandle) -} - -func (s *snapshot) getActionHandle(id PackageID, m source.ParseMode, a *analysis.Analyzer) *actionHandle { - key := actionKey{ - pkg: packageKey{ - id: id, - mode: m, - }, - analyzer: a, - } - - s.mu.Lock() - defer s.mu.Unlock() - - ah, ok := s.actions.Get(key) - if !ok { - return nil - } - return ah.(*actionHandle) -} - -func (s *snapshot) addActionHandle(ah *actionHandle, release func()) *actionHandle { - key := actionKey{ - analyzer: ah.analyzer, - pkg: packageKey{ - id: ah.pkg.m.ID, - mode: ah.pkg.mode, - }, - } - - s.mu.Lock() - defer s.mu.Unlock() - - // If another thread since cached a different handle, - // return it instead of overriding it. - if result, ok := s.actions.Get(key); ok { - release() - return result.(*actionHandle) - } - s.actions.Set(key, ah, func(_, _ interface{}) { release() }) - return ah -} - func (s *snapshot) getIDsForURI(uri span.URI) []PackageID { s.mu.Lock() defer s.mu.Unlock()