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

test(benchmark): add baby bear poseidon2 benchmark #519

Merged
merged 4 commits into from
Aug 14, 2024

Conversation

ashjeong
Copy link
Contributor

@ashjeong ashjeong commented Aug 13, 2024

Whereas all other benchmarks display all vendors on the same graph, Plonky3 and Horizen compute values with a different internal matrix, requiring them to be compared with Tachyon separately.

@chokobole
Copy link
Contributor

please add the scope benchmark to all the commits.

@ashjeong ashjeong changed the title test: add baby bear poseidon2 benchmark test(benchmark): add baby bear poseidon2 benchmark Aug 13, 2024
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch from 109ae21 to 843f3f4 Compare August 13, 2024 08:20
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch from 843f3f4 to bfcd3e5 Compare August 13, 2024 09:08
@ashjeong ashjeong marked this pull request as draft August 13, 2024 09:08
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch 3 times, most recently from 98facc0 to e646b19 Compare August 14, 2024 06:54
@batzor batzor force-pushed the test/add-poseidon2-benchmark branch from e646b19 to 940c74c Compare August 14, 2024 07:23
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch 3 times, most recently from bcdf927 to 0be182b Compare August 14, 2024 07:34
@ashjeong ashjeong marked this pull request as ready for review August 14, 2024 07:35
Copy link
Contributor

@TomTaehoonKim TomTaehoonKim left a comment

Choose a reason for hiding this comment

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

LGTM

benchmark/poseidon2/poseidon2_benchmark.cc Outdated Show resolved Hide resolved
benchmark/poseidon2/README.md Outdated Show resolved Hide resolved
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch from 0be182b to ccefa39 Compare August 14, 2024 08:21
Copy link
Contributor

@GideokKim GideokKim left a comment

Choose a reason for hiding this comment

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

LGTM

@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch from ccefa39 to 7843d32 Compare August 14, 2024 08:41
benchmark/poseidon2/poseidon2_benchmark.cc Outdated Show resolved Hide resolved
benchmark/poseidon2/README.md Outdated Show resolved Hide resolved
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch from 7843d32 to 23b439b Compare August 14, 2024 08:53
Copy link
Contributor

@chokobole chokobole left a comment

Choose a reason for hiding this comment

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

LGTM

Note that Poseidon2 permutations are now run 100 times to make the difference
in results more visible.

Authored by @chokobole, fixed by @ashjeong
@ashjeong ashjeong force-pushed the test/add-poseidon2-benchmark branch from 23b439b to 67d1c51 Compare August 14, 2024 08:56
Copy link
Contributor

@dongchangYoo dongchangYoo left a comment

Choose a reason for hiding this comment

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

[Optional] how about changing the names of the files?
from benchmark/poseidon2/poseidon2_benchmark_baby_bear_horizen_ubuntu_i9.png
to benchmark/poseidon2/baby_bear_horizen_ubuntu_i9.png

Else, LGTM

Copy link
Contributor

@batzor batzor left a comment

Choose a reason for hiding this comment

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

LGTM

@ashjeong
Copy link
Contributor Author

ashjeong commented Aug 14, 2024

[Optional] how about changing the names of the files? from benchmark/poseidon2/poseidon2_benchmark_baby_bear_horizen_ubuntu_i9.png to benchmark/poseidon2/baby_bear_horizen_ubuntu_i9.png

Else, LGTM

I do agree with you that this would be better shortened in the way you mentioned; however the photos are named in this way across all other benchmark photos and the standard becomes convoluted especially with the fft benchmark folder which has fft_bencmark... and ifft_benchmark... photos. Perhaps it can be changed all together later!

@chokobole chokobole merged commit 8d4e913 into main Aug 14, 2024
7 checks passed
@chokobole chokobole deleted the test/add-poseidon2-benchmark branch August 14, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants