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

Closed
9 of 12 tasks
prattmic opened this issue Jun 24, 2022 · 30 comments
Closed
9 of 12 tasks

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

prattmic opened this issue Jun 24, 2022 · 30 comments
Assignees
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Performance Tools This label describes issues relating to any tools in the x/tools repository.
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) (CL 442258). 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. (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 (EDIT: deferred to #59337):

  • 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 The path to resolution is known, but the work has not been done. labels Jun 24, 2022
@prattmic prattmic added this to the Unreleased milestone Jun 24, 2022
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Jun 24, 2022
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@dr2chase
Copy link
Contributor

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 Issues related to the Go language server, gopls. label Jul 14, 2022
@gopherbot
Copy link
Contributor

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

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

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.

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Aug 15, 2022
The go command can't always resolve arbitrary commits as versions, and
some commits have an x/tools replace directive that must be honored, so
use 'git clone' with an arbitrary commit ref, instead of 'go install'
with a Go module version, to install gopls.

Also rename BenchmarkIWL to BenchmarkInitialWorkspaceLoad.

For golang/go#53538

Change-Id: Ic3a08e4c023e0292f6595cc5b2ab59954d073546
Reviewed-on: https://go-review.googlesource.com/c/tools/+/422908
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
@dr2chase
Copy link
Contributor

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.

@findleyr
Copy link
Contributor

findleyr commented Aug 15, 2022

@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.go/types: don't build errors in AssignableTo, ConvertibleTo #54172).
  3. The gopls commit.

Would be really cool to get this working!

@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/benchmarks that referenced this issue Dec 13, 2022
For golang/go#53538

Change-Id: I0eb1d2f00ca1977bdd909e0a4bfe4367d388ba16
Reviewed-on: https://go-review.googlesource.com/c/benchmarks/+/453502
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Dylan Le <dungtuanle@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/build that referenced this issue Dec 21, 2022
The tools repo will soon have benchmark results. Split the dashboard by
repository (default 'go'), so that results aren't mixed together.

This is pretty straightforward. The Influx queries filter by repository
(backfilling any points without the 'repository' field as 'go') via a
URL parameter. The frontend adds a drop-down box to select the
repository.

The "All benchmarks" and "Regressions first" links unfortunately don't
maintain the repository setting (or duration/end time). That requires a
bit more JS magic.

For golang/go#53538.

Change-Id: I1d46a23558a37feef4c8c89cfb9c3927304ebb60
Reviewed-on: https://go-review.googlesource.com/c/build/+/458996
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Dylan Le <dungtuanle@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/benchmarks that referenced this issue Dec 22, 2022
For golang/go#53538.

Change-Id: Ic68861b65a7b5cd259ab6c8c82a10a08323f1a6d
Reviewed-on: https://go-review.googlesource.com/c/benchmarks/+/459096
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/perf that referenced this issue Dec 22, 2022
AllComparisonSeries shouldn't panic on bad input, it should return an
error.

For golang/go#53538.

Change-Id: I988b546ea3ad5bb9026956b3aa4feec809915b73
Reviewed-on: https://go-review.googlesource.com/c/perf/+/459177
Auto-Submit: Michael Pratt <mpratt@google.com>
Reviewed-by: David Chase <drchase@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/build that referenced this issue Dec 22, 2022
First and foremost, we must ingest uploads on alternative branches.
Right now ingest only branch master.

On the frontend, add a dropdown for branch, very similar to the
repository dropdown. In the future, we should probably fetch a list of
branches rather than hard-coding them.

Note that the branch here is the Go branch that the perf builder
triggered against, which is a bit confusing for subrepos, but I've added
a note to try to explain.

For golang/go#53538.

Change-Id: Ib5b74b9e5d7b67ce2b04bc2bb22e3521dffaee36
Reviewed-on: https://go-review.googlesource.com/c/build/+/459155
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Dec 22, 2022
…441d3

This fixes AllComparisonSeries to return errors rather than panicking.

For golang/go#53538.

Change-Id: I872a1a77c8aedc3827dec650c21afa7c68b540af
Reviewed-on: https://go-review.googlesource.com/c/build/+/459195
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
@findleyr
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/build that referenced this issue Feb 4, 2023
I had initially added known issues fairly aggressively in order to use
them to reduce noise in 'greplogs -triage'. Now that we are using
'watchflakes' for triage, that noise reduction is no longer important
(the failures are already clustered to their respective known issues),
and having greyed-out cells on the dashboard makes new regressions too
easy to miss.

Concretely:

- golang/go#42212 is mostly specific to x/net at this point (as
  golang/go#57841)

- There have been no failures matching golang/go#51001 since October.

- golang/go#52724 has been so rare lately that we hadn't yet added a
  'watchflakes' pattern for it.

- There have been no failures matching golang/go#51443 since May.

- There have been no failures matching golang/go#53116 or
  golang/go#53093 since I enabled 'watchflakes' for the builder in
  December.

- The linux-amd64-perf builder seems to be passing consistently for
  x/benchmarks and x/tools, so there is no need to refer to
  golang/go#53538 to explain failures on it.

Change-Id: Ia16db2a23e5fa037a299f1f56fb26f1cf84521e1
Reviewed-on: https://go-review.googlesource.com/c/build/+/465156
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

gopherbot pushed a commit to golang/tools that referenced this issue Mar 3, 2023
Extract benchmark state into a new repo type, so that we may run
benchmarks in multiple shared workspaces. Also, add missing cleanup
code.

Additionally, simplify to always run gopls in a separate process. This
means that the normal test profiling flags won't be useful, so add
support for threading through profiling flags to the external gopls
process.

For golang/go#53538

Change-Id: Ib9ab5920dc59f102c62b53b761379dd8ca2d7141
Reviewed-on: https://go-review.googlesource.com/c/tools/+/468940
TryBot-Result: Gopher Robot <gobot@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Mar 6, 2023
Extend existing benchmarks to run in more repos, choosing an initial set
with different features that may affect performance. Some of these take
too long if run in the same batch, so guard with -short.

Several benchmarks need a location within the codebase. For these, I
have chosen somewhat arbitrarily, but tried to select locations within
the core of the codebase. We can always adjust in the future.

Additionally:
 - fix the fake file polling to scale to larger codebases, by avoiding
   reading files if it isn't necessary
 - fix a polling bug related to symlinks
 - fix a couple places the benchmarks weren't cleaning up after
   themselves correctly
 - fix a bug where the gopls install used the wrong directory

For golang/go#53538

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

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
Copy link
Contributor

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

@prattmic
Copy link
Member Author

Sounds good, go for it.

@findleyr
Copy link
Contributor

Great, opened #59337.

@findleyr findleyr modified the milestones: gopls/later, gopls/v0.12.0 Mar 30, 2023
@golang golang locked and limited conversation to collaborators Mar 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. NeedsFix The path to resolution is known, but the work has not been done. Performance Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

6 participants