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 <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
diff --git a/internal/lsp/cache/graph.go b/internal/lsp/cache/graph.go
index f3fe077..36e658b 100644
--- a/internal/lsp/cache/graph.go
+++ b/internal/lsp/cache/graph.go
@@ -30,14 +30,6 @@
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 cbb5874..286d8f1 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -232,7 +232,7 @@
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 601ed45..369baca 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -705,25 +705,9 @@
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 @@
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 @@
return
}
idsToInvalidate[id] = newInvalidateMetadata
- for _, rid := range s.getImportedByLocked(id) {
+ for _, rid := range s.meta.importedBy[id] {
addRevDeps(rid, invalidateMetadata)
}
}
@@ -1976,58 +1959,56 @@
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.
