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

Tools: Add benchmark warnings for PRs and push graphs for commits into master #3998

Merged
merged 15 commits into from
May 19, 2022

Conversation

algochoi
Copy link
Contributor

@algochoi algochoi commented May 17, 2022

Summary

This PR adds a github action to track performance regressions for the BenchmarkUintMath benchmark.

Workflows

The PR introduces two github workflows:

Variance

There is some variance between runs because github actions might spin up a different machine each time (e.g. Intel Xeon8370C vs 8171M). Empirically, the variance seems to be 10~30% for the most part. We tried putting some baseline benchmarks (e.g. sleep(1s)), but the variance in sleeping between machines is extremely low to provide any useful metric.

Time series visualization

The github-actions-benchmark tool we used provides a graph to visualize performance benchmark. It requires permissions to push to the gh-pages branch and can show trends over commits in the master branch. An example in a local repo can be seen here.

Future work

  • Currently, this benchmark only runs for BenchmarkUintMath, but it could be useful to track other benchmarks in the future to see any regressions/improvements over time.
  • A way to reduce or track variance between CI runs. Currently, there seems to be limited support for this in the github-action-benchmark tool we used.

Test Plan

Tested on a local fork and branch. Example PR for the performance alert can be seen here.

Closes an internal ticket

@algochoi algochoi changed the title Add benchmark warnings for PRs and push graphs for commits into master Tools: Add benchmark warnings for PRs and push graphs for commits into master May 17, 2022
@algochoi algochoi marked this pull request as ready for review May 17, 2022 20:56
@codecov
Copy link

codecov bot commented May 17, 2022

Codecov Report

Merging #3998 (bb26b6a) into master (91095fa) will increase coverage by 4.67%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3998      +/-   ##
==========================================
+ Coverage   49.77%   54.45%   +4.67%     
==========================================
  Files         409      390      -19     
  Lines       69157    48500   -20657     
==========================================
- Hits        34426    26411    -8015     
+ Misses      31011    19868   -11143     
+ Partials     3720     2221    -1499     
Impacted Files Coverage Δ
catchup/service.go 68.14% <0.00%> (-1.49%) ⬇️
ledger/acctupdates.go 69.43% <0.00%> (-0.14%) ⬇️
network/wsNetwork.go 62.79% <0.00%> (ø)
data/committee/msgp_gen.go
node/msgp_gen.go
protocol/msgp_gen.go
data/transactions/msgp_gen.go
crypto/merklesignature/msgp_gen.go
ledger/ledgercore/msgp_gen.go
data/account/msgp_gen.go
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91095fa...bb26b6a. Read the comment docs.

@algochoi
Copy link
Contributor Author

algochoi commented May 19, 2022

@michaeldiamant thanks for the quick review and explanations for your reasoning - I agreed with all the comments and addressed them in the latest PR

Copy link
Contributor

@michaeldiamant michaeldiamant left a comment

Choose a reason for hiding this comment

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

@algochoi Thanks for the effort here - I'm excited to collect data and gain experience with the tooling! ☕

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

Successfully merging this pull request may close these issues.

2 participants