From 39d3d492601a90d889fee47076f09e3798734942 Mon Sep 17 00:00:00 2001 From: Rob Findley Date: Mon, 9 Aug 2021 10:39:21 -0400 Subject: [PATCH] internal/lsp/cache: use metadataGraph.Clone in snapshot.clone Rather than updating metadata directly in snapshot.clone, build a set of updates to apply and call metadata.Clone. After this change, metadata is only updated by cloning, so we can eliminate some code that works with mutable metadata. In the next CL we'll only update the metadata if something changed, but this is intentionally left out of this CL to isolate the change. Benchmark (didChange in kubernetes): ~55ms->65ms, because it is now more work to compute uris. For golang/go#45686 Change-Id: I048bed65760b266a209f67111c57fae29bd3e6f0 Reviewed-on: https://go-review.googlesource.com/c/tools/+/340852 gopls-CI: kokoro TryBot-Result: Gopher Robot Run-TryBot: Robert Findley Reviewed-by: Alan Donovan --- internal/lsp/cache/graph.go | 8 ---- internal/lsp/cache/session.go | 2 +- internal/lsp/cache/snapshot.go | 67 ++++++++++++---------------------- 3 files changed, 25 insertions(+), 52 deletions(-) diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go index f3fe077cf5b..36e658b3a86 100644 --- a/internal/lsp/cache/graph.go +++ b/internal/lsp/cache/graph.go @@ -30,14 +30,6 @@ type metadataGraph struct { ids map[span.URI][]PackageID } -func NewMetadataGraph() *metadataGraph { - return &metadataGraph{ - metadata: make(map[PackageID]*KnownMetadata), - importedBy: make(map[PackageID][]PackageID), - ids: make(map[span.URI][]PackageID), - } -} - // Clone creates a new metadataGraph, applying the given updates to the // receiver. func (g *metadataGraph) Clone(updates map[PackageID]*KnownMetadata) *metadataGraph { diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go index cbb58740621..286d8f12c46 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -232,7 +232,7 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, initializeOnce: &sync.Once{}, generation: s.cache.store.Generation(generationName(v, 0)), packages: make(map[packageKey]*packageHandle), - meta: NewMetadataGraph(), + meta: &metadataGraph{}, files: make(map[span.URI]source.VersionedFileHandle), goFiles: newGoFileMap(), symbols: make(map[span.URI]*symbolHandle), diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go index 601ed450a19..369baca8def 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -705,25 +705,9 @@ func (s *snapshot) getModTidyHandle(uri span.URI) *modTidyHandle { func (s *snapshot) getImportedBy(id PackageID) []PackageID { s.mu.Lock() defer s.mu.Unlock() - return s.getImportedByLocked(id) -} - -func (s *snapshot) getImportedByLocked(id PackageID) []PackageID { - // If we haven't rebuilt the import graph since creating the snapshot. - if len(s.meta.importedBy) == 0 { - s.rebuildImportGraph() - } return s.meta.importedBy[id] } -func (s *snapshot) rebuildImportGraph() { - for id, m := range s.meta.metadata { - for _, importID := range m.Deps { - s.meta.importedBy[importID] = append(s.meta.importedBy[importID], id) - } - } -} - func (s *snapshot) addPackageHandle(ph *packageHandle) *packageHandle { s.mu.Lock() defer s.mu.Unlock() @@ -1705,7 +1689,6 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC builtin: s.builtin, initializeOnce: s.initializeOnce, initializedErr: s.initializedErr, - meta: NewMetadataGraph(), packages: make(map[packageKey]*packageHandle, len(s.packages)), actions: make(map[actionKey]*actionHandle, len(s.actions)), files: make(map[span.URI]source.VersionedFileHandle, len(s.files)), @@ -1912,7 +1895,7 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC return } idsToInvalidate[id] = newInvalidateMetadata - for _, rid := range s.getImportedByLocked(id) { + for _, rid := range s.meta.importedBy[id] { addRevDeps(rid, invalidateMetadata) } } @@ -1976,58 +1959,56 @@ func (s *snapshot) clone(ctx, bgCtx context.Context, changes map[span.URI]*fileC addForwardDeps(id) } - // Copy the URI to package ID mappings, skipping only those URIs whose - // metadata will be reloaded in future calls to load. + // Compute which IDs are in the snapshot. + // + // TODO(rfindley): this step shouldn't be necessary, since we compute skipID + // above based on meta.ids. deleteInvalidMetadata := forceReloadMetadata || workspaceModeChanged idsInSnapshot := map[PackageID]bool{} // track all known IDs - for uri, ids := range s.meta.ids { - // Optimization: ids slices are typically numerous, short (<3), - // and rarely modified by this loop, so don't allocate copies - // until necessary. - var resultIDs []PackageID // nil implies equal to ids[:i:i] - for i, id := range ids { + for _, ids := range s.meta.ids { + for _, id := range ids { if skipID[id] || deleteInvalidMetadata && idsToInvalidate[id] { - resultIDs = ids[:i:i] // unshare continue } // The ID is not reachable from any workspace package, so it should // be deleted. if !reachableID[id] { - resultIDs = ids[:i:i] // unshare continue } idsInSnapshot[id] = true - if resultIDs != nil { - resultIDs = append(resultIDs, id) - } - } - if resultIDs == nil { - resultIDs = ids } - result.meta.ids[uri] = resultIDs } // TODO(adonovan): opt: represent PackageID as an index into a process-global // dup-free list of all package names ever seen, then use a bitmap instead of // a hash table for "PackageSet" (e.g. idsInSnapshot). - // Copy the package metadata. We only need to invalidate packages directly - // containing the affected file, and only if it changed in a relevant way. + // Compute which metadata updates are required. We only need to invalidate + // packages directly containing the affected file, and only if it changed in + // a relevant way. + metadataUpdates := make(map[PackageID]*KnownMetadata) for k, v := range s.meta.metadata { if !idsInSnapshot[k] { // Delete metadata for IDs that are no longer reachable from files // in the snapshot. + metadataUpdates[k] = nil continue } invalidateMetadata := idsToInvalidate[k] - // Mark invalidated metadata rather than deleting it outright. - result.meta.metadata[k] = &KnownMetadata{ - Metadata: v.Metadata, - Valid: v.Valid && !invalidateMetadata, - PkgFilesChanged: v.PkgFilesChanged || changedPkgFiles[k], - ShouldLoad: v.ShouldLoad || invalidateMetadata, + valid := v.Valid && !invalidateMetadata + pkgFilesChanged := v.PkgFilesChanged || changedPkgFiles[k] + shouldLoad := v.ShouldLoad || invalidateMetadata + if valid != v.Valid || pkgFilesChanged != v.PkgFilesChanged || shouldLoad != v.ShouldLoad { + // Mark invalidated metadata rather than deleting it outright. + metadataUpdates[k] = &KnownMetadata{ + Metadata: v.Metadata, + Valid: valid, + PkgFilesChanged: pkgFilesChanged, + ShouldLoad: shouldLoad, + } } } + result.meta = s.meta.Clone(metadataUpdates) result.workspacePackages = computeWorkspacePackages(result.meta) // Inherit all of the go.mod-related handles.