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/perf and x/perfdata: new subrepos #14304

Closed
rsc opened this issue Feb 12, 2016 · 21 comments
Closed

x/perf and x/perfdata: new subrepos #14304

rsc opened this issue Feb 12, 2016 · 21 comments

Comments

@rsc
Copy link
Contributor

rsc commented Feb 12, 2016

I propose to rename the current x/benchmarks subrepo to x/perf and then broaden its scope to include performance analysis tools such as the ones that Austin and I have developed over the past few years and keep in personal GitHub repos today.

The name x/benchmarks is too long and too specific: renaming it lets us correct both these problems, making it more reasonable to include general performance analysis tools, not just benchmarks. Of course, code for running and analyzing benchmarks, along with actual benchmarks, will still be a large part of the repository.

The specific initial structure I expect is:

x/perf/
    # benchmarks, each a standalone program writing standard benchmark output
    benchmarks/
        build
        compile
        garbage
        json
        http
        ...

    # performance analysis commands
    cmd/
        benchstat   # currently rsc.io/benchstat
        benchrun    # currently github.com/aclements/go-misc/benchmany run
        benchplot   # currently github.com/aclements/go-misc/benchmany plot
        benchlist   # currently github.com/aclements/go-misc/benchmany list
        perf2pprof  # convert Linux perf format to Go pprof, currently unpublished
        ...

    bench           # package for reading standard benchmark files and benchmark databases 

More benchmarks, comands, and packages would be added over time.

The change would be executed by creating a new x/perf repo, pushing the current x/benchmarks repo to it, and then moving files in the repository to reflect the new structure. Doing it this way preserves the change history (including commit hashes) for the existing benchmarks.

We could keep the x/benchmarks repo read-only for some transition period before deleting it, although I think very few people outside the core Go team refer to it.

I do not intend to write a design doc in x/proposal about this. Instead, some version of this information will go into a README in the new repo.

I also propose to create a new x/perfdata repo to hold performance data, the raw data that might drive a new Go performance dashboard. The idea behind putting the perfdata in its own repo is so that people working on or wanting to obtain the tools need not download all of our (potentially large) data, but people who do want to try a new analysis or create a new view of the data can get it all and experiment.

In fact, the x/perfdata repo is meant to be the defined interface between the related but very different jobs of collecting reliable benchmark data and performing useful analyses or creating useful presentations of that data. I the main reason the previous perf dashboard failed is that we did an OK job on data collection and a bad job on data presentation, and we didn't make the raw data available (it was stored in an App Engine datastore accessible only to the app developers). The proposed repo allows some people to work on collecting data and others (anyone!) to experiment with new ways to analyze or present it.

/cc @aclements @dvyukov @RLH @randall77 @dr2chase

@aclements
Copy link
Member

I started implementing this this week. I don't think we should put both the benchmarks and the tools in a single repository because their audiences are different. The benchmarks are for performance analysis of the Go core and are only useful to Go core developers. The tools, however, have a much wider audience, since they're useful for anyone using benchmarks in Go.

This suggests that we should 1) leave the benchmarks in their own repository, which might as well continue to be x/benchmarks, and 2) either create a new subrepo for performance tools and packages or put these in the x/tools repository. For 2, I don't have a strong opinion whether we create a new subrepo or use x/tools, except that x/tools is already a grab bag.

I think creating x/perfdata still makes sense. And we should still modify the existing benchmarks to produce standard benchmark output and probably convert them to separate binaries for better modularity (I have several other benchmarks it would make sense to put in there, but that don't make sense to add to the existing monolithic binary).

/cc @adg @quentinmit

@adg
Copy link
Contributor

adg commented Jun 10, 2016

SGTM

On 10 June 2016 at 02:29, Austin Clements notifications@github.com wrote:

I started implementing this this week. I don't think we should put both
the benchmarks and the tools in a single repository because their audiences
are different. The benchmarks are for performance analysis of the Go
core
and are only useful to Go core developers. The tools, however, have
a much wider audience, since they're useful for anyone using benchmarks in
Go.

This suggests that we should 1) leave the benchmarks in their own
repository, which might as well continue to be x/benchmarks, and 2) either
create a new subrepo for performance tools and packages or put these in the
x/tools repository. For 2, I don't have a strong opinion whether we create
a new subrepo or use x/tools, except that x/tools is already a grab bag.

I think creating x/perfdata still makes sense. And we should still modify
the existing benchmarks to produce standard benchmark output and probably
convert them to separate binaries for better modularity (I have several
other benchmarks it would make sense to put in there, but that don't make
sense to add to the existing monolithic binary).

/cc @adg https://github.com/adg @quentinmit
https://github.com/quentinmit


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14304 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AIDilcGix0Hx5i1HEskoNpRJ4_vbZPfaks5qKD9ZgaJpZM4HYxl9
.

@aclements
Copy link
Member

@adg, any thoughts on putting the tools in a new x/perf or in the existing x/tools?

@adg
Copy link
Contributor

adg commented Jun 14, 2016

If they're all performance-related then putting them in x/perf seems preferable to x/tools.

@quentinmit
Copy link
Contributor

Are you suggesting putting automated performance run data in a Git repo? GIt seems like the wrong tool for that job...

+1 @adg, x/perf sounds like a better home for the tools.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/33580 mentions this issue.

@rsc
Copy link
Contributor Author

rsc commented Dec 20, 2016

Update: we will add a perf subrepo for tools but store the data elsewhere (not in Git).

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34613 mentions this issue.

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34614 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 20, 2016
For golang/go#14304.

Change-Id: Ia90193d3a9ad027231f977ffa6b66cf60ea40683
Reviewed-on: https://go-review.googlesource.com/34615
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Dec 20, 2016
New perf subrepo for golang/go#14304.

Change-Id: I1656df375b1163dfb0a9e5a57f459f093fc5aadb
Reviewed-on: https://go-review.googlesource.com/34613
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
gopherbot pushed a commit to golang/build that referenced this issue Dec 20, 2016
For golang/go#14304.

Change-Id: I4b9737716bdb9de74fd9ca5e855f853494651b77
Reviewed-on: https://go-review.googlesource.com/34614
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/34661 mentions this issue.

gopherbot pushed a commit to golang/tools that referenced this issue Dec 21, 2016
For golang/go#14304.

Change-Id: Ia90193d3a9ad027231f977ffa6b66cf60ea40683
Reviewed-on: https://go-review.googlesource.com/34615
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-on: https://go-review.googlesource.com/34661
@rsc rsc changed the title proposal: x/perf and x/perfdata: new subrepos x/perf and x/perfdata: new subrepos Jan 23, 2017
@rsc rsc removed the Proposal label Jan 23, 2017
@gopherbot
Copy link
Contributor

CL https://golang.org/cl/35503 mentions this issue.

gopherbot pushed a commit to golang/perf that referenced this issue Jan 25, 2017
I copied the code from various dependencies in go-moremath
into a single 'internal/stats' package. That package is at the top
level of the repo because I expect to pull much of benchcmp
into an importable package.

For golang/go#14304.

Change-Id: Ie114839b2901f5060c202feb3ffc768bf43ce5da
Reviewed-on: https://go-review.googlesource.com/35503
Run-TryBot: Russ Cox <rsc@golang.org>
Reviewed-by: Quentin Smith <quentin@golang.org>
gopherbot pushed a commit to golang/benchmarks that referenced this issue Jan 31, 2017
This has better modularity, since it will let us add other benchamrks
that have different drivers. It also isolates the benchmarks better,
since it avoids contaminating the heap with data allocated at init
time by other benchmarks (e.g., the json benchmark does this).

For golang/go#14304.

Change-Id: I5636152c5ebbd4e416e4979f99c7eb192513a717
Reviewed-on: https://go-review.googlesource.com/33580
Reviewed-by: Dmitry Vyukov <dvyukov@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
@dgryski
Copy link
Contributor

dgryski commented Feb 15, 2018

What else needs to be done for this?

@bradfitz
Copy link
Contributor

Well, perfdata never happened.

@dgryski
Copy link
Contributor

dgryski commented Feb 15, 2018

I guess the repo didn't, but benchmarks are being uploaded to https://perf.golang.org/ .

@dmitshur
Copy link
Contributor

I think new subrepo(s) should also be added to the lists at https://golang.org/pkg/#subrepo and https://godoc.org/-/subrepo, shouldn't they?

@aclements
Copy link
Member

Yes, x/perf should be added to the subrepos list. I'm not actually sure where that's maintained. @andybons?

@dmitshur
Copy link
Contributor

We can add x/perf individually or include it as part of #24432.

@aclements
Copy link
Member

I think they can be added as part of #24432.

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/103075 mentions this issue: godoc/static: add perf, review, sync subrepos

gopherbot pushed a commit to golang/tools that referenced this issue Mar 30, 2018
They exist and are considered to be worth listing based on the decision
made in golang/go#24432. They weren't listed previously. This change
fixes that.

Document the remaining subrepos that are not meant to be listed (per
decision in golang/go#24432), so it's clear that it's intentional for
them to not be visible at https://golang.org/pkg/#subrepo.

Closes golang/go#24432.
Updates golang/go#14304.

Change-Id: Icc50ebfcdbc490c32519d92a1a838eb3f54c823d
Reviewed-on: https://go-review.googlesource.com/103075
Reviewed-by: Austin Clements <austin@google.com>
@dmitshur
Copy link
Contributor

x/perf has been added to (the future version of) https://tip.golang.org/pkg#subrepo as part of #24432.

If there's nothing else that needs to be done here, this issue can be closed.

@golang golang locked and limited conversation to collaborators Mar 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

10 participants