internal/lsp/cache: minor simplifications to Symbols
Minor cleanups based on studying the code in preparation
for saving a persistent index:
- Remove unused error result from Symbols method.
- Remove unnecessary fields from symbolHandle.
- Add various explanatory comments.
- In workspace_symbols.go:
- separate extract and match phases of collectSymbols clearly
- replace symbolCollector and matchWorker types by simple parameters
- combine loops (roots, buildMatcher)
- move buildMatcher creation down to where it is needed.
Change-Id: Ifcad61a9a8c7d70f573024bcfa76d476552ee428
Reviewed-on: https://go-review.googlesource.com/c/tools/+/412822
Reviewed-by: Robert Findley <rfindley@google.com>
diff --git a/internal/lsp/cache/cache.go b/internal/lsp/cache/cache.go
index 2a8a169..3640272 100644
--- a/internal/lsp/cache/cache.go
+++ b/internal/lsp/cache/cache.go
@@ -68,6 +68,7 @@
return true
}
+// GetFile stats and (maybe) reads the file, updates the cache, and returns it.
func (c *Cache) GetFile(ctx context.Context, uri span.URI) (source.FileHandle, error) {
return c.getFile(ctx, uri)
}
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index 9875ae4..3268173 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -997,9 +997,7 @@
}
// Symbols extracts and returns the symbols for each file in all the snapshot's views.
-func (s *snapshot) Symbols(ctx context.Context) (map[span.URI][]source.Symbol, error) {
- // Keep going on errors, but log the first failure.
- // Partial results are better than no symbol results.
+func (s *snapshot) Symbols(ctx context.Context) map[span.URI][]source.Symbol {
var (
group errgroup.Group
nprocs = 2 * runtime.GOMAXPROCS(-1) // symbolize is a mix of I/O and CPU
@@ -1023,10 +1021,12 @@
return nil
})
}
+ // Keep going on errors, but log the first failure.
+ // Partial results are better than no symbol results.
if err := group.Wait(); err != nil {
event.Error(ctx, "getting snapshot symbols", err)
}
- return result, nil
+ return result
}
func (s *snapshot) MetadataForFile(ctx context.Context, uri span.URI) ([]source.Metadata, error) {
@@ -1137,11 +1137,10 @@
return s.symbols[uri]
}
-func (s *snapshot) addSymbolHandle(sh *symbolHandle) *symbolHandle {
+func (s *snapshot) addSymbolHandle(uri span.URI, sh *symbolHandle) *symbolHandle {
s.mu.Lock()
defer s.mu.Unlock()
- uri := sh.fh.URI()
// If the package handle has already been cached,
// return the cached handle instead of overriding it.
if sh, ok := s.symbols[uri]; ok {
@@ -1338,7 +1337,7 @@
return fh, nil
}
- fh, err := s.view.session.cache.getFile(ctx, f.URI())
+ fh, err := s.view.session.cache.getFile(ctx, f.URI()) // read the file
if err != nil {
return nil, err
}
diff --git a/internal/lsp/cache/symbols.go b/internal/lsp/cache/symbols.go
index bf5e00b..d56a036 100644
--- a/internal/lsp/cache/symbols.go
+++ b/internal/lsp/cache/symbols.go
@@ -18,13 +18,9 @@
"golang.org/x/tools/internal/memoize"
)
+// A symbolHandle contains a handle to the result of symbolizing a file.
type symbolHandle struct {
handle *memoize.Handle
-
- fh source.FileHandle
-
- // key is the hashed key for the package.
- key symbolHandleKey
}
// symbolData contains the data produced by extracting symbols from a file.
@@ -33,30 +29,30 @@
err error
}
-type symbolHandleKey source.Hash
-
+// buildSymbolHandle returns a handle to the result of symbolizing a file,
+// if necessary creating it and saving it in the snapshot.
func (s *snapshot) buildSymbolHandle(ctx context.Context, fh source.FileHandle) *symbolHandle {
if h := s.getSymbolHandle(fh.URI()); h != nil {
return h
}
+ type symbolHandleKey source.Hash
key := symbolHandleKey(fh.FileIdentity().Hash)
- h := s.generation.Bind(key, func(_ context.Context, arg memoize.Arg) interface{} {
+ handle := s.generation.Bind(key, func(_ context.Context, arg memoize.Arg) interface{} {
snapshot := arg.(*snapshot)
- data := &symbolData{}
- data.symbols, data.err = symbolize(snapshot, fh)
- return data
+ symbols, err := symbolize(snapshot, fh)
+ return &symbolData{symbols, err}
}, nil)
sh := &symbolHandle{
- handle: h,
- fh: fh,
- key: key,
+ handle: handle,
}
- return s.addSymbolHandle(sh)
+
+ return s.addSymbolHandle(fh.URI(), sh)
}
-// symbolize extracts symbols from a file. It uses a parsed file already
-// present in the cache but otherwise does not populate the cache.
+// symbolize reads and parses a file and extracts symbols from it.
+// It may use a parsed file already present in the cache but
+// otherwise does not populate the cache.
func symbolize(snapshot *snapshot, fh source.FileHandle) ([]source.Symbol, error) {
src, err := fh.Read()
if err != nil {
diff --git a/internal/lsp/source/view.go b/internal/lsp/source/view.go
index 7960b0c..0d8d661 100644
--- a/internal/lsp/source/view.go
+++ b/internal/lsp/source/view.go
@@ -175,7 +175,7 @@
ActivePackages(ctx context.Context) ([]Package, error)
// Symbols returns all symbols in the snapshot.
- Symbols(ctx context.Context) (map[span.URI][]Symbol, error)
+ Symbols(ctx context.Context) map[span.URI][]Symbol
// Metadata returns package metadata associated with the given file URI.
MetadataForFile(ctx context.Context, uri span.URI) ([]Metadata, error)
diff --git a/internal/lsp/source/workspace_symbol.go b/internal/lsp/source/workspace_symbol.go
index 11e22d1..c7cfe5c 100644
--- a/internal/lsp/source/workspace_symbol.go
+++ b/internal/lsp/source/workspace_symbol.go
@@ -50,14 +50,26 @@
// with a different configured SymbolMatcher per View. Therefore we assume that
// Session level configuration will define the SymbolMatcher to be used for the
// WorkspaceSymbols method.
-func WorkspaceSymbols(ctx context.Context, matcherType SymbolMatcher, style SymbolStyle, views []View, query string) ([]protocol.SymbolInformation, error) {
+func WorkspaceSymbols(ctx context.Context, matcher SymbolMatcher, style SymbolStyle, views []View, query string) ([]protocol.SymbolInformation, error) {
ctx, done := event.Start(ctx, "source.WorkspaceSymbols")
defer done()
if query == "" {
return nil, nil
}
- sc := newSymbolCollector(matcherType, style, query)
- return sc.walk(ctx, views)
+
+ var s symbolizer
+ switch style {
+ case DynamicSymbols:
+ s = dynamicSymbolMatch
+ case FullyQualifiedSymbols:
+ s = fullyQualifiedSymbolMatch
+ case PackageQualifiedSymbols:
+ s = packageSymbolMatch
+ default:
+ panic(fmt.Errorf("unknown symbol style: %v", style))
+ }
+
+ return collectSymbols(ctx, views, matcher, s, query)
}
// A matcherFunc returns the index and score of a symbol match.
@@ -136,43 +148,6 @@
return nil, 0
}
-// symbolCollector holds context as we walk Packages, gathering symbols that
-// match a given query.
-//
-// How we match symbols is parameterized by two interfaces:
-// - A matcherFunc determines how well a string symbol matches a query. It
-// returns a non-negative score indicating the quality of the match. A score
-// of zero indicates no match.
-// - A symbolizer determines how we extract the symbol for an object. This
-// enables the 'symbolStyle' configuration option.
-type symbolCollector struct {
- // These types parameterize the symbol-matching pass.
- matchers []matcherFunc
- symbolizer symbolizer
-
- symbolStore
-}
-
-func newSymbolCollector(matcher SymbolMatcher, style SymbolStyle, query string) *symbolCollector {
- var s symbolizer
- switch style {
- case DynamicSymbols:
- s = dynamicSymbolMatch
- case FullyQualifiedSymbols:
- s = fullyQualifiedSymbolMatch
- case PackageQualifiedSymbols:
- s = packageSymbolMatch
- default:
- panic(fmt.Errorf("unknown symbol style: %v", style))
- }
- sc := &symbolCollector{symbolizer: s}
- sc.matchers = make([]matcherFunc, runtime.GOMAXPROCS(-1))
- for i := range sc.matchers {
- sc.matchers[i] = buildMatcher(matcher, query)
- }
- return sc
-}
-
func buildMatcher(matcher SymbolMatcher, query string) matcherFunc {
switch matcher {
case SymbolFuzzy:
@@ -302,36 +277,42 @@
return first, score
}
-func (sc *symbolCollector) walk(ctx context.Context, views []View) ([]protocol.SymbolInformation, error) {
- // Use the root view URIs for determining (lexically) whether a uri is in any
- // open workspace.
+// collectSymbols calls snapshot.Symbols to walk the syntax trees of
+// all files in the views' current snapshots, and returns a sorted,
+// scored list of symbols that best match the parameters.
+//
+// How it matches symbols is parameterized by two interfaces:
+// - A matcherFunc determines how well a string symbol matches a query. It
+// returns a non-negative score indicating the quality of the match. A score
+// of zero indicates no match.
+// - A symbolizer determines how we extract the symbol for an object. This
+// enables the 'symbolStyle' configuration option.
+//
+func collectSymbols(ctx context.Context, views []View, matcherType SymbolMatcher, symbolizer symbolizer, query string) ([]protocol.SymbolInformation, error) {
+
+ // Extract symbols from all files.
+ var work []symbolFile
var roots []string
- for _, v := range views {
- roots = append(roots, strings.TrimRight(string(v.Folder()), "/"))
- }
-
- results := make(chan *symbolStore)
- matcherlen := len(sc.matchers)
- files := make(map[span.URI]symbolFile)
-
+ seen := make(map[span.URI]bool)
+ // TODO(adonovan): opt: parallelize this loop? How often is len > 1?
for _, v := range views {
snapshot, release := v.Snapshot(ctx)
defer release()
- psyms, err := snapshot.Symbols(ctx)
- if err != nil {
- return nil, err
- }
+
+ // Use the root view URIs for determining (lexically)
+ // whether a URI is in any open workspace.
+ roots = append(roots, strings.TrimRight(string(v.Folder()), "/"))
filters := v.Options().DirectoryFilters
folder := filepath.ToSlash(v.Folder().Filename())
- for uri, syms := range psyms {
+ for uri, syms := range snapshot.Symbols(ctx) {
norm := filepath.ToSlash(uri.Filename())
nm := strings.TrimPrefix(norm, folder)
if FiltersDisallow(nm, filters) {
continue
}
// Only scan each file once.
- if _, ok := files[uri]; ok {
+ if seen[uri] {
continue
}
mds, err := snapshot.MetadataForFile(ctx, uri)
@@ -343,39 +324,37 @@
// TODO: should use the bug reporting API
continue
}
- files[uri] = symbolFile{uri, mds[0], syms}
+ seen[uri] = true
+ work = append(work, symbolFile{uri, mds[0], syms})
}
}
- var work []symbolFile
- for _, f := range files {
- work = append(work, f)
- }
-
- // Compute matches concurrently. Each symbolWorker has its own symbolStore,
+ // Match symbols in parallel.
+ // Each worker has its own symbolStore,
// which we merge at the end.
- for i, matcher := range sc.matchers {
- go func(i int, matcher matcherFunc) {
- w := &symbolWorker{
- symbolizer: sc.symbolizer,
- matcher: matcher,
- ss: &symbolStore{},
- roots: roots,
+ nmatchers := runtime.GOMAXPROCS(-1) // matching is CPU bound
+ results := make(chan *symbolStore)
+ for i := 0; i < nmatchers; i++ {
+ go func(i int) {
+ matcher := buildMatcher(matcherType, query)
+ store := new(symbolStore)
+ // Assign files to workers in round-robin fashion.
+ for j := i; j < len(work); j += nmatchers {
+ matchFile(store, symbolizer, matcher, roots, work[j])
}
- for j := i; j < len(work); j += matcherlen {
- w.matchFile(work[j])
- }
- results <- w.ss
- }(i, matcher)
+ results <- store
+ }(i)
}
- for i := 0; i < matcherlen; i++ {
- ss := <-results
- for _, si := range ss.res {
- sc.store(si)
+ // Gather and merge results as they arrive.
+ var unified symbolStore
+ for i := 0; i < nmatchers; i++ {
+ store := <-results
+ for _, syms := range store.res {
+ unified.store(syms)
}
}
- return sc.results(), nil
+ return unified.results(), nil
}
// FilterDisallow is code from the body of cache.pathExcludedByFilter in cache/view.go
@@ -407,20 +386,13 @@
syms []Symbol
}
-// symbolWorker matches symbols and captures the highest scoring results.
-type symbolWorker struct {
- symbolizer symbolizer
- matcher matcherFunc
- ss *symbolStore
- roots []string
-}
-
-func (w *symbolWorker) matchFile(i symbolFile) {
+// matchFile scans a symbol file and adds matching symbols to the store.
+func matchFile(store *symbolStore, symbolizer symbolizer, matcher matcherFunc, roots []string, i symbolFile) {
for _, sym := range i.syms {
- symbolParts, score := w.symbolizer(sym.Name, i.md, w.matcher)
+ symbolParts, score := symbolizer(sym.Name, i.md, matcher)
// Check if the score is too low before applying any downranking.
- if w.ss.tooLow(score) {
+ if store.tooLow(score) {
continue
}
@@ -463,7 +435,7 @@
}
inWorkspace := false
- for _, root := range w.roots {
+ for _, root := range roots {
if strings.HasPrefix(string(i.uri), root) {
inWorkspace = true
break
@@ -484,7 +456,7 @@
}
score *= 1.0 - depth*depthFactor
- if w.ss.tooLow(score) {
+ if store.tooLow(score) {
continue
}
@@ -496,7 +468,7 @@
rng: sym.Range,
container: i.md.PackagePath(),
}
- w.ss.store(si)
+ store.store(si)
}
}
