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

feat: add benchmarks and performance checks on PRs #881

Merged
merged 18 commits into from
Jun 20, 2023

Conversation

ajnavarro
Copy link
Contributor

@ajnavarro ajnavarro commented Jun 8, 2023

Description

Add benchmark monitoring to keep track of improved/decreased performance. We also added checks to run fast benchmarks on every PR.

it closes #689

Still a WIP:

  • Use a GitHub self-hosted runner to keep benchmark results consistent
  • Add a README.md inside the .benchmarks folder to help people add new benchmarks to graphs and PR checks.
  • Remove -short flag when generating the graphs

Contributors Checklist

  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests

screenshot-localhost_8080-2023 06 08-15_44_09

@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 🤖 gnovm Issues or PRs gnovm related labels Jun 8, 2023
@ajnavarro ajnavarro changed the title feature: add benchmarks and performance checks on PRs feat: add benchmarks and performance checks on PRs Jun 8, 2023
benchmarks: []
diff: (current.NsPerOp - base.NsPerOp) / base.NsPerOp * 100
thresholds:
max: 10
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what it means, and what happens if we hit this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are rules that will be executed when the benchmark-check.yml action is triggered on every pr. It will compare the diff between the operations with the previous execution, and if the diff is bigger than 10%, it will fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 9 to 49
charts:
- name: Type assertion benchmarks (gnovm)
benchmarks: [ 'TypeAssertion.' ]
package: github.com\/gnolang\/gno\/gnovm\/pkg\/gnolang
- name: Reflect benchmarks (gnovm)
benchmarks: [ 'Reflect.' ]
package: github.com\/gnolang\/gno\/gnovm\/pkg\/gnolang
- name: Equality benchmarks (gnovm)
benchmarks: [ '.Equality' ]
package: github.com\/gnolang\/gno\/gnovm\/pkg\/gnolang
- name: LoopyMain benchmarks (gnovm)
benchmarks: [ '.LoopyMain' ]
package: github.com\/gnolang\/gno\/gnovm\/pkg\/gnolang
- name: Mempool (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/bft/mempool$
- name: State (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/bft/state$
- name: Types (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/bft/types$
- name: WAL (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/bft/wal$
- name: Bcrypt (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/crypto/bcrypt$
- name: ed25519 (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/crypto/ed25519$
- name: Merkle (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/crypto/merkle$
- name: secp256k1 (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/crypto/secp256k1$
- name: iavl (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/iavl.
- name: random (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/random$
- name: auth (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/sdk/auth$
- name: cache (tm2)
package: ^github.com/gnolang/gno/tm2/pkg/store/cache$
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can probably generate this dynamically

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if they seem to be the same, I had to tweak every package to have good graphs on the generated dashboard. As an example, LoopyMain benchmark takes a lot more time compared with other benchmarks on the same package, making the output graph unreadable, so I split them into two sections, one with the LoopyMain benchmark alone, and the other with all the other benchmarks.

- uses: actions/setup-go@v2
with: { go-version: "1.20" }
- name: gobenchdata publish
run: go run go.bobheadxi.dev/gobenchdata@v1 action
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add a go install step before; just to make sure that we've more comprehensible error handling and sense of speed for install and run.

@moul
Copy link
Member

moul commented Jun 8, 2023

wow, that looks powerful, and I like how it replaces an infra with git 👍

@@ -0,0 +1,23 @@
name: benchmarks publish
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be merged to have a single github action; maybe by merging the on: triggers and adding a if push + branch=master condition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, they are two different things, one is adding information about performance deltas on the PR, and the other is generating the dashboard with all the benchmark results. I can merge them if it clarifies what benchmark action does.

@ajnavarro ajnavarro force-pushed the feature/benchmarks branch 7 times, most recently from 0eacec2 to 3df7b56 Compare June 15, 2023 11:50
@ajnavarro
Copy link
Contributor Author

ajnavarro commented Jun 15, 2023

Until I have a self-hosted runner managed by the company and a token with write permissions, I managed to do it on my own on a fork and using my home server to run the benchmarks:

https://ajnavarro.com/gno-benchmarks/

I'm having some problems with the second run. I opened an issue: bobheadxi/gobenchdata#76 Problem fixed on my side, I was not using a stateless self-hosted runner

@ajnavarro ajnavarro added the 🌟 improvement performance improvements, refactors ... label Jun 16, 2023
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
demand too

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro ajnavarro marked this pull request as ready for review June 16, 2023 10:17
@ajnavarro ajnavarro requested review from jaekwon and a team as code owners June 16, 2023 10:17
@ajnavarro
Copy link
Contributor Author

Everything is tested and working on my side using a fork, we only need an access token with write permissions to write into benchmarks gh-pages branch all the results, and a stateless self-hosted runner to have consistent results every time we execute the tests.

CC @moul @albttx

Copy link
Member

@zivkovicmilos zivkovicmilos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this effort very much - it's a start in the right direction for performance tracking

I've left minor comments, otherwise looks great 💯

tm2/pkg/std/coin_benchmark_test.go Show resolved Hide resolved
tm2/pkg/iavl/benchmarks/bench_test.go Show resolved Hide resolved
tm2/pkg/crypto/secp256k1/bench_test.go Show resolved Hide resolved
tm2/pkg/bft/wal/wal_test.go Show resolved Hide resolved
tm2/pkg/amino/benchmark_test.go Show resolved Hide resolved
.github/pull_request_template.md Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Outdated Show resolved Hide resolved
.github/workflows/benchmark.yml Show resolved Hide resolved
@@ -0,0 +1,32 @@
# Benchmarks

This folder is where benchmarks are configured to be added on the dashboard generated in [benchmarks](https://gnoland.github.io/benchmarks).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@moul @albttx
Can we host these at benchmarks.gno.land, or somewhere similar?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea, let's do it

Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
Signed-off-by: Antonio Navarro Perez <antnavper@gmail.com>
@ajnavarro
Copy link
Contributor Author

As suggested by @moul , I changed to publish all benchmark results into a local branch, gh-benchmarks and added a process to copy that branch into benchmark's gh-pages branch here: gnolang/benchmarks#7

@moul moul merged commit 60bab86 into gnolang:master Jun 20, 2023
72 of 73 checks passed
@ajnavarro ajnavarro mentioned this pull request Jul 31, 2023
7 tasks
Doozers pushed a commit to Doozers/gno that referenced this pull request Aug 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 🤖 gnovm Issues or PRs gnovm related 🌟 improvement performance improvements, refactors ...
Projects
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

Performance dashboard: track and document performance improvements.
3 participants