go icon indicating copy to clipboard operation
go copied to clipboard

x/tools,x/build: performance monitoring for gopls

Open prattmic opened this issue 2 years ago • 22 comments

#48803 tracks performance monitoring work in general, primarily targeting the main Go repo. x/tools folks would like to track performance for gopls, which is feasible with a bit of extra work.

The primary goal is to track the benchmarks in x/tools/gopls/internal/regtest/bench, with the latest tagged version of gopls as the baseline, a built using the latest stable Go toolchain.

Things we need to do to get there:

  • [x] Adjust the benchmarks to have unique names. (Currently they are all testing.T tests that happen to print benchfmt output, but all with the same name. Current plan is to convert them to standard testing.B benchmarks.)
  • [x] Start reporting the repository under test into the results. Probably just "repo: go" / "repo: tools" in the benchfmt / Influx. (CLs 413915, 413686, 413687)

In the Coordinator:

This will be an x/tools builder to get proper triggering. I think we still want to use x/benchmarks/cmd/bench, so builds will have to additionally checkout x/benchmarks.

In runBenchmarkTests:

  • [x] Differentiate x/benchmarks runs (test Go repo) vs x/tools runs (test x/tools) (CL 442258). For x/tools mode:

    • [x] Continue to check out and build the baseline Go toolchain [1]
    • [x] Find and check out the baseline x/tools commit. This likely requires changes to maintner to lookup subrepo releases [2].
    • [x] Check out x/benchmark tip.
    • [x] Tell cmd/bench where everything is. Pass: BENCH_SUBREPO_PATH, BENCH_SUBREPO_BASELINE_PATH. BENCH_BASELINE_GOROOT is already passed and will be used as the toolchain to build with.
  • [x] Add a new toolchain-commit benchfmt field for the Go toolchain version. For Go repo builds this will be the same as baseline-commit, but for x/tools we'll want to be able to see the toolchain version somehow. (CL 442258)

  • [ ] In cmd/bench, when BENCH_REPO == tools, run something like go test -bench=. -count=10 golang.org/x/tools/gopls/internal/regtest/bench/... in experiment and baseline checkouts.

On the frontend:

  • [ ] Links to commits will be broken (they go to the Go repo). Fix those.
  • [ ] We probably also want some UI distinction that these are x/tools benchmarks and thus the x axis and baseline is completely unrelated to other benchmarks.
  • [ ] Because runs will trigger on changes to the Go repo, we will end up with duplicate points at the same experiment commit date (overlapping on the x axis). We should visually deduplicate these somehow (perhaps only render the first one we find).

[1] We'll still end up with a tip toolchain checked out and built on the builder which we just ignore. This is useless extra work, but I'm not sure it is worth the extra complexity to avoid.

[2] Perhaps we should just have cmd/bench run go get golang.org/x/tools/gopls@latest? This is far simpler, though it diverges from how we handle the Go repo baseline (i.e., by doing it from the coordinator)

cc @findleyr @mknyszek @dr2chase @aclements

prattmic avatar Jun 24 '22 15:06 prattmic

Change https://go.dev/cl/413915 mentions this issue: cmd/bench: add repo key

gopherbot avatar Jun 24 '22 17:06 gopherbot

Change https://go.dev/cl/413687 mentions this issue: perf: ingest repo key to influx

gopherbot avatar Jun 24 '22 17:06 gopherbot

Change https://go.dev/cl/413686 mentions this issue: cmd/coordinator: pass repo name to cmd/bench

gopherbot avatar Jun 24 '22 17:06 gopherbot

Regarding " Perhaps we should just have cmd/bench run go get golang.org/x/tools/gopls@latest? This is far simpler, though it diverges from how we handle the Go repo baseline (i.e., by doing it from the coordinator)" if this were outsourced to bent, autofetching at latest is the default if no version of the test repo is specified, but I don't know if the commit/date of the repo that it obtains is communicated to the database.

dr2chase avatar Jun 24 '22 19:06 dr2chase

Change https://go.dev/cl/419988 mentions this issue: gopls/internal/regtest/bench: refactor and improve benchmarks

gopherbot avatar Jul 29 '22 17:07 gopherbot

With CL 419988, gopls benchmarks are now proper benchmarks, and can be run without any additional flags. For example:

> cd gopls/internal/regtest/bench
> go test -bench=DidChange
checking out https://go.googlesource.com/tools@gopls/v0.9.0 to /tmp/gopls-bench1659971946
goos: linux
goarch: amd64
pkg: golang.org/x/tools/gopls/internal/regtest/bench
cpu: Intel(R) Core(TM) i7-10610U CPU @ 1.80GHz
BenchmarkDidChange-8        2515            564962 ns/op
PASS
ok      golang.org/x/tools/gopls/internal/regtest/bench 10.368s

Additionally, the benchmark suite can be used with an externally compiled version of gopls:

> go test -bench=DidChange -gopls=$(which gopls0.8.4)
checking out https://go.googlesource.com/tools@gopls/v0.9.0 to /tmp/gopls-bench3718370246
goos: linux
goarch: amd64
pkg: golang.org/x/tools/gopls/internal/regtest/bench
cpu: Intel(R) Core(TM) i7-10610U CPU @ 1.80GHz
BenchmarkDidChange-8        1038           2048212 ns/op
PASS
ok      golang.org/x/tools/gopls/internal/regtest/bench 21.239s

(the bit about checking out gopls/v0.9.0 may be confusing, I realize now: that's just an arbitrary repo:commit used as the workspace for benchmarking)

findleyr avatar Aug 04 '22 16:08 findleyr

This will appear in 0.9.2? I tested it, but "@latest" appeared to grab 0.9.1 which lacks the new code.

dr2chase avatar Aug 10 '22 15:08 dr2chase

Yes, that's correct. Can we use @master for benchmarks, actually? There is an additional fix that also isn't in v0.9.2 (which will be released today or tomorrow.

findleyr avatar Aug 10 '22 15:08 findleyr

Change https://go.dev/cl/422908 mentions this issue: gopls/internal/regtest/bench: replace -gopls_version with -gopls_commit

gopherbot avatar Aug 15 '22 16:08 gopherbot

I did the testing @master, it worked work. I'm not sure what the plan for where we report this benchmark's results should be; I assume you are interested in benchmarking gopls changes, not compiler changes. I can look at this, in terms of new knobs, it might be easy to add.

dr2chase avatar Aug 15 '22 20:08 dr2chase

@dr2chase thanks!

Thinking about this use-case, I just added a new flag in CL 422908. The -gopls_commit flag can be given an arbitrary commitish, and the test runner will build and use gopls at that commit.

Does that help? There are too many dimensions here:

  1. The commit containing the actual benchmarks (what if the benchmark meaningfully changes?)
  2. The Go commit (which gopls is, for the record, sensitive to, c.f.#54172).
  3. The gopls commit.

Would be really cool to get this working!

findleyr avatar Aug 15 '22 21:08 findleyr

Change https://go.dev/cl/442257 mentions this issue: cmd/coordinator: move baseline toolchain install to method

gopherbot avatar Oct 11 '22 19:10 gopherbot

Change https://go.dev/cl/442258 mentions this issue: cmd/coordinator: add subrepo benchmarking support

gopherbot avatar Oct 12 '22 20:10 gopherbot

Change https://go.dev/cl/442615 mentions this issue: dashboard: add linux-amd64-perf builder for x/tools

gopherbot avatar Oct 12 '22 20:10 gopherbot

The coordinator changes are in, so linux-amd64-perf builders are now running for x/tools on the release branches. Builds are failing because cmd/bench needs to add support for the tools repo.

prattmic avatar Oct 18 '22 18:10 prattmic

Change https://go.dev/cl/449036 mentions this issue: dashboard/builders: add KI on linux-amd64-perf

gopherbot avatar Nov 09 '22 18:11 gopherbot

Change https://go.dev/cl/453502 mentions this issue: cmd/bench: add subrepo directories to run benchmarks on

gopherbot avatar Nov 30 '22 19:11 gopherbot

Change https://go.dev/cl/458996 mentions this issue: perf: add repository selection drop-down

gopherbot avatar Dec 21 '22 21:12 gopherbot

Change https://go.dev/cl/459155 mentions this issue: perf: dashboard support for multiple branches

gopherbot avatar Dec 22 '22 14:12 gopherbot

Change https://go.dev/cl/459096 mentions this issue: cmd/bench: set runstamp for subrepo tests

gopherbot avatar Dec 22 '22 15:12 gopherbot

Change https://go.dev/cl/459177 mentions this issue: benchseries: return error on time parse error

gopherbot avatar Dec 22 '22 16:12 gopherbot

Change https://go.dev/cl/459195 mentions this issue: all: upgrade to golang.org/x/perf@3fd27c2392831a268043c8e09d4599e7f1c441d3

gopherbot avatar Dec 22 '22 17:12 gopherbot

Following up on a discussion with @dle8, the easiest way to run benchmarks at the experiment (but gopls at baseline) is as follows. Assume $BASELINE and $EXPERIMENT are the directories where resp. baseline and experiment are checked out.

cd $BASELINE/gopls
go build
cd $EXPERIMENT/gopls
go build
go test ./internal/regtest/bench -bench=.* -gopls_path=$BASELINE/gopls/gopls  # run experiment benchmarks, using baseline gopls
go test ./internal/regtest/bench -bench=.* -gopls_path=$EXPERIMENT/gopls/gopls  # run experiment benchmarks, using experiment gopls

findleyr avatar Jan 10 '23 22:01 findleyr

Change https://go.dev/cl/465156 mentions this issue: dashboard: unmark known-issues with low failure rates

gopherbot avatar Feb 03 '23 20:02 gopherbot

Change https://go.dev/cl/469355 mentions this issue: gopls/internal/regtest/bench: add benchmarks in a wider variety of repos

gopherbot avatar Feb 18 '23 00:02 gopherbot

Change https://go.dev/cl/468940 mentions this issue: gopls/internal/regtest/bench: support benchmarking multiple repos

gopherbot avatar Feb 18 '23 00:02 gopherbot

I think all the backend work is done here, and we're using the new dashboard heavily.

There is more UI work to be done, but I'm not sure when it will be prioritized. Unassigning myself for now.

findleyr avatar Mar 30 '23 18:03 findleyr

@prattmic how about closing this out, and opening a separate, lower priority issue for UI improvements?

findleyr avatar Mar 30 '23 18:03 findleyr

Sounds good, go for it.

prattmic avatar Mar 30 '23 18:03 prattmic

Great, opened #59337.

findleyr avatar Mar 30 '23 21:03 findleyr