diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go index 6450c60a916..1a013444568 100644 --- a/gopls/internal/lsp/fake/editor.go +++ b/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 } diff --git a/gopls/internal/lsp/fake/sandbox.go b/gopls/internal/lsp/fake/sandbox.go index 018bace3b30..a1557569bd7 100644 --- a/gopls/internal/lsp/fake/sandbox.go +++ b/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 diff --git a/gopls/internal/lsp/fake/workdir.go b/gopls/internal/lsp/fake/workdir.go index 97d70b9cafb..29344514d0a 100644 --- a/gopls/internal/lsp/fake/workdir.go +++ b/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,29 +357,75 @@ 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{ @@ -436,6 +433,6 @@ func (w *Workdir) pollFiles() ([]protocol.FileEvent, error) { Type: protocol.Deleted, }) } - w.files = files + w.files = newFiles return evts, nil } diff --git a/gopls/internal/lsp/fake/workdir_test.go b/gopls/internal/lsp/fake/workdir_test.go index d036658ef2d..fe89fa72dc3 100644 --- a/gopls/internal/lsp/fake/workdir_test.go +++ b/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) diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go index fa06e274e35..76b3dcfd489 100644 --- a/gopls/internal/regtest/bench/bench_test.go +++ b/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) } diff --git a/gopls/internal/regtest/bench/completion_test.go b/gopls/internal/regtest/bench/completion_test.go index a89a4ff68f7..2ddd8c11ef2 100644 --- a/gopls/internal/regtest/bench/completion_test.go +++ b/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) diff --git a/gopls/internal/regtest/bench/definition_test.go b/gopls/internal/regtest/bench/definition_test.go index cdffcf654ad..a3e68f5327a 100644 --- a/gopls/internal/regtest/bench/definition_test.go +++ b/gopls/internal/regtest/bench/definition_test.go @@ -9,16 +9,31 @@ import ( ) func BenchmarkDefinition(b *testing.B) { - env := repos["tools"].sharedEnv(b) - - env.OpenFile("internal/imports/mod.go") - loc := env.RegexpSearch("internal/imports/mod.go", "ModuleJSON") - env.GoToDefinition(loc) - env.Await(env.DoneWithOpen()) + tests := []struct { + repo string + file string + regexp string + }{ + {"istio", "pkg/config/model.go", `gogotypes\.(MarshalAny)`}, + {"kubernetes", "pkg/controller/lookup_cache.go", `hashutil\.(DeepHashObject)`}, + {"kuma", "api/generic/insights.go", `proto\.(Message)`}, + {"pkgsite", "internal/log/log.go", `derrors\.(Wrap)`}, + {"starlark", "starlark/eval.go", "prog.compiled.(Encode)"}, + {"tools", "internal/lsp/cache/check.go", `(snapshot)\) buildKey`}, + } - b.ResetTimer() + for _, test := range tests { + b.Run(test.repo, func(b *testing.B) { + env := getRepo(b, test.repo).sharedEnv(b) + env.OpenFile(test.file) + loc := env.RegexpSearch(test.file, test.regexp) + env.Await(env.DoneWithOpen()) + env.GoToDefinition(loc) // pre-warm the query, and open the target file + b.ResetTimer() - for i := 0; i < b.N; i++ { - env.GoToDefinition(loc) + for i := 0; i < b.N; i++ { + env.GoToDefinition(loc) // pre-warm the query + } + }) } } diff --git a/gopls/internal/regtest/bench/didchange_test.go b/gopls/internal/regtest/bench/didchange_test.go index e18ad4e9aa6..bb639ee15ac 100644 --- a/gopls/internal/regtest/bench/didchange_test.go +++ b/gopls/internal/regtest/bench/didchange_test.go @@ -15,27 +15,42 @@ import ( // synthetic modifications in a comment. It controls pacing by waiting for the // server to actually start processing the didChange notification before // proceeding. Notably it does not wait for diagnostics to complete. -// -// Uses -workdir and -file to control where the edits occur. func BenchmarkDidChange(b *testing.B) { - env := repos["tools"].sharedEnv(b) - const filename = "go/ast/astutil/util.go" - env.OpenFile(filename) - env.AfterChange() + tests := []struct { + repo string + file string + }{ + {"istio", "pkg/fuzz/util.go"}, + {"kubernetes", "pkg/controller/lookup_cache.go"}, + {"kuma", "api/generic/insights.go"}, + {"pkgsite", "internal/frontend/server.go"}, + {"starlark", "starlark/eval.go"}, + {"tools", "internal/lsp/cache/snapshot.go"}, + } - // Insert the text we'll be modifying at the top of the file. - env.EditBuffer(filename, protocol.TextEdit{NewText: "// __REGTEST_PLACEHOLDER_0__\n"}) + for _, test := range tests { + edits := 0 // bench function may execute multiple times + b.Run(test.repo, func(b *testing.B) { + env := getRepo(b, test.repo).sharedEnv(b) + env.OpenFile(test.file) + env.AfterChange() + // Insert the text we'll be modifying at the top of the file. + env.EditBuffer(test.file, protocol.TextEdit{NewText: "// __REGTEST_PLACEHOLDER_0__\n"}) + env.AfterChange() + b.ResetTimer() - b.ResetTimer() - for i := 0; i < b.N; i++ { - env.EditBuffer(filename, protocol.TextEdit{ - Range: protocol.Range{ - Start: protocol.Position{Line: 0, Character: 0}, - End: protocol.Position{Line: 1, Character: 0}, - }, - // Increment the placeholder text, to ensure cache misses. - NewText: fmt.Sprintf("// __REGTEST_PLACEHOLDER_%d__\n", i+1), + for i := 0; i < b.N; i++ { + edits++ + env.EditBuffer(test.file, protocol.TextEdit{ + Range: protocol.Range{ + Start: protocol.Position{Line: 0, Character: 0}, + End: protocol.Position{Line: 1, Character: 0}, + }, + // Increment the placeholder text, to ensure cache misses. + NewText: fmt.Sprintf("// __REGTEST_PLACEHOLDER_%d__\n", edits), + }) + env.Await(env.StartedChange()) + } }) - env.Await(env.StartedChange()) } } diff --git a/gopls/internal/regtest/bench/hover_test.go b/gopls/internal/regtest/bench/hover_test.go index ebd89b87d0d..e89e03b332a 100644 --- a/gopls/internal/regtest/bench/hover_test.go +++ b/gopls/internal/regtest/bench/hover_test.go @@ -9,15 +9,31 @@ import ( ) func BenchmarkHover(b *testing.B) { - env := repos["tools"].sharedEnv(b) - - env.OpenFile("internal/imports/mod.go") - loc := env.RegexpSearch("internal/imports/mod.go", "bytes") - env.Await(env.DoneWithOpen()) + tests := []struct { + repo string + file string + regexp string + }{ + {"istio", "pkg/config/model.go", `gogotypes\.(MarshalAny)`}, + {"kubernetes", "pkg/apis/core/types.go", "type (Pod)"}, + {"kuma", "api/generic/insights.go", `proto\.(Message)`}, + {"pkgsite", "internal/log/log.go", `derrors\.(Wrap)`}, + {"starlark", "starlark/eval.go", "prog.compiled.(Encode)"}, + {"tools", "internal/lsp/cache/check.go", `(snapshot)\) buildKey`}, + } - b.ResetTimer() + for _, test := range tests { + b.Run(test.repo, func(b *testing.B) { + env := getRepo(b, test.repo).sharedEnv(b) + env.OpenFile(test.file) + loc := env.RegexpSearch(test.file, test.regexp) + env.Await(env.DoneWithOpen()) + env.Hover(loc) // pre-warm the query + b.ResetTimer() - for i := 0; i < b.N; i++ { - env.Hover(loc) + for i := 0; i < b.N; i++ { + env.Hover(loc) // pre-warm the query + } + }) } } diff --git a/gopls/internal/regtest/bench/implementations_test.go b/gopls/internal/regtest/bench/implementations_test.go index 7f83987d049..219f42a374c 100644 --- a/gopls/internal/regtest/bench/implementations_test.go +++ b/gopls/internal/regtest/bench/implementations_test.go @@ -7,15 +7,31 @@ package bench import "testing" func BenchmarkImplementations(b *testing.B) { - env := repos["tools"].sharedEnv(b) - - env.OpenFile("internal/imports/mod.go") - loc := env.RegexpSearch("internal/imports/mod.go", "initAllMods") - env.Await(env.DoneWithOpen()) + tests := []struct { + repo string + file string + regexp string + }{ + {"istio", "pkg/config/mesh/watcher.go", `type (Watcher)`}, + {"kubernetes", "pkg/controller/lookup_cache.go", `objectWithMeta`}, + {"kuma", "api/generic/insights.go", `type (Insight)`}, + {"pkgsite", "internal/datasource.go", `type (DataSource)`}, + {"starlark", "syntax/syntax.go", `type (Expr)`}, + {"tools", "internal/lsp/source/view.go", `type (Snapshot)`}, + } - b.ResetTimer() + for _, test := range tests { + b.Run(test.repo, func(b *testing.B) { + env := getRepo(b, test.repo).sharedEnv(b) + env.OpenFile(test.file) + loc := env.RegexpSearch(test.file, test.regexp) + env.Await(env.DoneWithOpen()) + env.Implementations(loc) // pre-warm the query + b.ResetTimer() - for i := 0; i < b.N; i++ { - env.Implementations(loc) + for i := 0; i < b.N; i++ { + env.Implementations(loc) + } + }) } } diff --git a/gopls/internal/regtest/bench/iwl_test.go b/gopls/internal/regtest/bench/iwl_test.go index 44cdc785fa9..974fc1eb1c8 100644 --- a/gopls/internal/regtest/bench/iwl_test.go +++ b/gopls/internal/regtest/bench/iwl_test.go @@ -8,6 +8,7 @@ import ( "testing" "golang.org/x/tools/gopls/internal/lsp/command" + "golang.org/x/tools/gopls/internal/lsp/fake" "golang.org/x/tools/gopls/internal/lsp/protocol" . "golang.org/x/tools/gopls/internal/lsp/regtest" ) @@ -15,27 +16,57 @@ import ( // BenchmarkInitialWorkspaceLoad benchmarks the initial workspace load time for // a new editing session. func BenchmarkInitialWorkspaceLoad(b *testing.B) { - repo := repos["tools"] - b.ResetTimer() - - for i := 0; i < b.N; i++ { - // Exclude the time to set up the env from the benchmark time, as this may - // involve installing gopls and/or checking out the repo dir. - b.StopTimer() - env := repo.newEnv(b) - b.StartTimer() - - env.OpenFile("internal/lsp/diagnostics.go") - env.Await(InitialWorkspaceLoad) - b.StopTimer() - params := &protocol.ExecuteCommandParams{ - Command: command.MemStats.ID(), - } - var memstats command.MemStatsResult - env.ExecuteCommand(params, &memstats) - b.ReportMetric(float64(memstats.HeapAlloc), "alloc_bytes") - b.ReportMetric(float64(memstats.HeapInUse), "in_use_bytes") - env.Close() - b.StartTimer() + tests := []struct { + repo string + file string + }{ + {"tools", "internal/lsp/cache/snapshot.go"}, + {"kubernetes", "pkg/controller/lookup_cache.go"}, + {"pkgsite", "internal/frontend/server.go"}, + {"starlark", "starlark/eval.go"}, + {"istio", "pkg/fuzz/util.go"}, + {"kuma", "api/generic/insights.go"}, } + + for _, test := range tests { + b.Run(test.repo, func(b *testing.B) { + repo := getRepo(b, test.repo) + // get the (initialized) shared env to ensure the cache is warm. + // Reuse its GOPATH so that we get cache hits for things in the module + // cache. + sharedEnv := repo.sharedEnv(b) + b.ResetTimer() + + for i := 0; i < b.N; i++ { + doIWL(b, sharedEnv.Sandbox.GOPATH(), repo, test.file) + } + }) + } +} + +func doIWL(b *testing.B, gopath string, repo *repo, file string) { + // Exclude the time to set up the env from the benchmark time, as this may + // involve installing gopls and/or checking out the repo dir. + b.StopTimer() + config := fake.EditorConfig{Env: map[string]string{"GOPATH": gopath}} + env := repo.newEnv(b, "iwl."+repo.name, config) + defer env.Close() + b.StartTimer() + + // Open an arbitrary file to ensure that gopls starts working. + // + // In the future, this may matter if gopls doesn't eagerly construct + // the workspace. + env.OpenFile(file) + + env.Await(InitialWorkspaceLoad) + b.StopTimer() + params := &protocol.ExecuteCommandParams{ + Command: command.MemStats.ID(), + } + var memstats command.MemStatsResult + env.ExecuteCommand(params, &memstats) + b.ReportMetric(float64(memstats.HeapAlloc), "alloc_bytes") + b.ReportMetric(float64(memstats.HeapInUse), "in_use_bytes") + b.StartTimer() } diff --git a/gopls/internal/regtest/bench/references_test.go b/gopls/internal/regtest/bench/references_test.go index 782275053b5..d47ea56a47e 100644 --- a/gopls/internal/regtest/bench/references_test.go +++ b/gopls/internal/regtest/bench/references_test.go @@ -7,16 +7,31 @@ package bench import "testing" func BenchmarkReferences(b *testing.B) { - env := repos["tools"].sharedEnv(b) - - env.OpenFile("internal/imports/mod.go") - loc := env.RegexpSearch("internal/imports/mod.go", "gopathwalk") - env.References(loc) - env.Await(env.DoneWithOpen()) + tests := []struct { + repo string + file string + regexp string + }{ + {"istio", "pkg/config/model.go", "type (Meta)"}, + {"kubernetes", "pkg/controller/lookup_cache.go", "type (objectWithMeta)"}, + {"kuma", "pkg/events/interfaces.go", "type (Event)"}, + {"pkgsite", "internal/log/log.go", "func (Infof)"}, + {"starlark", "syntax/syntax.go", "type (Ident)"}, + {"tools", "internal/lsp/source/view.go", "type (Snapshot)"}, + } - b.ResetTimer() + for _, test := range tests { + b.Run(test.repo, func(b *testing.B) { + env := getRepo(b, test.repo).sharedEnv(b) + env.OpenFile(test.file) + loc := env.RegexpSearch(test.file, test.regexp) + env.Await(env.DoneWithOpen()) + env.References(loc) // pre-warm the query + b.ResetTimer() - for i := 0; i < b.N; i++ { - env.References(loc) + for i := 0; i < b.N; i++ { + env.References(loc) + } + }) } } diff --git a/gopls/internal/regtest/bench/rename_test.go b/gopls/internal/regtest/bench/rename_test.go index 7339c76251f..bd1ce94910c 100644 --- a/gopls/internal/regtest/bench/rename_test.go +++ b/gopls/internal/regtest/bench/rename_test.go @@ -10,16 +10,35 @@ import ( ) func BenchmarkRename(b *testing.B) { - env := repos["tools"].sharedEnv(b) - - env.OpenFile("internal/imports/mod.go") - env.Await(env.DoneWithOpen()) + tests := []struct { + repo string + file string + regexp string + baseName string + }{ + {"kubernetes", "pkg/controller/lookup_cache.go", `hashutil\.(DeepHashObject)`, "DeepHashObject"}, + {"kuma", "pkg/events/interfaces.go", `Delete`, "Delete"}, + {"istio", "pkg/config/model.go", `(Namespace) string`, "Namespace"}, + {"pkgsite", "internal/log/log.go", `func (Infof)`, "Infof"}, + {"starlark", "starlark/eval.go", `Program\) (Filename)`, "Filename"}, + {"tools", "internal/lsp/cache/snapshot.go", `meta \*(metadataGraph)`, "metadataGraph"}, + } - b.ResetTimer() + for _, test := range tests { + names := 0 // bench function may execute multiple times + b.Run(test.repo, func(b *testing.B) { + env := getRepo(b, test.repo).sharedEnv(b) + env.OpenFile(test.file) + loc := env.RegexpSearch(test.file, test.regexp) + env.Await(env.DoneWithOpen()) + env.Rename(loc, test.baseName+"X") // pre-warm the query + b.ResetTimer() - for i := 1; i < b.N; i++ { - loc := env.RegexpSearch("internal/imports/mod.go", "gopathwalk") - newName := fmt.Sprintf("%s%d", "gopathwalk", i) - env.Rename(loc, newName) + for i := 0; i < b.N; i++ { + names++ + newName := fmt.Sprintf("%s%d", test.baseName, names) + env.Rename(loc, newName) + } + }) } } diff --git a/gopls/internal/regtest/bench/repo_test.go b/gopls/internal/regtest/bench/repo_test.go index 9b7ce72c625..5ca24ec90ac 100644 --- a/gopls/internal/regtest/bench/repo_test.go +++ b/gopls/internal/regtest/bench/repo_test.go @@ -14,14 +14,77 @@ import ( "path/filepath" "sync" "testing" + "time" "golang.org/x/tools/gopls/internal/lsp/fake" . "golang.org/x/tools/gopls/internal/lsp/regtest" ) // repos holds shared repositories for use in benchmarks. +// +// These repos were selected to represent a variety of different types of +// codebases. var repos = map[string]*repo{ - "tools": {name: "tools", url: "https://go.googlesource.com/tools", commit: "gopls/v0.9.0"}, + // Used by x/benchmarks; large. + "istio": { + name: "istio", + url: "https://github.com/istio/istio", + commit: "1.17.0", + }, + + // Kubernetes is a large repo with many dependencies, and in the past has + // been about as large a repo as gopls could handle. + "kubernetes": { + name: "kubernetes", + url: "https://github.com/kubernetes/kubernetes", + commit: "v1.24.0", + }, + + // A large, industrial application. + "kuma": { + name: "kuma", + url: "https://github.com/kumahq/kuma", + commit: "2.1.1", + }, + + // x/pkgsite is familiar and represents a common use case (a webserver). It + // also has a number of static non-go files and template files. + "pkgsite": { + name: "pkgsite", + url: "https://go.googlesource.com/pkgsite", + commit: "81f6f8d4175ad0bf6feaa03543cc433f8b04b19b", + short: true, + }, + + // A tiny self-contained project. + "starlark": { + name: "starlark", + url: "https://github.com/google/starlark-go", + commit: "3f75dec8e4039385901a30981e3703470d77e027", + short: true, + }, + + // The current repository, which is medium-small and has very few dependencies. + "tools": { + name: "tools", + url: "https://go.googlesource.com/tools", + commit: "gopls/v0.9.0", + short: true, + }, +} + +// getRepo gets the requested repo, and skips the test if -short is set and +// repo is not configured as a short repo. +func getRepo(tb testing.TB, name string) *repo { + tb.Helper() + repo := repos[name] + if repo == nil { + tb.Fatalf("repo %s does not exist", name) + } + if !repo.short && testing.Short() { + tb.Skipf("large repo %s does not run whith -short", repo.name) + } + return repo } // A repo represents a working directory for a repository checked out at a @@ -33,7 +96,8 @@ type repo struct { // static configuration name string // must be unique, used for subdirectory url string // repo url - commit string // commitish, e.g. tag or short commit hash + commit string // full commit hash or tag + short bool // whether this repo runs with -short dirOnce sync.Once dir string // directory contaning source code checked out to url@commit @@ -71,6 +135,8 @@ func (r *repo) sharedEnv(tb testing.TB) *Env { r.editorOnce.Do(func() { dir := r.getDir() + start := time.Now() + log.Printf("starting initial workspace load for %s", r.name) ts, err := newGoplsServer(r.name) if err != nil { log.Fatal(err) @@ -83,6 +149,7 @@ func (r *repo) sharedEnv(tb testing.TB) *Env { if err := r.awaiter.Await(context.Background(), InitialWorkspaceLoad); err != nil { log.Fatal(err) } + log.Printf("initial workspace load (cold) for %s took %v", r.name, time.Since(start)) }) return &Env{ @@ -99,14 +166,14 @@ func (r *repo) sharedEnv(tb testing.TB) *Env { // // It is the caller's responsibility to call Close on the resulting Env when it // is no longer needed. -func (r *repo) newEnv(tb testing.TB) *Env { +func (r *repo) newEnv(tb testing.TB, name string, config fake.EditorConfig) *Env { dir := r.getDir() - ts, err := newGoplsServer(tb.Name()) + ts, err := newGoplsServer(name) if err != nil { tb.Fatal(err) } - sandbox, editor, awaiter, err := connectEditor(dir, fake.EditorConfig{}, ts) + sandbox, editor, awaiter, err := connectEditor(dir, config, ts) if err != nil { log.Fatalf("connecting editor: %v", err) } diff --git a/gopls/internal/regtest/bench/workspace_symbols_test.go b/gopls/internal/regtest/bench/workspace_symbols_test.go index ac9ad531b1f..975422ac651 100644 --- a/gopls/internal/regtest/bench/workspace_symbols_test.go +++ b/gopls/internal/regtest/bench/workspace_symbols_test.go @@ -15,21 +15,23 @@ var symbolQuery = flag.String("symbol_query", "test", "symbol query to use in be // BenchmarkWorkspaceSymbols benchmarks the time to execute a workspace symbols // request (controlled by the -symbol_query flag). func BenchmarkWorkspaceSymbols(b *testing.B) { - env := repos["tools"].sharedEnv(b) - - // Make an initial symbol query to warm the cache. - symbols := env.Symbol(*symbolQuery) - - if testing.Verbose() { - fmt.Println("Results:") - for i := 0; i < len(symbols); i++ { - fmt.Printf("\t%d. %s (%s)\n", i, symbols[i].Name, symbols[i].ContainerName) - } - } - - b.ResetTimer() - - for i := 0; i < b.N; i++ { - env.Symbol(*symbolQuery) + for name := range repos { + b.Run(name, func(b *testing.B) { + env := getRepo(b, name).sharedEnv(b) + symbols := env.Symbol(*symbolQuery) // warm the cache + + if testing.Verbose() { + fmt.Println("Results:") + for i, symbol := range symbols { + fmt.Printf("\t%d. %s (%s)\n", i, symbol.Name, symbol.ContainerName) + } + } + + b.ResetTimer() + + for i := 0; i < b.N; i++ { + env.Symbol(*symbolQuery) + } + }) } }