From 21090a2aa8d3719d5a5d4264e41696529522f0bd Mon Sep 17 00:00:00 2001 From: Robert Findley Date: Fri, 1 Sep 2023 13:00:44 -0400 Subject: [PATCH] gopls/internal/lsp/cache: use persistent.Set in a couple places Use the new persistent.Set type to track known dirs and unloadable files. Change-Id: I3e0d4bdc846f4c37a0046a01bf67d83bc06b9598 Reviewed-on: https://go-review.googlesource.com/c/tools/+/524762 LUCI-TryBot-Result: Go LUCI Reviewed-by: Alan Donovan Auto-Submit: Robert Findley --- gopls/internal/lsp/cache/maps.go | 43 ---------------------------- gopls/internal/lsp/cache/session.go | 12 ++++---- gopls/internal/lsp/cache/snapshot.go | 30 +++++++++---------- 3 files changed, 19 insertions(+), 66 deletions(-) diff --git a/gopls/internal/lsp/cache/maps.go b/gopls/internal/lsp/cache/maps.go index 3fa866cb840..edb72d5c123 100644 --- a/gopls/internal/lsp/cache/maps.go +++ b/gopls/internal/lsp/cache/maps.go @@ -76,46 +76,3 @@ func (m filesMap) overlays() []*Overlay { } return overlays } - -type knownDirsSet struct { - impl *persistent.Map[span.URI, struct{}] -} - -func newKnownDirsSet() knownDirsSet { - return knownDirsSet{ - impl: new(persistent.Map[span.URI, struct{}]), - } -} - -func (s knownDirsSet) Clone() knownDirsSet { - return knownDirsSet{ - impl: s.impl.Clone(), - } -} - -func (s knownDirsSet) Destroy() { - s.impl.Destroy() -} - -func (s knownDirsSet) Contains(key span.URI) bool { - _, ok := s.impl.Get(key) - return ok -} - -func (s knownDirsSet) Range(do func(key span.URI)) { - s.impl.Range(func(key span.URI, value struct{}) { - do(key) - }) -} - -func (s knownDirsSet) SetAll(other knownDirsSet) { - s.impl.SetAll(other.impl) -} - -func (s knownDirsSet) Insert(key span.URI) { - s.impl.Set(key, struct{}{}, nil) -} - -func (s knownDirsSet) Remove(key span.URI) { - s.impl.Delete(key) -} diff --git a/gopls/internal/lsp/cache/session.go b/gopls/internal/lsp/cache/session.go index cd51e6d498a..1e463fa3f4f 100644 --- a/gopls/internal/lsp/cache/session.go +++ b/gopls/internal/lsp/cache/session.go @@ -176,13 +176,13 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, activePackages: new(persistent.Map[PackageID, *Package]), symbolizeHandles: new(persistent.Map[span.URI, *memoize.Promise]), workspacePackages: make(map[PackageID]PackagePath), - unloadableFiles: make(map[span.URI]struct{}), + unloadableFiles: new(persistent.Set[span.URI]), parseModHandles: new(persistent.Map[span.URI, *memoize.Promise]), parseWorkHandles: new(persistent.Map[span.URI, *memoize.Promise]), modTidyHandles: new(persistent.Map[span.URI, *memoize.Promise]), modVulnHandles: new(persistent.Map[span.URI, *memoize.Promise]), modWhyHandles: new(persistent.Map[span.URI, *memoize.Promise]), - knownSubdirs: newKnownDirsSet(), + knownSubdirs: new(persistent.Set[span.URI]), workspaceModFiles: wsModFiles, workspaceModFilesErr: wsModFilesErr, pkgIndex: typerefs.NewPackageIndex(), @@ -619,15 +619,15 @@ func (s *Session) ExpandModificationsToDirectories(ctx context.Context, changes // knownDirectories returns all of the directories known to the given // snapshots, including workspace directories and their subdirectories. // It is responsibility of the caller to destroy the returned set. -func knownDirectories(ctx context.Context, snapshots []*snapshot) knownDirsSet { - result := newKnownDirsSet() +func knownDirectories(ctx context.Context, snapshots []*snapshot) *persistent.Set[span.URI] { + result := new(persistent.Set[span.URI]) for _, snapshot := range snapshots { dirs := snapshot.dirs(ctx) for _, dir := range dirs { - result.Insert(dir) + result.Add(dir) } knownSubdirs := snapshot.getKnownSubdirs(dirs) - result.SetAll(knownSubdirs) + result.AddAll(knownSubdirs) knownSubdirs.Destroy() } return result diff --git a/gopls/internal/lsp/cache/snapshot.go b/gopls/internal/lsp/cache/snapshot.go index a914880a4e3..94eceed869b 100644 --- a/gopls/internal/lsp/cache/snapshot.go +++ b/gopls/internal/lsp/cache/snapshot.go @@ -130,7 +130,7 @@ type snapshot struct { shouldLoad map[PackageID][]PackagePath // unloadableFiles keeps track of files that we've failed to load. - unloadableFiles map[span.URI]struct{} + unloadableFiles *persistent.Set[span.URI] // TODO(rfindley): rename the handles below to "promises". A promise is // different from a handle (we mutate the package handle.) @@ -152,7 +152,7 @@ type snapshot struct { // knownSubdirs is the set of subdirectory URIs in the workspace, // used to create glob patterns for file watching. - knownSubdirs knownDirsSet + knownSubdirs *persistent.Set[span.URI] knownSubdirsCache map[string]struct{} // memo of knownSubdirs as a set of filenames // unprocessedSubdirChanges are any changes that might affect the set of // subdirectories in the workspace. They are not reflected to knownSubdirs @@ -269,6 +269,7 @@ func (s *snapshot) destroy(destroyedBy string) { s.modTidyHandles.Destroy() s.modVulnHandles.Destroy() s.modWhyHandles.Destroy() + s.unloadableFiles.Destroy() } func (s *snapshot) SequenceID() uint64 { @@ -750,7 +751,7 @@ func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source } // Check if uri is known to be unloadable. - _, unloadable := s.unloadableFiles[uri] + unloadable := s.unloadableFiles.Contains(uri) s.mu.Unlock() @@ -803,7 +804,7 @@ func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]*source // so if we get here and still have // no IDs, uri is unloadable. if !unloadable && len(ids) == 0 { - s.unloadableFiles[uri] = struct{}{} + s.unloadableFiles.Add(uri) } // Sort packages "narrowest" to "widest" (in practice: @@ -1017,14 +1018,14 @@ func (s *snapshot) collectAllKnownSubdirs(ctx context.Context) { defer s.mu.Unlock() s.knownSubdirs.Destroy() - s.knownSubdirs = newKnownDirsSet() + s.knownSubdirs = new(persistent.Set[span.URI]) s.knownSubdirsCache = nil s.files.Range(func(uri span.URI, fh source.FileHandle) { s.addKnownSubdirLocked(uri, dirs) }) } -func (s *snapshot) getKnownSubdirs(wsDirs []span.URI) knownDirsSet { +func (s *snapshot) getKnownSubdirs(wsDirs []span.URI) *persistent.Set[span.URI] { s.mu.Lock() defer s.mu.Unlock() @@ -1075,7 +1076,7 @@ func (s *snapshot) addKnownSubdirLocked(uri span.URI, dirs []span.URI) { if s.knownSubdirs.Contains(uri) { break } - s.knownSubdirs.Insert(uri) + s.knownSubdirs.Add(uri) dir = filepath.Dir(dir) s.knownSubdirsCache = nil } @@ -1592,7 +1593,7 @@ func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error { s.mu.Lock() loadable := files[:0] for _, file := range files { - if _, unloadable := s.unloadableFiles[file.URI()]; !unloadable { + if !s.unloadableFiles.Contains(file.URI()) { loadable = append(loadable, file) } } @@ -1655,7 +1656,7 @@ func (s *snapshot) reloadOrphanedOpenFiles(ctx context.Context) error { // metadata graph that resulted from loading. uri := file.URI() if len(s.meta.ids[uri]) == 0 { - s.unloadableFiles[uri] = struct{}{} + s.unloadableFiles.Add(uri) } } @@ -1969,7 +1970,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC files: s.files.Clone(), symbolizeHandles: s.symbolizeHandles.Clone(), workspacePackages: make(map[PackageID]PackagePath, len(s.workspacePackages)), - unloadableFiles: make(map[span.URI]struct{}, len(s.unloadableFiles)), + unloadableFiles: s.unloadableFiles.Clone(), // see the TODO for unloadableFiles below parseModHandles: s.parseModHandles.Clone(), parseWorkHandles: s.parseWorkHandles.Clone(), modTidyHandles: s.modTidyHandles.Clone(), @@ -1993,16 +1994,11 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // incref/decref operation that might destroy it prematurely.) release := result.Acquire() - // Copy the set of unloadable files. - // - // TODO(rfindley): this looks wrong. Shouldn't we clear unloadableFiles on + // TODO(rfindley): this looks wrong. Should we clear unloadableFiles on // changes to environment or workspace layout, or more generally on any // metadata change? // // Maybe not, as major configuration changes cause a new view. - for k, v := range s.unloadableFiles { - result.unloadableFiles[k] = v - } // Add all of the known subdirectories, but don't update them for the // changed files. We need to rebuild the workspace module to know the @@ -2119,7 +2115,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC // TODO(rfindley): this also looks wrong, as typing in an unloadable file // will result in repeated reloads. We should only delete if metadata // changed. - delete(result.unloadableFiles, uri) + result.unloadableFiles.Remove(uri) } // Deleting an import can cause list errors due to import cycles to be