Show benchmarks comparison in PR comments #34
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for 36a39b7Click to view benchmark
|
piotr-roslaniec
force-pushed
the
sd-benchmarks
branch
from
85da77a
to
3ee6ac5
Compare
piotr-roslaniec
force-pushed
the
benchmarks-pr-compare
branch
from
da129e4
to
014da1e
Compare
|
I'm surprised at the variance between the base and PR benchmarks. Most are negligible but we also have +10% and -4%. Once the commits have been squashed I'm happy to merge this. |
|
@piotr-roslaniec Polynomial benchmarks aren't appearing in the PR comment. Does the comment get updated as new commits are pushed? |
This is because of the shared execution environment on GitHub Actions. It's not recommended to run performance-sensitive workloads there, but there are ways around that we could implement. As long as it's nothing major, we can use common sense and our local environments to figure out whether the performance was actually impacted by any given change. Benchmarks in PRs could be treated as sanity checks for now (like one would use code coverage, etc.). |
piotr-roslaniec
force-pushed
the
benchmarks-pr-compare
branch
from
8a521a9
to
f5e92b2
Compare
So far the workflow has been failing, but soon we're going to find out. |
piotr-roslaniec
force-pushed
the
benchmarks-pr-compare
branch
2 times, most recently
from
e49e90a
to
3faaf96
Compare
|
GitHub Actions keeps running some old commit. The current commit is: |
theref
approved these changes
Jan 19, 2023
|
The benchmark job is failing because GH Action runs two benchmarks: one on the current commit HEAD (works) and one on the base branch ( |
piotr-roslaniec
force-pushed
the
benchmarks-pr-compare
branch
from
9b4b881
to
0a24d30
Compare
Benchmark for e5466b6Click to view benchmark
|
|
Those benchmarks take a lot of time to compute: They run two times in one GH action, and then once again in some other action, for 3 times total. I'm going to update and introduce some changes after merging this PR, but first I want to make sure everything works ok. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
main). See Update decryption benchmarks intpke#25.tpke#25). For now, let's run both, and let's see if we prefer one over another.Naive random polynomial evaluation vs Arkworks implementation