gopls/internal/regtest/bench: add benchmarks in a wider variety of repos

Extend existing benchmarks to run in more repos, choosing an initial set
with different features that may affect performance. Some of these take
too long if run in the same batch, so guard with -short.

Several benchmarks need a location within the codebase. For these, I
have chosen somewhat arbitrarily, but tried to select locations within
the core of the codebase. We can always adjust in the future.

Additionally:
 - fix the fake file polling to scale to larger codebases, by avoiding
   reading files if it isn't necessary
 - fix a polling bug related to symlinks
 - fix a couple places the benchmarks weren't cleaning up after
   themselves correctly
 - fix a bug where the gopls install used the wrong directory

For golang/go#53538

Change-Id: I559031cb068086cd5ec19e36bb12da396194933c
Reviewed-on: https://go-review.googlesource.com/c/tools/+/469355
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>

gopls/internal/lsp/fake/editor.go

@@ -106,7 +106,7 @@ type EditorConfig struct {
 	Settings map[string]interface{}
 }
 
-// NewEditor Creates a new Editor.
+// NewEditor creates a new Editor.
 func NewEditor(sandbox *Sandbox, config EditorConfig) *Editor {
 	return &Editor{
 		buffers:    make(map[string]buffer),
@@ -959,7 +959,7 @@ func (e *Editor) ExecuteCommand(ctx context.Context, params *protocol.ExecuteCom
 	// Some commands use the go command, which writes directly to disk.
 	// For convenience, check for those changes.
 	if err := e.sandbox.Workdir.CheckForFileChanges(ctx); err != nil {
-		return nil, err
+		return nil, fmt.Errorf("checking for file changes: %v", err)
 	}
 	return result, nil
 }

gopls/internal/lsp/fake/sandbox.go

@@ -118,7 +118,10 @@ func NewSandbox(config *SandboxConfig) (_ *Sandbox, err error) {
 	// this is used for running in an existing directory.
 	// TODO(findleyr): refactor this to be less of a workaround.
 	if filepath.IsAbs(config.Workdir) {
-		sb.Workdir = NewWorkdir(config.Workdir)
+		sb.Workdir, err = NewWorkdir(config.Workdir, nil)
+		if err != nil {
+			return nil, err
+		}
 		return sb, nil
 	}
 	var workdir string
@@ -136,8 +139,8 @@ func NewSandbox(config *SandboxConfig) (_ *Sandbox, err error) {
 	if err := os.MkdirAll(workdir, 0755); err != nil {
 		return nil, err
 	}
-	sb.Workdir = NewWorkdir(workdir)
-	if err := sb.Workdir.writeInitialFiles(config.Files); err != nil {
+	sb.Workdir, err = NewWorkdir(workdir, config.Files)
+	if err != nil {
 		return nil, err
 	}
 	return sb, nil

gopls/internal/lsp/fake/workdir.go

@@ -9,6 +9,7 @@ import (
 	"context"
 	"crypto/sha256"
 	"fmt"
+	"io/fs"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -103,49 +104,27 @@ type Workdir struct {
 	files map[string]fileID
 }
 
-// fileID is a file identity for the purposes of detecting on-disk
-// modifications.
-type fileID struct {
-	hash  string
-	mtime time.Time
-}
-
 // NewWorkdir writes the txtar-encoded file data in txt to dir, and returns a
 // Workir for operating on these files using
-func NewWorkdir(dir string) *Workdir {
-	return &Workdir{RelativeTo: RelativeTo(dir)}
-}
-
-func hashFile(data []byte) string {
-	return fmt.Sprintf("%x", sha256.Sum256(data))
-}
-
-func (w *Workdir) writeInitialFiles(files map[string][]byte) error {
-	w.files = map[string]fileID{}
+func NewWorkdir(dir string, files map[string][]byte) (*Workdir, error) {
+	w := &Workdir{RelativeTo: RelativeTo(dir)}
 	for name, data := range files {
 		if err := writeFileData(name, data, w.RelativeTo); err != nil {
-			return fmt.Errorf("writing to workdir: %w", err)
+			return nil, fmt.Errorf("writing to workdir: %w", err)
 		}
-		fp := w.AbsPath(name)
+	}
+	_, err := w.pollFiles() // poll files to populate the files map.
+	return w, err
+}
 
-		// We need the mtime of the file just written for the purposes of tracking
-		// file identity. Calling Stat here could theoretically return an mtime
-		// that is inconsistent with the file contents represented by the hash, but
-		// since we "own" this file we assume that the mtime is correct.
-		//
-		// Furthermore, see the documentation for Workdir.files for why mismatches
-		// between identifiers are considered to be benign.
-		fi, err := os.Stat(fp)
-		if err != nil {
-			return fmt.Errorf("reading file info: %v", err)
-		}
+// fileID identifies a file version on disk.
+type fileID struct {
+	mtime time.Time
+	hash  string // empty if mtime is old enough to be reliabe; otherwise a file digest
+}
 
-		w.files[name] = fileID{
-			hash:  hashFile(data),
-			mtime: fi.ModTime(),
-		}
-	}
-	return nil
+func hashFile(data []byte) string {
+	return fmt.Sprintf("%x", sha256.Sum256(data))
 }
 
 // RootURI returns the root URI for this working directory of this scratch
@@ -335,49 +314,21 @@ func (w *Workdir) RenameFile(ctx context.Context, oldPath, newPath string) error
 // ListFiles returns a new sorted list of the relative paths of files in dir,
 // recursively.
 func (w *Workdir) ListFiles(dir string) ([]string, error) {
-	m, err := w.listFiles(dir)
-	if err != nil {
-		return nil, err
-	}
-
-	var paths []string
-	for p := range m {
-		paths = append(paths, p)
-	}
-	sort.Strings(paths)
-	return paths, nil
-}
-
-// listFiles lists files in the given directory, returning a map of relative
-// path to contents and modification time.
-func (w *Workdir) listFiles(dir string) (map[string]fileID, error) {
-	files := make(map[string]fileID)
 	absDir := w.AbsPath(dir)
+	var paths []string
 	if err := filepath.Walk(absDir, func(fp string, info os.FileInfo, err error) error {
 		if err != nil {
 			return err
 		}
-		if info.IsDir() {
-			return nil
-		}
-		path := w.RelPath(fp)
-
-		data, err := ioutil.ReadFile(fp)
-		if err != nil {
-			return err
-		}
-		// The content returned by ioutil.ReadFile could be inconsistent with
-		// info.ModTime(), due to a subsequent modification. See the documentation
-		// for w.files for why we consider this to be benign.
-		files[path] = fileID{
-			hash:  hashFile(data),
-			mtime: info.ModTime(),
+		if info.Mode()&(fs.ModeDir|fs.ModeSymlink) == 0 {
+			paths = append(paths, w.RelPath(fp))
 		}
 		return nil
 	}); err != nil {
 		return nil, err
 	}
-	return files, nil
+	sort.Strings(paths)
+	return paths, nil
 }
 
 // CheckForFileChanges walks the working directory and checks for any files
@@ -406,36 +357,82 @@ func (w *Workdir) pollFiles() ([]protocol.FileEvent, error) {
 	w.fileMu.Lock()
 	defer w.fileMu.Unlock()
 
-	files, err := w.listFiles(".")
-	if err != nil {
-		return nil, err
-	}
+	newFiles := make(map[string]fileID)
 	var evts []protocol.FileEvent
-	// Check which files have been added or modified.
-	for path, id := range files {
-		oldID, ok := w.files[path]
-		delete(w.files, path)
-		var typ protocol.FileChangeType
-		switch {
-		case !ok:
-			typ = protocol.Created
-		case oldID != id:
-			typ = protocol.Changed
-		default:
-			continue
+	if err := filepath.Walk(string(w.RelativeTo), func(fp string, info os.FileInfo, err error) error {
+		if err != nil {
+			return err
 		}
-		evts = append(evts, protocol.FileEvent{
-			URI:  w.URI(path),
-			Type: typ,
-		})
+		// Skip directories and symbolic links (which may be links to directories).
+		//
+		// The latter matters for repos like Kubernetes, which use symlinks.
+		if info.Mode()&(fs.ModeDir|fs.ModeSymlink) != 0 {
+			return nil
+		}
+
+		// Opt: avoid reading the file if mtime is sufficently old to be reliable.
+		//
+		// If mtime is recent, it may not sufficiently identify the file contents:
+		// a subsequent write could result in the same mtime. For these cases, we
+		// must read the file contents.
+		id := fileID{mtime: info.ModTime()}
+		if time.Since(info.ModTime()) < 2*time.Second {
+			data, err := ioutil.ReadFile(fp)
+			if err != nil {
+				return err
+			}
+			id.hash = hashFile(data)
+		}
+		path := w.RelPath(fp)
+		newFiles[path] = id
+
+		if w.files != nil {
+			oldID, ok := w.files[path]
+			delete(w.files, path)
+			switch {
+			case !ok:
+				evts = append(evts, protocol.FileEvent{
+					URI:  w.URI(path),
+					Type: protocol.Created,
+				})
+			case oldID != id:
+				changed := true
+
+				// Check whether oldID and id do not match because oldID was polled at
+				// a recent enough to time such as to require hashing.
+				//
+				// In this case, read the content to check whether the file actually
+				// changed.
+				if oldID.mtime.Equal(id.mtime) && oldID.hash != "" && id.hash == "" {
+					data, err := ioutil.ReadFile(fp)
+					if err != nil {
+						return err
+					}
+					if hashFile(data) == oldID.hash {
+						changed = false
+					}
+				}
+				if changed {
+					evts = append(evts, protocol.FileEvent{
+						URI:  w.URI(path),
+						Type: protocol.Changed,
+					})
+				}
+			}
+		}
+
+		return nil
+	}); err != nil {
+		return nil, err
 	}
+
 	// Any remaining files must have been deleted.
 	for path := range w.files {
 		evts = append(evts, protocol.FileEvent{
 			URI:  w.URI(path),
 			Type: protocol.Deleted,
 		})
 	}
-	w.files = files
+	w.files = newFiles
 	return evts, nil
 }

gopls/internal/lsp/fake/workdir_test.go

@@ -8,7 +8,6 @@ import (
 	"context"
 	"io/ioutil"
 	"os"
-	"sort"
 	"sync"
 	"testing"
 
@@ -37,8 +36,8 @@ func newWorkdir(t *testing.T, txt string) (*Workdir, *eventBuffer, func()) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	wd := NewWorkdir(tmpdir)
-	if err := wd.writeInitialFiles(UnpackTxt(txt)); err != nil {
+	wd, err := NewWorkdir(tmpdir, UnpackTxt(txt))
+	if err != nil {
 		t.Fatal(err)
 	}
 	cleanup := func() {
@@ -162,35 +161,6 @@ func TestWorkdir_FileWatching(t *testing.T) {
 	checkEvent(changeMap{"bar.go": protocol.Deleted})
 }
 
-func TestWorkdir_ListFiles(t *testing.T) {
-	wd, _, cleanup := newWorkdir(t, sharedData)
-	defer cleanup()
-
-	checkFiles := func(dir string, want []string) {
-		files, err := wd.listFiles(dir)
-		if err != nil {
-			t.Fatal(err)
-		}
-		sort.Strings(want)
-		var got []string
-		for p := range files {
-			got = append(got, p)
-		}
-		sort.Strings(got)
-		if len(got) != len(want) {
-			t.Fatalf("ListFiles(): len = %d, want %d; got=%v; want=%v", len(got), len(want), got, want)
-		}
-		for i, f := range got {
-			if f != want[i] {
-				t.Errorf("ListFiles()[%d] = %s, want %s", i, f, want[i])
-			}
-		}
-	}
-
-	checkFiles(".", []string{"go.mod", "nested/README.md"})
-	checkFiles("nested", []string{"nested/README.md"})
-}
-
 func TestWorkdir_CheckForFileChanges(t *testing.T) {
 	t.Skip("broken on darwin-amd64-10_12")
 	wd, events, cleanup := newWorkdir(t, sharedData)

gopls/internal/regtest/bench/bench_test.go

@@ -164,18 +164,18 @@ func getInstalledGopls() string {
 	if *goplsCommit == "" {
 		panic("must provide -gopls_commit")
 	}
-	toolsDir := filepath.Join(getTempDir(), "tools")
+	toolsDir := filepath.Join(getTempDir(), "gopls_build")
 	goplsPath := filepath.Join(toolsDir, "gopls", "gopls")
 
 	installGoplsOnce.Do(func() {
-		log.Printf("installing gopls: checking out x/tools@%s\n", *goplsCommit)
+		log.Printf("installing gopls: checking out x/tools@%s into %s\n", *goplsCommit, toolsDir)
 		if err := shallowClone(toolsDir, "https://go.googlesource.com/tools", *goplsCommit); err != nil {
 			log.Fatal(err)
 		}
 
 		log.Println("installing gopls: building...")
 		bld := exec.Command("go", "build", ".")
-		bld.Dir = filepath.Join(getTempDir(), "tools", "gopls")
+		bld.Dir = filepath.Join(toolsDir, "gopls")
 		if output, err := bld.CombinedOutput(); err != nil {
 			log.Fatalf("building gopls: %v\n%s", err, output)
 		}

gopls/internal/regtest/bench/completion_test.go

@@ -8,10 +8,13 @@ import (
 	"fmt"
 	"testing"
 
+	"golang.org/x/tools/gopls/internal/lsp/fake"
 	"golang.org/x/tools/gopls/internal/lsp/protocol"
 	. "golang.org/x/tools/gopls/internal/lsp/regtest"
 )
 
+// TODO(rfindley): update these completion tests to run on multiple repos.
+
 type completionBenchOptions struct {
 	file, locationRegexp string
 
@@ -21,8 +24,8 @@ type completionBenchOptions struct {
 }
 
 func benchmarkCompletion(options completionBenchOptions, b *testing.B) {
-	repo := repos["tools"]
-	env := repo.newEnv(b)
+	repo := getRepo(b, "tools")
+	env := repo.newEnv(b, "completion.tools", fake.EditorConfig{})
 	defer env.Close()
 
 	// Run edits required for this completion.
@@ -41,12 +44,7 @@ func benchmarkCompletion(options completionBenchOptions, b *testing.B) {
 		}
 	}
 
-	b.ResetTimer()
-
-	// Use a subtest to ensure that benchmarkCompletion does not itself get
-	// executed multiple times (as it is doing expensive environment
-	// initialization).
-	b.Run("completion", func(b *testing.B) {
+	b.Run("tools", func(b *testing.B) {
 		for i := 0; i < b.N; i++ {
 			if options.beforeCompletion != nil {
 				options.beforeCompletion(env)
@@ -56,7 +54,7 @@ func benchmarkCompletion(options completionBenchOptions, b *testing.B) {
 	})
 }
 
-// endPosInBuffer returns the position for last character in the buffer for
+// endRangeInBuffer returns the position for last character in the buffer for
 // the given file.
 func endRangeInBuffer(env *Env, name string) protocol.Range {
 	buffer := env.BufferText(name)