gopls: commands to start/stop profiling, and a new benchmark metric
Total CPU used by gopls is a critical metric for our users, yet was not
previously captured in any benchmark. This change uses the new pprof
parsing utilities added in CL 507885 to instrument a cpu_seconds
benchmark metric, for now just associated with the DidChange benchmark.
This is achieved via new LSP commands that start and stop a profile. The
benchmark runner uses these commands to bracket the critical section of
the benchmark, then parses the resulting profile for its total sampled
CPU.
Additionally, the benchmark runner is updated to actually check for the
existence of the custom command before instrumenting the new metric.
This allows it to be compatible with testing older versions of gopls.
The same technique is adopted for memstats metrics.
I only instrumented BenchmarkDidChange, because the profile file schema
is getting truly out of hand. I'll try to simplify it before
instrumenting all the other benchmarks, but want to do that in a
separate CL.
For golang/go#60926
Change-Id: Ia082bad49e8d30c567a7c07e050511d49b93738b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/508449
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
diff --git a/gopls/doc/commands.md b/gopls/doc/commands.md
index b259f63..eff548a 100644
--- a/gopls/doc/commands.md
+++ b/gopls/doc/commands.md
@@ -364,14 +364,14 @@
// Optional: the address (including port) for the debug server to listen on.
// If not provided, the debug server will bind to "localhost:0", and the
// full debug URL will be contained in the result.
- //
+ //
// If there is more than one gopls instance along the serving path (i.e. you
// are using a daemon), each gopls instance will attempt to start debugging.
// If Addr specifies a port, only the daemon will be able to bind to that
// port, and each intermediate gopls instance will fail to start debugging.
// For this reason it is recommended not to specify a port (or equivalently,
// to specify ":0").
- //
+ //
// If the server was already debugging this field has no effect, and the
// result will contain the previously configured debug URL(s).
"Addr": string,
@@ -385,7 +385,7 @@
// The URLs to use to access the debug servers, for all gopls instances in
// the serving path. For the common case of a single gopls instance (i.e. no
// daemon), this will be exactly one address.
- //
+ //
// In the case of one or more gopls instances forwarding the LSP to a daemon,
// URLs will contain debug addresses for each server in the serving path, in
// serving order. The daemon debug address will be the last entry in the
@@ -396,6 +396,48 @@
}
```
+### **start capturing a profile of gopls' execution.**
+Identifier: `gopls.start_profile`
+
+Start a new pprof profile. Before using the resulting file, profiling must
+be stopped with a corresponding call to StopProfile.
+
+This command is intended for internal use only, by the gopls benchmark
+runner.
+
+Args:
+
+```
+struct{}
+```
+
+Result:
+
+```
+struct{}
+```
+
+### **stop an ongoing profile.**
+Identifier: `gopls.stop_profile`
+
+This command is intended for internal use only, by the gopls benchmark
+runner.
+
+Args:
+
+```
+struct{}
+```
+
+Result:
+
+```
+{
+ // File is the profile file name.
+ "File": string,
+}
+```
+
### **Run test(s) (legacy)**
Identifier: `gopls.test`
diff --git a/gopls/doc/generate.go b/gopls/doc/generate.go
index 332faeb..f7e6997 100644
--- a/gopls/doc/generate.go
+++ b/gopls/doc/generate.go
@@ -465,7 +465,11 @@
if fld.Doc != "" && level == 0 {
doclines := strings.Split(fld.Doc, "\n")
for _, line := range doclines {
- fmt.Fprintf(&b, "%s\t// %s\n", indent, line)
+ text := ""
+ if line != "" {
+ text = " " + line
+ }
+ fmt.Fprintf(&b, "%s\t//%s\n", indent, text)
}
}
tag := strings.Split(fld.JSONTag, ",")[0]
diff --git a/gopls/internal/lsp/command.go b/gopls/internal/lsp/command.go
index 7c6bcfa..ff64670 100644
--- a/gopls/internal/lsp/command.go
+++ b/gopls/internal/lsp/command.go
@@ -16,6 +16,7 @@
"os/exec"
"path/filepath"
"runtime"
+ "runtime/pprof"
"sort"
"strings"
"time"
@@ -843,6 +844,48 @@
return result, nil
}
+func (c *commandHandler) StartProfile(ctx context.Context, args command.StartProfileArgs) (result command.StartProfileResult, _ error) {
+ file, err := os.CreateTemp("", "gopls-profile-*")
+ if err != nil {
+ return result, fmt.Errorf("creating temp profile file: %v", err)
+ }
+
+ c.s.ongoingProfileMu.Lock()
+ defer c.s.ongoingProfileMu.Unlock()
+
+ if c.s.ongoingProfile != nil {
+ file.Close() // ignore error
+ return result, fmt.Errorf("profile already started (for %q)", c.s.ongoingProfile.Name())
+ }
+
+ if err := pprof.StartCPUProfile(file); err != nil {
+ file.Close() // ignore error
+ return result, fmt.Errorf("starting profile: %v", err)
+ }
+
+ c.s.ongoingProfile = file
+ return result, nil
+}
+
+func (c *commandHandler) StopProfile(ctx context.Context, args command.StopProfileArgs) (result command.StopProfileResult, _ error) {
+ c.s.ongoingProfileMu.Lock()
+ defer c.s.ongoingProfileMu.Unlock()
+
+ prof := c.s.ongoingProfile
+ c.s.ongoingProfile = nil
+
+ if prof == nil {
+ return result, fmt.Errorf("no ongoing profile")
+ }
+
+ pprof.StopCPUProfile()
+ if err := prof.Close(); err != nil {
+ return result, fmt.Errorf("closing profile file: %v", err)
+ }
+ result.File = prof.Name()
+ return result, nil
+}
+
// Copy of pkgLoadConfig defined in internal/lsp/cmd/vulncheck.go
// TODO(hyangah): decide where to define this.
type pkgLoadConfig struct {
diff --git a/gopls/internal/lsp/command/command_gen.go b/gopls/internal/lsp/command/command_gen.go
index 8003b17..25a101c 100644
--- a/gopls/internal/lsp/command/command_gen.go
+++ b/gopls/internal/lsp/command/command_gen.go
@@ -38,6 +38,8 @@
RunGovulncheck Command = "run_govulncheck"
RunTests Command = "run_tests"
StartDebugging Command = "start_debugging"
+ StartProfile Command = "start_profile"
+ StopProfile Command = "stop_profile"
Test Command = "test"
Tidy Command = "tidy"
ToggleGCDetails Command = "toggle_gc_details"
@@ -67,6 +69,8 @@
RunGovulncheck,
RunTests,
StartDebugging,
+ StartProfile,
+ StopProfile,
Test,
Tidy,
ToggleGCDetails,
@@ -188,6 +192,18 @@
return nil, err
}
return s.StartDebugging(ctx, a0)
+ case "gopls.start_profile":
+ var a0 StartProfileArgs
+ if err := UnmarshalArgs(params.Arguments, &a0); err != nil {
+ return nil, err
+ }
+ return s.StartProfile(ctx, a0)
+ case "gopls.stop_profile":
+ var a0 StopProfileArgs
+ if err := UnmarshalArgs(params.Arguments, &a0); err != nil {
+ return nil, err
+ }
+ return s.StopProfile(ctx, a0)
case "gopls.test":
var a0 protocol.DocumentURI
var a1 []string
@@ -460,6 +476,30 @@
}, nil
}
+func NewStartProfileCommand(title string, a0 StartProfileArgs) (protocol.Command, error) {
+ args, err := MarshalArgs(a0)
+ if err != nil {
+ return protocol.Command{}, err
+ }
+ return protocol.Command{
+ Title: title,
+ Command: "gopls.start_profile",
+ Arguments: args,
+ }, nil
+}
+
+func NewStopProfileCommand(title string, a0 StopProfileArgs) (protocol.Command, error) {
+ args, err := MarshalArgs(a0)
+ if err != nil {
+ return protocol.Command{}, err
+ }
+ return protocol.Command{
+ Title: title,
+ Command: "gopls.stop_profile",
+ Arguments: args,
+ }, nil
+}
+
func NewTestCommand(title string, a0 protocol.DocumentURI, a1 []string, a2 []string) (protocol.Command, error) {
args, err := MarshalArgs(a0, a1, a2)
if err != nil {
diff --git a/gopls/internal/lsp/command/interface.go b/gopls/internal/lsp/command/interface.go
index ababac6..ef9d1fb 100644
--- a/gopls/internal/lsp/command/interface.go
+++ b/gopls/internal/lsp/command/interface.go
@@ -145,6 +145,21 @@
// address.
StartDebugging(context.Context, DebuggingArgs) (DebuggingResult, error)
+ // StartProfile: start capturing a profile of gopls' execution.
+ //
+ // Start a new pprof profile. Before using the resulting file, profiling must
+ // be stopped with a corresponding call to StopProfile.
+ //
+ // This command is intended for internal use only, by the gopls benchmark
+ // runner.
+ StartProfile(context.Context, StartProfileArgs) (StartProfileResult, error)
+
+ // StopProfile: stop an ongoing profile.
+ //
+ // This command is intended for internal use only, by the gopls benchmark
+ // runner.
+ StopProfile(context.Context, StopProfileArgs) (StopProfileResult, error)
+
// RunGovulncheck: Run govulncheck.
//
// Run vulnerability check (`govulncheck`).
@@ -327,6 +342,30 @@
URLs []string
}
+// StartProfileArgs holds the arguments to the StartProfile command.
+//
+// It is a placeholder for future compatibility.
+type StartProfileArgs struct {
+}
+
+// StartProfileResult holds the result of the StartProfile command.
+//
+// It is a placeholder for future compatibility.
+type StartProfileResult struct {
+}
+
+// StopProfileArgs holds the arguments to the StopProfile command.
+//
+// It is a placeholder for future compatibility.
+type StopProfileArgs struct {
+}
+
+// StopProfileResult holds the result to the StopProfile command.
+type StopProfileResult struct {
+ // File is the profile file name.
+ File string
+}
+
type ResetGoModDiagnosticsArgs struct {
URIArg
diff --git a/gopls/internal/lsp/fake/editor.go b/gopls/internal/lsp/fake/editor.go
index 45def8f..b6e507c 100644
--- a/gopls/internal/lsp/fake/editor.go
+++ b/gopls/internal/lsp/fake/editor.go
@@ -305,6 +305,16 @@
return nil
}
+// HasCommand reports whether the connected server supports the command with the given ID.
+func (e *Editor) HasCommand(id string) bool {
+ for _, command := range e.serverCapabilities.ExecuteCommandProvider.Commands {
+ if command == id {
+ return true
+ }
+ }
+ return false
+}
+
// makeWorkspaceFolders creates a slice of workspace folders to use for
// this editing session, based on the editor configuration.
func makeWorkspaceFolders(sandbox *Sandbox, paths []string) (folders []protocol.WorkspaceFolder) {
diff --git a/gopls/internal/lsp/regtest/wrappers.go b/gopls/internal/lsp/regtest/wrappers.go
index 5d5d2f7..d0df086 100644
--- a/gopls/internal/lsp/regtest/wrappers.go
+++ b/gopls/internal/lsp/regtest/wrappers.go
@@ -370,6 +370,41 @@
}
}
+// StartProfile starts a CPU profile with the given name, using the
+// gopls.start_profile custom command. It calls t.Fatal on any error.
+//
+// The resulting stop function must be called to stop profiling (using the
+// gopls.stop_profile custom command).
+func (e *Env) StartProfile() (stop func() string) {
+ // TODO(golang/go#61217): revisit the ergonomics of these command APIs.
+ //
+ // This would be a lot simpler if we generated params constructors.
+ args, err := command.MarshalArgs(command.StartProfileArgs{})
+ if err != nil {
+ e.T.Fatal(err)
+ }
+ params := &protocol.ExecuteCommandParams{
+ Command: command.StartProfile.ID(),
+ Arguments: args,
+ }
+ var result command.StartProfileResult
+ e.ExecuteCommand(params, &result)
+
+ return func() string {
+ stopArgs, err := command.MarshalArgs(command.StopProfileArgs{})
+ if err != nil {
+ e.T.Fatal(err)
+ }
+ stopParams := &protocol.ExecuteCommandParams{
+ Command: command.StopProfile.ID(),
+ Arguments: stopArgs,
+ }
+ var result command.StopProfileResult
+ e.ExecuteCommand(stopParams, &result)
+ return result.File
+ }
+}
+
// InlayHints calls textDocument/inlayHints for the given path, calling t.Fatal on
// any error.
func (e *Env) InlayHints(path string) []protocol.InlayHint {
diff --git a/gopls/internal/lsp/server.go b/gopls/internal/lsp/server.go
index db69565..79d16e1 100644
--- a/gopls/internal/lsp/server.go
+++ b/gopls/internal/lsp/server.go
@@ -10,6 +10,7 @@
import (
"context"
"fmt"
+ "os"
"sync"
"golang.org/x/tools/gopls/internal/lsp/cache"
@@ -109,6 +110,11 @@
// report with an error message.
criticalErrorStatusMu sync.Mutex
criticalErrorStatus *progress.WorkDone
+
+ // Track an ongoing CPU profile created with the StartProfile command and
+ // terminated with the StopProfile command.
+ ongoingProfileMu sync.Mutex
+ ongoingProfile *os.File // if non-nil, an ongoing profile is writing to this file
}
func (s *Server) workDoneProgressCancel(ctx context.Context, params *protocol.WorkDoneProgressCancelParams) error {
diff --git a/gopls/internal/lsp/source/api_json.go b/gopls/internal/lsp/source/api_json.go
index f777fdb..1d9e703 100644
--- a/gopls/internal/lsp/source/api_json.go
+++ b/gopls/internal/lsp/source/api_json.go
@@ -791,8 +791,22 @@
Command: "gopls.start_debugging",
Title: "Start the gopls debug server",
Doc: "Start the gopls debug server if it isn't running, and return the debug\naddress.",
- ArgDoc: "{\n\t// Optional: the address (including port) for the debug server to listen on.\n\t// If not provided, the debug server will bind to \"localhost:0\", and the\n\t// full debug URL will be contained in the result.\n\t// \n\t// If there is more than one gopls instance along the serving path (i.e. you\n\t// are using a daemon), each gopls instance will attempt to start debugging.\n\t// If Addr specifies a port, only the daemon will be able to bind to that\n\t// port, and each intermediate gopls instance will fail to start debugging.\n\t// For this reason it is recommended not to specify a port (or equivalently,\n\t// to specify \":0\").\n\t// \n\t// If the server was already debugging this field has no effect, and the\n\t// result will contain the previously configured debug URL(s).\n\t\"Addr\": string,\n}",
- ResultDoc: "{\n\t// The URLs to use to access the debug servers, for all gopls instances in\n\t// the serving path. For the common case of a single gopls instance (i.e. no\n\t// daemon), this will be exactly one address.\n\t// \n\t// In the case of one or more gopls instances forwarding the LSP to a daemon,\n\t// URLs will contain debug addresses for each server in the serving path, in\n\t// serving order. The daemon debug address will be the last entry in the\n\t// slice. If any intermediate gopls instance fails to start debugging, no\n\t// error will be returned but the debug URL for that server in the URLs slice\n\t// will be empty.\n\t\"URLs\": []string,\n}",
+ ArgDoc: "{\n\t// Optional: the address (including port) for the debug server to listen on.\n\t// If not provided, the debug server will bind to \"localhost:0\", and the\n\t// full debug URL will be contained in the result.\n\t//\n\t// If there is more than one gopls instance along the serving path (i.e. you\n\t// are using a daemon), each gopls instance will attempt to start debugging.\n\t// If Addr specifies a port, only the daemon will be able to bind to that\n\t// port, and each intermediate gopls instance will fail to start debugging.\n\t// For this reason it is recommended not to specify a port (or equivalently,\n\t// to specify \":0\").\n\t//\n\t// If the server was already debugging this field has no effect, and the\n\t// result will contain the previously configured debug URL(s).\n\t\"Addr\": string,\n}",
+ ResultDoc: "{\n\t// The URLs to use to access the debug servers, for all gopls instances in\n\t// the serving path. For the common case of a single gopls instance (i.e. no\n\t// daemon), this will be exactly one address.\n\t//\n\t// In the case of one or more gopls instances forwarding the LSP to a daemon,\n\t// URLs will contain debug addresses for each server in the serving path, in\n\t// serving order. The daemon debug address will be the last entry in the\n\t// slice. If any intermediate gopls instance fails to start debugging, no\n\t// error will be returned but the debug URL for that server in the URLs slice\n\t// will be empty.\n\t\"URLs\": []string,\n}",
+ },
+ {
+ Command: "gopls.start_profile",
+ Title: "start capturing a profile of gopls' execution.",
+ Doc: "Start a new pprof profile. Before using the resulting file, profiling must\nbe stopped with a corresponding call to StopProfile.\n\nThis command is intended for internal use only, by the gopls benchmark\nrunner.",
+ ArgDoc: "struct{}",
+ ResultDoc: "struct{}",
+ },
+ {
+ Command: "gopls.stop_profile",
+ Title: "stop an ongoing profile.",
+ Doc: "This command is intended for internal use only, by the gopls benchmark\nrunner.",
+ ArgDoc: "struct{}",
+ ResultDoc: "{\n\t// File is the profile file name.\n\t\"File\": string,\n}",
},
{
Command: "gopls.test",
diff --git a/gopls/internal/regtest/bench/bench_test.go b/gopls/internal/regtest/bench/bench_test.go
index 28eec27..b486a0c 100644
--- a/gopls/internal/regtest/bench/bench_test.go
+++ b/gopls/internal/regtest/bench/bench_test.go
@@ -5,9 +5,12 @@
package bench
import (
+ "bytes"
+ "compress/gzip"
"context"
"flag"
"fmt"
+ "io"
"io/ioutil"
"log"
"os"
@@ -20,11 +23,14 @@
"golang.org/x/tools/gopls/internal/bug"
"golang.org/x/tools/gopls/internal/hooks"
"golang.org/x/tools/gopls/internal/lsp/cmd"
+ "golang.org/x/tools/gopls/internal/lsp/command"
"golang.org/x/tools/gopls/internal/lsp/fake"
+ "golang.org/x/tools/gopls/internal/lsp/regtest"
"golang.org/x/tools/internal/event"
"golang.org/x/tools/internal/fakenet"
"golang.org/x/tools/internal/jsonrpc2"
"golang.org/x/tools/internal/jsonrpc2/servertest"
+ "golang.org/x/tools/internal/pprof"
"golang.org/x/tools/internal/tool"
. "golang.org/x/tools/gopls/internal/lsp/regtest"
@@ -251,3 +257,61 @@
return clientConn
}
+
+// startProfileIfSupported checks to see if the remote gopls instance supports
+// the start/stop profiling commands. If so, it starts profiling and returns a
+// function that stops profiling and records the total CPU seconds sampled in the
+// cpu_seconds benchmark metric.
+//
+// If the remote gopls instance does not support profiling commands, this
+// function returns nil.
+//
+// If the supplied userSuffix is non-empty, the profile is written to
+// <repo>.<userSuffix>, and not deleted when the benchmark exits. Otherwise,
+// the profile is written to a temp file that is deleted after the cpu_seconds
+// metric has been computed.
+func startProfileIfSupported(env *regtest.Env, repo, userSuffix string) func(*testing.B) {
+ if !env.Editor.HasCommand(command.StartProfile.ID()) {
+ return nil
+ }
+ stopProfile := env.StartProfile()
+ return func(b *testing.B) {
+ b.StopTimer()
+ profFile := stopProfile()
+ totalCPU, err := totalCPUForProfile(profFile)
+ if err != nil {
+ b.Fatalf("reading profile: %v", err)
+ }
+ b.ReportMetric(totalCPU.Seconds(), "cpu_seconds")
+ if userSuffix == "" {
+ // The user didn't request this profile file, so delete it to clean up.
+ if err := os.Remove(profFile); err != nil {
+ b.Errorf("removing profile file: %v", err)
+ }
+ } else {
+ // NOTE: if this proves unreliable (due to e.g. EXDEV), we can fall back
+ // on Read+Write+Remove.
+ if err := os.Rename(profFile, repo+"."+userSuffix); err != nil {
+ b.Fatalf("renaming profile file: %v", err)
+ }
+ }
+ }
+}
+
+// totalCPUForProfile reads the pprof profile with the given file name, parses,
+// and aggregates the total CPU sampled during the profile.
+func totalCPUForProfile(filename string) (time.Duration, error) {
+ protoGz, err := os.ReadFile(filename)
+ if err != nil {
+ return 0, err
+ }
+ rd, err := gzip.NewReader(bytes.NewReader(protoGz))
+ if err != nil {
+ return 0, fmt.Errorf("creating gzip reader for %s: %v", filename, err)
+ }
+ data, err := io.ReadAll(rd)
+ if err != nil {
+ return 0, fmt.Errorf("reading %s: %v", filename, err)
+ }
+ return pprof.TotalTime(data)
+}
diff --git a/gopls/internal/regtest/bench/didchange_test.go b/gopls/internal/regtest/bench/didchange_test.go
index 2030f32..ca736fa 100644
--- a/gopls/internal/regtest/bench/didchange_test.go
+++ b/gopls/internal/regtest/bench/didchange_test.go
@@ -5,6 +5,7 @@
package bench
import (
+ "flag"
"fmt"
"sync/atomic"
"testing"
@@ -14,6 +15,13 @@
"golang.org/x/tools/gopls/internal/lsp/protocol"
)
+var didChangeProfile = flag.String(
+ "didchange_cpuprofile",
+ "",
+ `If set, profile BenchmarkDidChange using this cpu profile suffix. Incompatible with gopls_cpuprofile.
+See "Profiling" in the package doc for more information.`,
+)
+
// Use a global edit counter as bench function may execute multiple times, and
// we want to avoid cache hits. Use time.Now to also avoid cache hits from the
// shared file cache.
@@ -48,6 +56,10 @@
env.AfterChange()
b.ResetTimer()
+ if stopAndRecord := startProfileIfSupported(env, test.repo, *didChangeProfile); stopAndRecord != nil {
+ defer stopAndRecord(b)
+ }
+
for i := 0; i < b.N; i++ {
edits := atomic.AddInt64(&editID, 1)
env.EditBuffer(test.file, protocol.TextEdit{
diff --git a/gopls/internal/regtest/bench/doc.go b/gopls/internal/regtest/bench/doc.go
index 8265437..08298ee 100644
--- a/gopls/internal/regtest/bench/doc.go
+++ b/gopls/internal/regtest/bench/doc.go
@@ -14,19 +14,34 @@
//
// # Profiling
//
-// As benchmark functions run gopls in a separate process, the normal test
-// flags for profiling are not useful. Instead the -gopls_cpuprofile,
+// Benchmark functions run gopls in a separate process, which means the normal
+// test flags for profiling aren't useful. Instead the -gopls_cpuprofile,
// -gopls_memprofile, -gopls_allocprofile, and -gopls_trace flags may be used
// to pass through profiling flags to the gopls process. Each of these flags
-// sets a suffix for the respective gopls profiling flag, which is prefixed
-// with a name corresponding to the shared repository or (in some cases)
-// benchmark name. For example, settings -gopls_cpuprofile=cpu.out will result
-// in profiles named tools.cpu.out, BenchmarkInitialWorkspaceLoad.cpu.out, etc.
-// Here, tools.cpu.out is the cpu profile for the shared x/tools session, which
-// may be used by multiple benchmark functions, and
-// BenchmarkInitialWorkspaceLoad is the cpu profile for the last iteration of
-// the initial workspace load test, which starts a new editor session for each
-// iteration.
+// sets a suffix for the respective gopls binary profiling flag, which is
+// prefixed with a name corresponding to the shared repository or (in some
+// cases) benchmark name. For example, setting -gopls_cpuprofile=cpu will
+// result in profiles named tools.cpu, iwl.tools.cpu, etc. Here, tools.cpu is
+// the cpu profile for the shared x/tools session, which may be used by
+// multiple benchmark functions, and iwl.tools.cpu is the cpu profile for the
+// last iteration of the initial workspace load test, which starts a new editor
+// session for each iteration.
+//
+// In some cases we want to collect profiles that are bracketed around the
+// specific inner operations being tested by the benchmark (for example
+// DidChange processing). To support this use-case, recent versions of gopls
+// include custom LSP commands to start and stop profiling. If these commands
+// are supported, the benchmark runner will instrument certain benchmarks to
+// collect profiles and compute a cpu_seconds benchmark metric recording the
+// total CPU sampled during the profile. These profile files may be specified
+// using benchmark-specific suffix flags, such as -didchange_cpuprofile, in
+// which case the profile files will be written according to the naming schema
+// described in the previous paragraph, and will not be deleted when the
+// benchmark exits. For example, setting -didchange_cpuprofile=change.cpu
+// results in a tools.change.cpu file created during BenchmarkDidChange/tools.
+//
+// TODO(rfindley): simplify these profiling flags to just have a single profile
+// per test that "does the right thing".
//
// # Integration with perf.golang.org
//
diff --git a/gopls/internal/regtest/bench/iwl_test.go b/gopls/internal/regtest/bench/iwl_test.go
index b7e9ad30..6e5f0ce 100644
--- a/gopls/internal/regtest/bench/iwl_test.go
+++ b/gopls/internal/regtest/bench/iwl_test.go
@@ -56,12 +56,11 @@
b.StartTimer()
// Note: in the future, we may need to open a file in order to cause gopls to
- // start loading. the workspace.
+ // start loading the workspace.
env.Await(InitialWorkspaceLoad)
- // TODO(rfindley): remove this guard once the released gopls version supports
- // the memstats command.
- if !testing.Short() {
+
+ if env.Editor.HasCommand(command.MemStats.ID()) {
b.StopTimer()
params := &protocol.ExecuteCommandParams{
Command: command.MemStats.ID(),
diff --git a/internal/tool/tool.go b/internal/tool/tool.go
index cf3b0a4..f4dd8d1 100644
--- a/internal/tool/tool.go
+++ b/internal/tool/tool.go
@@ -106,7 +106,7 @@
// Run is the inner loop for Main; invoked by Main, recursively by
// Run, and by various tests. It runs the application and returns an
// error.
-func Run(ctx context.Context, s *flag.FlagSet, app Application, args []string) error {
+func Run(ctx context.Context, s *flag.FlagSet, app Application, args []string) (resultErr error) {
s.Usage = func() {
if app.ShortHelp() != "" {
fmt.Fprintf(s.Output(), "%s\n\nUsage:\n ", app.ShortHelp())
@@ -133,9 +133,15 @@
return err
}
if err := pprof.StartCPUProfile(f); err != nil {
+ f.Close() // ignore error
return err
}
- defer pprof.StopCPUProfile()
+ defer func() {
+ pprof.StopCPUProfile()
+ if closeErr := f.Close(); resultErr == nil {
+ resultErr = closeErr
+ }
+ }()
}
if p != nil && p.Trace != "" {
@@ -144,10 +150,14 @@
return err
}
if err := trace.Start(f); err != nil {
+ f.Close() // ignore error
return err
}
defer func() {
trace.Stop()
+ if closeErr := f.Close(); resultErr == nil {
+ resultErr = closeErr
+ }
log.Printf("To view the trace, run:\n$ go tool trace view %s", p.Trace)
}()
}
