gopls/internal/lsp/cache: don't rebuild filters for each modcache dir

While investigating a post-review comment on CL 538796, I observed that
the importsState.SkipPathInScan func is building a directory filterer
for every call. This can be very expensive, and may partially explain
the performance problems encountered in golang/go#59216.

Fix this, and update the test to catch bugs related to mixing up relpath
vs abspath (a mistake I made during this CL).

Updates golang/go#59216

Change-Id: I696429161ba039a380df1cf258edf78369acb398
Reviewed-on: https://go-review.googlesource.com/c/tools/+/544515
LUCI-TryBot-Result: Go LUCI <[email protected]>
Reviewed-by: Alan Donovan <[email protected]>
Auto-Submit: Robert Findley <[email protected]>

Author: findleyr

gopls/internal/lsp/cache/session.go

@@ -122,6 +122,32 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition, folder *F
 	baseCtx := event.Detach(xcontext.Detach(ctx))
 	backgroundCtx, cancel := context.WithCancel(baseCtx)
 
+	// Compute a skip function to use for module cache scanning.
+	//
+	// Note that unlike other filtering operations, we definitely don't want to
+	// exclude the gomodcache here, even if it is contained in the workspace
+	// folder.
+	//
+	// TODO(rfindley): consolidate with relPathExcludedByFilter(Func), Filterer,
+	// View.filterFunc.
+	var skipPath func(string) bool
+	{
+		// Compute a prefix match, respecting segment boundaries, by ensuring
+		// the pattern (dir) has a trailing slash.
+		dirPrefix := strings.TrimSuffix(string(folder.Dir), "/") + "/"
+		filterer := NewFilterer(folder.Options.DirectoryFilters)
+		skipPath = func(dir string) bool {
+			uri := strings.TrimSuffix(string(protocol.URIFromPath(dir)), "/")
+			// Note that the logic below doesn't handle the case where uri ==
+			// v.folder.Dir, because there is no point in excluding the entire
+			// workspace folder!
+			if rel := strings.TrimPrefix(uri, dirPrefix); rel != uri {
+				return filterer.Disallow(rel)
+			}
+			return false
+		}
+	}
+
 	v := &View{
 		id:                   strconv.FormatInt(index, 10),
 		gocmdRunner:          s.gocmdRunner,
@@ -132,24 +158,15 @@ func (s *Session) createView(ctx context.Context, def *viewDefinition, folder *F
 		parseCache:           s.parseCache,
 		fs:                   s.overlayFS,
 		viewDefinition:       def,
-	}
-	v.importsState = &importsState{
-		ctx: backgroundCtx,
-		processEnv: &imports.ProcessEnv{
-			GocmdRunner: s.gocmdRunner,
-			SkipPathInScan: func(dir string) bool {
-				prefix := strings.TrimSuffix(string(v.folder.Dir), "/") + "/"
-				uri := strings.TrimSuffix(string(protocol.URIFromPath(dir)), "/")
-				if !strings.HasPrefix(uri+"/", prefix) {
-					return false
-				}
-				filterer := NewFilterer(folder.Options.DirectoryFilters)
-				rel := strings.TrimPrefix(uri, prefix)
-				disallow := filterer.Disallow(rel)
-				return disallow
+		importsState: &importsState{
+			ctx: backgroundCtx,
+			processEnv: &imports.ProcessEnv{
+				GocmdRunner:    s.gocmdRunner,
+				SkipPathInScan: skipPath,
 			},
 		},
 	}
+
 	v.snapshot = &Snapshot{
 		sequenceID:       seqID,
 		globalID:         nextSnapshotID(),

gopls/internal/lsp/cache/view.go

@@ -585,11 +585,11 @@ func (v *View) contains(uri protocol.DocumentURI) bool {
 // directoryFilters.
 func (v *View) filterFunc() func(protocol.DocumentURI) bool {
 	folderDir := v.folder.Dir.Path()
-	filterer := buildFilterer(folderDir, v.gomodcache, v.folder.Options)
+	filterer := buildFilterer(folderDir, v.gomodcache, v.folder.Options.DirectoryFilters)
 	return func(uri protocol.DocumentURI) bool {
 		// Only filter relative to the configured root directory.
 		if pathutil.InDir(folderDir, uri.Path()) {
-			return pathExcludedByFilter(strings.TrimPrefix(uri.Path(), folderDir), filterer)
+			return relPathExcludedByFilter(strings.TrimPrefix(uri.Path(), folderDir), filterer)
 		}
 		return false
 	}
@@ -971,7 +971,7 @@ func getViewDefinition(ctx context.Context, runner *gocommand.Runner, fs file.So
 
 	// filterFunc is the path filter function for this workspace folder. Notably,
 	// it is relative to folder (which is specified by the user), not root.
-	filterFunc := pathExcludedByFilterFunc(folder.Dir.Path(), def.gomodcache, folder.Options)
+	filterFunc := relPathExcludedByFilterFunc(folder.Dir.Path(), def.gomodcache, folder.Options.DirectoryFilters)
 	def.gomod, err = findWorkspaceModFile(ctx, folder.Dir, fs, filterFunc)
 	if err != nil {
 		return nil, err
@@ -1197,6 +1197,8 @@ var modFlagRegexp = regexp.MustCompile(`-mod[ =](\w+)`)
 // after we have a version of the workspace go.mod file on disk. Getting a
 // FileHandle from the cache for temporary files is problematic, since we
 // cannot delete it.
+//
+// TODO(rfindley): move this to snapshot.go.
 func (s *Snapshot) vendorEnabled(ctx context.Context, modURI protocol.DocumentURI, modContent []byte) (bool, error) {
 	// Legacy GOPATH workspace?
 	if len(s.view.workspaceModFiles) == 0 {
@@ -1244,27 +1246,26 @@ func allFilesExcluded(files []string, filterFunc func(protocol.DocumentURI) bool
 	return true
 }
 
-func pathExcludedByFilterFunc(folder, gomodcache string, opts *settings.Options) func(string) bool {
-	filterer := buildFilterer(folder, gomodcache, opts)
+// relPathExcludedByFilterFunc returns a func that filters paths relative to the
+// given folder according the given GOMODCACHE value and directory filters (see
+// settings.BuildOptions.DirectoryFilters).
+//
+// The resulting func returns true if the directory should be skipped.
+func relPathExcludedByFilterFunc(folder, gomodcache string, directoryFilters []string) func(string) bool {
+	filterer := buildFilterer(folder, gomodcache, directoryFilters)
 	return func(path string) bool {
-		return pathExcludedByFilter(path, filterer)
+		return relPathExcludedByFilter(path, filterer)
 	}
 }
 
-// pathExcludedByFilter reports whether the path (relative to the workspace
-// folder) should be excluded by the configured directory filters.
-//
-// TODO(rfindley): passing root and gomodcache here makes it confusing whether
-// path should be absolute or relative, and has already caused at least one
-// bug.
-func pathExcludedByFilter(path string, filterer *Filterer) bool {
+func relPathExcludedByFilter(path string, filterer *Filterer) bool {
 	path = strings.TrimPrefix(filepath.ToSlash(path), "/")
 	return filterer.Disallow(path)
 }
 
-func buildFilterer(folder, gomodcache string, opts *settings.Options) *Filterer {
-	filters := opts.DirectoryFilters
-
+func buildFilterer(folder, gomodcache string, directoryFilters []string) *Filterer {
+	var filters []string
+	filters = append(filters, directoryFilters...)
 	if pref := strings.TrimPrefix(gomodcache, folder); pref != gomodcache {
 		modcacheFilter := "-" + strings.TrimPrefix(filepath.ToSlash(pref), "/")
 		filters = append(filters, modcacheFilter)

gopls/internal/lsp/cache/view_test.go

@@ -152,12 +152,12 @@ func TestFilters(t *testing.T) {
 	for _, tt := range tests {
 		filterer := NewFilterer(tt.filters)
 		for _, inc := range tt.included {
-			if pathExcludedByFilter(inc, filterer) {
+			if relPathExcludedByFilter(inc, filterer) {
 				t.Errorf("filters %q excluded %v, wanted included", tt.filters, inc)
 			}
 		}
 		for _, exc := range tt.excluded {
-			if !pathExcludedByFilter(exc, filterer) {
+			if !relPathExcludedByFilter(exc, filterer) {
 				t.Errorf("filters %q included %v, wanted excluded", tt.filters, exc)
 			}
 		}

gopls/internal/regtest/workspace/directoryfilters_test.go

@@ -171,23 +171,22 @@ package main
 
 func main() {
 	bye.Goodbye()
+	hi.Hello()
 }
 -- p/bye/bye.go --
 package bye
 
 func Goodbye() {}
+-- hi/hi.go --
+package hi
+
+func Hello() {}
 `
 
 	WithOptions(
 		Settings{
-			"directoryFilters": []string{"-**/bye"},
+			"directoryFilters": []string{"-**/bye", "-hi"},
 		},
-		// This test breaks in 'Experimental' mode, because with
-		// experimentalWorkspaceModule set we the goimports scan behaves
-		// differently.
-		//
-		// Since this feature is going away (golang/go#52897), don't investigate.
-		Modes(Default),
 	).Run(t, files, func(t *testing.T, env *Env) {
 		env.OpenFile("main.go")
 		beforeSave := env.BufferText("main.go")