Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 of 12 tasks
prattmic opened this issue Jun 24, 2022 · 8 comments
Open
1 of 12 tasks

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

prattmic opened this issue Jun 24, 2022 · 8 comments
Assignees
Labels
gopls NeedsFix Performance Tools
Milestone

Comments

@prattmic
Copy link
Member

prattmic commented Jun 24, 2022

#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:

  • 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.)
  • 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:

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

    • Continue to check out and build the baseline Go toolchain [1]
    • Find and check out the baseline x/tools commit. This likely requires changes to maintner to lookup subrepo releases [2].
    • Check out x/benchmark tip.
    • 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.
  • 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.

  • 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 prattmic added Performance NeedsFix labels Jun 24, 2022
@prattmic prattmic added this to the Unreleased milestone Jun 24, 2022
@gopherbot gopherbot added the Tools label Jun 24, 2022
@gopherbot
Copy link

gopherbot commented Jun 24, 2022

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

@gopherbot
Copy link

gopherbot commented Jun 24, 2022

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

@gopherbot
Copy link

gopherbot commented Jun 24, 2022

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

@dr2chase
Copy link
Contributor

dr2chase commented Jun 24, 2022

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.

@findleyr findleyr added the gopls label Jul 14, 2022
@gopherbot
Copy link

gopherbot commented Jul 29, 2022

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

gopherbot pushed a commit to golang/tools that referenced this issue Aug 4, 2022
Significantly refactor the gopls benchmarks to turn them into proper
benchmarks, eliminate the need for passing flags, and allow running them
on external gopls processes so that they may be used to test older gopls
versions.

Doing this required decoupling the benchmarks themselves from the
regtest.Runner. Instead, they just create their own regtest.Env to use
for scripting operations. In order to facilitate this, I tried to
redefine Env as a convenience wrapper around other primitives.

By using a separate environment setup for benchmarks, I was able to
eliminate a lot of regtest.Options that existed only to prevent the
regtest runner from adding instrumentation that would affect
benchmarking. This also helped clean up Runner.Run somewhat, though it
is still too complicated.

Also eliminate the unused AnyDiagnosticAtCurrentVersion, and make a few
other TODOs about future cleanup.

For golang/go#53992
For golang/go#53538

Change-Id: Idbf923178d4256900c3c05bc8999c0c9839a3c07
Reviewed-on: https://go-review.googlesource.com/c/tools/+/419988
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Peter Weinberger <pjw@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@findleyr
Copy link
Contributor

findleyr commented Aug 4, 2022

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 findleyr modified the milestones: gopls/v0.9.2, gopls/v0.10.0 Aug 4, 2022
@dr2chase
Copy link
Contributor

dr2chase commented Aug 10, 2022

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

@findleyr
Copy link
Contributor

findleyr commented Aug 10, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gopls NeedsFix Performance Tools
Projects
None yet
Development

No branches or pull requests

4 participants