From 9a60c72cd5e8fbd3ea8adc7d4efd5b1644b8ba77 Mon Sep 17 00:00:00 2001 From: Will Roden Date: Tue, 29 Dec 2020 13:54:47 -0600 Subject: [PATCH 1/3] json out --- benchdiff.go | 110 ++++++++++++++++++++++++++------ benchdiff_test.go | 8 +-- cmd/benchdiff/benchdiff.go | 21 ++++-- pkg/benchstat/benchstat.go | 9 +-- pkg/benchstat/benchstat_test.go | 6 +- 5 files changed, 113 insertions(+), 41 deletions(-) diff --git a/benchdiff.go b/benchdiff.go index afab9ed..6efee53 100644 --- a/benchdiff.go +++ b/benchdiff.go @@ -2,6 +2,7 @@ package benchdiff import ( "bytes" + "encoding/json" "fmt" "io" "os" @@ -13,8 +14,8 @@ import ( "golang.org/x/perf/benchstat" ) -// Differ runs benchstats and outputs their deltas -type Differ struct { +// Benchdiff runs benchstats and outputs their deltas +type Benchdiff struct { BenchCmd string BenchArgs string ResultsDir string @@ -23,9 +24,10 @@ type Differ struct { Writer io.Writer Benchstat *pkgbenchstat.Benchstat Force bool + JSONOutput bool } -func (c *Differ) baseOutputFile() (string, error) { +func (c *Benchdiff) baseOutputFile() (string, error) { runner := &gitRunner{ repoPath: c.Path, } @@ -39,7 +41,9 @@ func (c *Differ) baseOutputFile() (string, error) { } type runBenchmarksResults struct { - worktreeOutputFile, baseOutputFile string + worktreeOutputFile string + baseOutputFile string + benchmarkCmd string } func fileExists(path string) bool { @@ -50,7 +54,8 @@ func fileExists(path string) bool { return true } -func (c *Differ) runBenchmarks() (result *runBenchmarksResults, err error) { +func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) { + result = new(runBenchmarksResults) worktreeFilename := filepath.Join(c.ResultsDir, "benchstatter-worktree.out") worktreeFile, err := os.Create(worktreeFilename) if err != nil { @@ -64,7 +69,7 @@ func (c *Differ) runBenchmarks() (result *runBenchmarksResults, err error) { }() cmd := exec.Command(c.BenchCmd, strings.Fields(c.BenchArgs)...) //nolint:gosec // this is fine - fmt.Println(c.BenchArgs) + result.benchmarkCmd = cmd.String() cmd.Stdout = worktreeFile err = cmd.Run() if err != nil { @@ -76,10 +81,8 @@ func (c *Differ) runBenchmarks() (result *runBenchmarksResults, err error) { return nil, err } - result = &runBenchmarksResults{ - worktreeOutputFile: worktreeFilename, - baseOutputFile: baseFilename, - } + result.baseOutputFile = baseFilename + result.worktreeOutputFile = worktreeFilename if fileExists(baseFilename) && !c.Force { return result, nil @@ -120,8 +123,8 @@ func (c *Differ) runBenchmarks() (result *runBenchmarksResults, err error) { return result, nil } -// Run runs the Differ -func (c *Differ) Run() (*RunResult, error) { +// Run runs the Benchdiff +func (c *Benchdiff) Run() (*RunResult, error) { err := os.MkdirAll(c.ResultsDir, 0o700) if err != nil { return nil, err @@ -135,19 +138,88 @@ func (c *Differ) Run() (*RunResult, error) { return nil, err } result := &RunResult{ - tables: collection.Tables(), + benchCmd: res.benchmarkCmd, + tables: collection.Tables(), } return result, nil } -// OutputResult outputs a Run result -func (c *Differ) OutputResult(runResult *RunResult) error { - return c.Benchstat.OutputTables(runResult.tables) -} - // RunResult is the result of a Run type RunResult struct { - tables []*benchstat.Table + benchCmd string + tables []*benchstat.Table +} + +// RunResultOutputOptions options for RunResult.WriteOutput +type RunResultOutputOptions struct { + BenchstatFormatter pkgbenchstat.OutputFormatter // default benchstat.TextFormatter(nil) + OutputFormat string // one of json or human. default: human +} + +// WriteOutput outputs the result +func (r *RunResult) WriteOutput(w io.Writer, opts *RunResultOutputOptions) error { + if opts == nil { + opts = new(RunResultOutputOptions) + } + finalOpts := &RunResultOutputOptions{ + BenchstatFormatter: pkgbenchstat.TextFormatter(nil), + OutputFormat: "human", + } + if opts.BenchstatFormatter != nil { + finalOpts.BenchstatFormatter = opts.BenchstatFormatter + } + + if opts.OutputFormat != "" { + finalOpts.OutputFormat = opts.OutputFormat + } + + var benchstatBuf bytes.Buffer + err := finalOpts.BenchstatFormatter(&benchstatBuf, r.tables) + if err != nil { + return err + } + + var fn func(io.Writer, string) error + switch finalOpts.OutputFormat { + case "human": + fn = r.writeHumanResult + case "json": + fn = r.writeJSONResult + default: + return fmt.Errorf("unknown OutputFormat") + } + return fn(w, benchstatBuf.String()) +} + +func (r *RunResult) writeJSONResult(w io.Writer, benchstatResult string) error { + type runResultJSON struct { + BenchCommand string `json:"bench_command,omitempty"` + BenchstatResult string `json:"benchstat_result,omitempty"` + } + o := runResultJSON{ + BenchCommand: r.benchCmd, + BenchstatResult: benchstatResult, + } + encoder := json.NewEncoder(w) + encoder.SetIndent("", " ") + return encoder.Encode(&o) +} + +func (r *RunResult) writeHumanResult(w io.Writer, benchstatResult string) error { + var err error + if r.benchCmd != "" { + _, err = fmt.Fprintf(w, "bench command:\n %s\n", r.benchCmd) + if err != nil { + return err + } + } + if benchstatResult != "" { + _, err = fmt.Fprintf(w, "result:\n\n%s\n", benchstatResult) + if err != nil { + return err + } + } + return nil } // HasChangeType returns true if the result has at least one change with the given type diff --git a/benchdiff_test.go b/benchdiff_test.go index 4ce5013..e050d5b 100644 --- a/benchdiff_test.go +++ b/benchdiff_test.go @@ -40,11 +40,11 @@ func testInDir(t *testing.T, dir string) { }) } -func TestDiffer_Run(t *testing.T) { +func TestBenchstat_Run(t *testing.T) { dir := tmpDir(t) setupTestRepo(t, dir) testInDir(t, dir) - differ := Differ{ + differ := Benchdiff{ BenchCmd: "go", BenchArgs: "test -bench . -benchmem -count 10 -benchtime 10x .", ResultsDir: "./tmp", @@ -52,9 +52,7 @@ func TestDiffer_Run(t *testing.T) { Path: ".", Benchstat: &benchstat.Benchstat{}, } - result, err := differ.Run() - require.NoError(t, err) - err = differ.OutputResult(result) + _, err := differ.Run() require.NoError(t, err) } diff --git a/cmd/benchdiff/benchdiff.go b/cmd/benchdiff/benchdiff.go index ed4c99c..c692614 100644 --- a/cmd/benchdiff/benchdiff.go +++ b/cmd/benchdiff/benchdiff.go @@ -18,12 +18,12 @@ const defaultBenchArgsTmpl = `test -bench {{.Bench}} -run '^$' -benchmem -count var benchstatVars = kong.Vars{ "AlphaDefault": "0.05", "AlphaHelp": `consider change significant if p < α (default 0.05)`, - "CSVHelp": `print results in CSV form`, + "CSVHelp": `format benchstat results as CSV`, "DeltaTestHelp": `significance test to apply to delta: utest, ttest, or none`, "DeltaTestDefault": `utest`, "DeltaTestEnum": `utest,ttest,none`, "GeomeanHelp": `print the geometric mean of each file`, - "HTMLHelp": `print results as an HTML table`, + "HTMLHelp": `format benchstat results as CSV an HTML table`, "NorangeHelp": `suppress range columns (CSV only)`, "ReverseSortHelp": `reverse sort order`, "SortHelp": `sort by order: delta, name, none`, @@ -57,6 +57,7 @@ var benchVars = kong.Vars{ "BaseRefHelp": `The git ref to be used as a baseline.`, "ForceBaseHelp": `Rerun benchmarks on the base reference even if the output already exists.`, "DegradationExitHelp": `Exit code when there is a degradation in the results.`, + "JSONOutputHelp": `Format output as JSON. When true the --csv and --html flags affect only the "benchstat_output" field.`, } var cli struct { @@ -68,7 +69,8 @@ var cli struct { ForceBase bool `kong:"help=${ForceBaseHelp}"` Packages string `kong:"default='./...',help=${PackagesHelp}"` ResultsDir string `kong:"type=dir,default=${ResultsDirDefault},help=${ResultsDirHelp}"` - DegradationExit int `kong:"type=on-degradation,default=0,help=${DegradationExitHelp}"` + DegradationExit int `kong:"name=on-degradation,default=0,help=${DegradationExitHelp}"` + JSONOutput bool `kong:"help=${JSONOutputHelp}"` BenchstatOpts benchstatOpts `kong:"embed"` } @@ -80,7 +82,7 @@ func main() { err = tmpl.Execute(&benchArgs, cli) kctx.FatalIfErrorf(err) - differ := &benchdiff.Differ{ + differ := &benchdiff.Benchdiff{ BenchCmd: cli.BenchCmd, BenchArgs: benchArgs.String(), ResultsDir: cli.ResultsDir, @@ -92,7 +94,16 @@ func main() { } result, err := differ.Run() kctx.FatalIfErrorf(err) - err = differ.OutputResult(result) + + outputFormat := "human" + if cli.JSONOutput { + outputFormat = "json" + } + + err = result.WriteOutput(os.Stdout, &benchdiff.RunResultOutputOptions{ + BenchstatFormatter: buildBenchstat(cli.BenchstatOpts).OutputFormatter, + OutputFormat: outputFormat, + }) kctx.FatalIfErrorf(err) if result.HasChangeType(benchdiff.DegradingChange) { os.Exit(cli.DegradationExit) diff --git a/pkg/benchstat/benchstat.go b/pkg/benchstat/benchstat.go index 0e97a1d..9e5fb64 100644 --- a/pkg/benchstat/benchstat.go +++ b/pkg/benchstat/benchstat.go @@ -37,9 +37,6 @@ type Benchstat struct { // OutputFormatter determines how the output will be formatted. Default is TextFormatter OutputFormatter OutputFormatter - - // Writer is where to write output. Default is stdout. - Writer io.Writer } // OutputFormatter formats benchstat output @@ -72,11 +69,7 @@ func (b *Benchstat) Run(files ...string) (*benchstat.Collection, error) { } // OutputTables outputs the results from tables using b.OutputFormatter -func (b *Benchstat) OutputTables(tables []*benchstat.Table) error { - writer := b.Writer - if writer == nil { - writer = os.Stdout - } +func (b *Benchstat) OutputTables(writer io.Writer, tables []*benchstat.Table) error { formatter := b.OutputFormatter if formatter == nil { formatter = TextFormatter(nil) diff --git a/pkg/benchstat/benchstat_test.go b/pkg/benchstat/benchstat_test.go index b31f682..8f64510 100644 --- a/pkg/benchstat/benchstat_test.go +++ b/pkg/benchstat/benchstat_test.go @@ -12,12 +12,10 @@ func TestBenchstat_Run(t *testing.T) { worktreeFile := filepath.FromSlash("./testdata/outputs/benchstatter-worktree.out") baseFile := filepath.FromSlash("./testdata/outputs/benchstatter-1.out") var buf bytes.Buffer - bs := &Benchstat{ - Writer: &buf, - } + bs := &Benchstat{} collection, err := bs.Run(worktreeFile, baseFile) require.NoError(t, err) - err = bs.OutputTables(collection.Tables()) + err = bs.OutputTables(&buf, collection.Tables()) require.NoError(t, err) want := `name old time/op new time/op delta From 1ea70fe721781cfd7c06a88fbba107a4e1ee723b Mon Sep 17 00:00:00 2001 From: Will Roden Date: Tue, 29 Dec 2020 15:17:51 -0600 Subject: [PATCH 2/3] add baseSHA and headSHA to output --- benchdiff.go | 95 +++++++++++-------- gitrunner.go | 16 +++- pkg/benchstat/benchstat_test.go | 4 +- .../{benchstatter-1.out => benchdiff-1.out} | 4 +- ...er-worktree.out => benchdiff-worktree.out} | 4 +- 5 files changed, 77 insertions(+), 46 deletions(-) rename pkg/benchstat/testdata/outputs/{benchstatter-1.out => benchdiff-1.out} (88%) rename pkg/benchstat/testdata/outputs/{benchstatter-worktree.out => benchdiff-worktree.out} (88%) diff --git a/benchdiff.go b/benchdiff.go index 6efee53..3c2b23c 100644 --- a/benchdiff.go +++ b/benchdiff.go @@ -27,23 +27,12 @@ type Benchdiff struct { JSONOutput bool } -func (c *Benchdiff) baseOutputFile() (string, error) { - runner := &gitRunner{ - repoPath: c.Path, - } - revision, err := runner.run("rev-parse", c.BaseRef) - if err != nil { - return "", err - } - revision = bytes.TrimSpace(revision) - name := fmt.Sprintf("benchstatter-%s.out", string(revision)) - return filepath.Join(c.ResultsDir, name), nil -} - type runBenchmarksResults struct { worktreeOutputFile string baseOutputFile string benchmarkCmd string + headSHA string + baseSHA string } func fileExists(path string) bool { @@ -54,9 +43,24 @@ func fileExists(path string) bool { return true } +func (c *Benchdiff) gitRunner() *gitRunner { + return &gitRunner{ + repoPath: c.Path, + } +} + +func (c *Benchdiff) baseRefRunner() *refRunner { + return &refRunner{ + ref: c.BaseRef, + gitRunner: gitRunner{ + repoPath: c.Path, + }, + } +} + func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) { result = new(runBenchmarksResults) - worktreeFilename := filepath.Join(c.ResultsDir, "benchstatter-worktree.out") + worktreeFilename := filepath.Join(c.ResultsDir, "benchdiff-worktree.out") worktreeFile, err := os.Create(worktreeFilename) if err != nil { return nil, err @@ -76,11 +80,18 @@ func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) { return nil, err } - baseFilename, err := c.baseOutputFile() + headSHA, err := c.gitRunner().getRefSha("HEAD") + if err != nil { + return nil, err + } + baseSHA, err := c.gitRunner().getRefSha(c.BaseRef) if err != nil { return nil, err } + baseFilename := fmt.Sprintf("benchdiff-%s.out", baseSHA) + result.headSHA = headSHA + result.baseSHA = baseSHA result.baseOutputFile = baseFilename result.worktreeOutputFile = worktreeFilename @@ -102,14 +113,8 @@ func (c *Benchdiff) runBenchmarks() (result *runBenchmarksResults, err error) { baseCmd := exec.Command(c.BenchCmd, strings.Fields(c.BenchArgs)...) //nolint:gosec // this is fine baseCmd.Stdout = baseFile var baseCmdErr error - runner := &refRunner{ - ref: c.BaseRef, - gitRunner: gitRunner{ - repoPath: c.Path, - gitExecutable: "", - }, - } - err = runner.run(func() { + + err = c.baseRefRunner().run(func() { baseCmdErr = baseCmd.Run() }) if err != nil { @@ -138,6 +143,8 @@ func (c *Benchdiff) Run() (*RunResult, error) { return nil, err } result := &RunResult{ + headSHA: res.headSHA, + baseSHA: res.baseSHA, benchCmd: res.benchmarkCmd, tables: collection.Tables(), } @@ -146,6 +153,8 @@ func (c *Benchdiff) Run() (*RunResult, error) { // RunResult is the result of a Run type RunResult struct { + headSHA string + baseSHA string benchCmd string tables []*benchstat.Table } @@ -194,31 +203,39 @@ func (r *RunResult) WriteOutput(w io.Writer, opts *RunResultOutputOptions) error func (r *RunResult) writeJSONResult(w io.Writer, benchstatResult string) error { type runResultJSON struct { BenchCommand string `json:"bench_command,omitempty"` - BenchstatResult string `json:"benchstat_result,omitempty"` - } - o := runResultJSON{ - BenchCommand: r.benchCmd, - BenchstatResult: benchstatResult, + HeadSHA string `json:"head_sha,omitempty"` + BaseSHA string `json:"base_sha,omitempty"` + BenchstatOutput string `json:"benchstat_output,omitempty"` } encoder := json.NewEncoder(w) encoder.SetIndent("", " ") - return encoder.Encode(&o) + return encoder.Encode(&runResultJSON{ + BenchCommand: r.benchCmd, + BenchstatOutput: benchstatResult, + HeadSHA: r.headSHA, + BaseSHA: r.baseSHA, + }) } func (r *RunResult) writeHumanResult(w io.Writer, benchstatResult string) error { var err error - if r.benchCmd != "" { - _, err = fmt.Fprintf(w, "bench command:\n %s\n", r.benchCmd) - if err != nil { - return err - } + _, err = fmt.Fprintf(w, "bench command:\n %s\n", r.benchCmd) + if err != nil { + return err } - if benchstatResult != "" { - _, err = fmt.Fprintf(w, "result:\n\n%s\n", benchstatResult) - if err != nil { - return err - } + _, err = fmt.Fprintf(w, "HEAD sha:\n %s\n", r.headSHA) + if err != nil { + return err + } + _, err = fmt.Fprintf(w, "base sha:\n %s\n", r.baseSHA) + if err != nil { + return err } + _, err = fmt.Fprintf(w, "result:\n\n%s\n", benchstatResult) + if err != nil { + return err + } + return nil } diff --git a/gitrunner.go b/gitrunner.go index 9526f55..01cc3ce 100644 --- a/gitrunner.go +++ b/gitrunner.go @@ -12,6 +12,15 @@ type gitRunner struct { gitExecutable string } +func (r *gitRunner) getRefSha(ref string) (string, error) { + b, err := r.run("rev-parse", ref) + if err != nil { + return "", err + } + b = bytes.TrimSpace(b) + return string(b), nil +} + func (r *gitRunner) run(args ...string) ([]byte, error) { executable := "git" if r.gitExecutable != "" { @@ -23,7 +32,12 @@ func (r *gitRunner) run(args ...string) ([]byte, error) { if err != nil { return nil, err } - return cmd.Output() + + b, err := cmd.Output() + if exitErr, ok := err.(*exec.ExitError); ok { + err = fmt.Errorf("error running git command: %s", string(exitErr.Stderr)) + } + return b, err } type refRunner struct { diff --git a/pkg/benchstat/benchstat_test.go b/pkg/benchstat/benchstat_test.go index 8f64510..a1e623b 100644 --- a/pkg/benchstat/benchstat_test.go +++ b/pkg/benchstat/benchstat_test.go @@ -9,8 +9,8 @@ import ( ) func TestBenchstat_Run(t *testing.T) { - worktreeFile := filepath.FromSlash("./testdata/outputs/benchstatter-worktree.out") - baseFile := filepath.FromSlash("./testdata/outputs/benchstatter-1.out") + worktreeFile := filepath.FromSlash("./testdata/outputs/benchdiff-worktree.out") + baseFile := filepath.FromSlash("./testdata/outputs/benchdiff-1.out") var buf bytes.Buffer bs := &Benchstat{} collection, err := bs.Run(worktreeFile, baseFile) diff --git a/pkg/benchstat/testdata/outputs/benchstatter-1.out b/pkg/benchstat/testdata/outputs/benchdiff-1.out similarity index 88% rename from pkg/benchstat/testdata/outputs/benchstatter-1.out rename to pkg/benchstat/testdata/outputs/benchdiff-1.out index 11fadcd..f42516e 100644 --- a/pkg/benchstat/testdata/outputs/benchstatter-1.out +++ b/pkg/benchstat/testdata/outputs/benchdiff-1.out @@ -1,6 +1,6 @@ goos: darwin goarch: amd64 -pkg: github.com/WillAbides/benchstatter/tmp/747290359 +pkg: github.com/willabides/benchdiff/tmp/747290359 BenchmarkDoNothing-8 10 11164321 ns/op 12 B/op 0 allocs/op BenchmarkDoNothing-8 10 10853508 ns/op 14 B/op 0 allocs/op BenchmarkDoNothing-8 10 11656783 ns/op 11 B/op 0 allocs/op @@ -12,4 +12,4 @@ BenchmarkDoNothing-8 10 10506228 ns/op 12 B/op 0 allocs/ BenchmarkDoNothing-8 10 11475983 ns/op 11 B/op 0 allocs/op BenchmarkDoNothing-8 10 10771303 ns/op 11 B/op 0 allocs/op PASS -ok github.com/WillAbides/benchstatter/tmp/747290359 1.223s +ok github.com/willabides/benchdiff/tmp/747290359 1.223s diff --git a/pkg/benchstat/testdata/outputs/benchstatter-worktree.out b/pkg/benchstat/testdata/outputs/benchdiff-worktree.out similarity index 88% rename from pkg/benchstat/testdata/outputs/benchstatter-worktree.out rename to pkg/benchstat/testdata/outputs/benchdiff-worktree.out index 2e5f1c8..cce481c 100644 --- a/pkg/benchstat/testdata/outputs/benchstatter-worktree.out +++ b/pkg/benchstat/testdata/outputs/benchdiff-worktree.out @@ -1,6 +1,6 @@ goos: darwin goarch: amd64 -pkg: github.com/WillAbides/benchstatter/tmp/747290359 +pkg: github.com/willabides/benchdiff/tmp/747290359 BenchmarkDoNothing-8 10 1146337 ns/op 37 B/op 0 allocs/op BenchmarkDoNothing-8 10 1249409 ns/op 33 B/op 0 allocs/op BenchmarkDoNothing-8 10 1382591 ns/op 32 B/op 0 allocs/op @@ -12,4 +12,4 @@ BenchmarkDoNothing-8 10 1335703 ns/op 32 B/op 0 allocs/ BenchmarkDoNothing-8 10 1402933 ns/op 32 B/op 0 allocs/op BenchmarkDoNothing-8 10 1378127 ns/op 32 B/op 0 allocs/op PASS -ok github.com/WillAbides/benchstatter/tmp/747290359 0.171s +ok github.com/willabides/benchdiff/tmp/747290359 0.171s From 5604dda9f3ab298b79b4de28e13ca5e0f35b152a Mon Sep 17 00:00:00 2001 From: Will Roden Date: Tue, 29 Dec 2020 15:19:09 -0600 Subject: [PATCH 3/3] benchstat output --- benchdiff.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benchdiff.go b/benchdiff.go index 3c2b23c..fd974cc 100644 --- a/benchdiff.go +++ b/benchdiff.go @@ -231,7 +231,7 @@ func (r *RunResult) writeHumanResult(w io.Writer, benchstatResult string) error if err != nil { return err } - _, err = fmt.Fprintf(w, "result:\n\n%s\n", benchstatResult) + _, err = fmt.Fprintf(w, "benchstat output:\n\n%s\n", benchstatResult) if err != nil { return err }