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

benchmark: also report the inverse of the performance (ops/s) #8

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

tomato42
Copy link
Collaborator

@tomato42 tomato42 commented Jul 18, 2024

Report the performance in operations per second too, like so:

---------------------------
  Kyber512 | (1000 calls)
---------------------------
Keygen: 5.569 s (179.571/s)
Enc: 8.278 s (120.799/s)
Dec: 8.278 s (74.929/s)
---------------------------
  Kyber768 | (1000 calls)
---------------------------
Keygen: 8.761 s (114.138/s)
Enc: 12.793 s (78.17/s)
Dec: 12.793 s (49.208/s)
---------------------------
  Kyber1024 | (1000 calls)
---------------------------
Keygen: 13.473 s (74.22/s)
Enc: 18.706 s (53.46/s)
Dec: 18.706 s (34.171/s)

@GiacomoPope
Copy link
Owner

GiacomoPope commented Jul 18, 2024

While we're changing this, maybe we could use something like tabulate (or just do our own string formatting) to make this table prettier?

     Kyber512 - (1000 iterations)
-----------------------------------
Algorithm     | time (s) | ops / s
-----------------------------------
Keygen        | 5.569    | 180
Encapsulation | 8.278    | 121
Decapsulation | 8.278    | 75

Probably worth rounding to an integer for the ops / s, do you agree?

@tomato42
Copy link
Collaborator Author

tomato42 commented Jul 18, 2024

that's literally the second thing I wanted to propose 😁

but I was thinking of doing a transpose of it:

Group  |  keygen  | keygen/s | encapsulation | encapsulation/s | decapsulation | decapsulation/s 
------------------------------------------------------------------------------------------------
Kyber512 | 123 ...

(i.e. something that openssl speed outputs)

@GiacomoPope
Copy link
Owner

Yeah this seems good. The current output was something I wrote in 3 minutes to just check what's happening! I also thing average time is one metric, but minimum / maximum times is maybe interesting too.

@tomato42
Copy link
Collaborator Author

aah, right, openssl does report per operation time and inverse of it; will change this code to do the same

min and max of timings is a bit too much: properly benchmarking python code is a whole 'nother kettle of fish...

@GiacomoPope
Copy link
Owner

haha yeah, hence the conservative README:

TODO: Better benchmarks? Although this was never about speed haha.

For now, here are some approximate benchmarks:

@tomato42
Copy link
Collaborator Author

Updated to output something like this:

----------------------------------------------------------------------
 group     | keygen | keygen/s | encap  | encap/s  | decap  | decap/s
----------------------------------------------------------------------
 Kyber512   0.00584s    171.21  0.00867s    115.28  0.01396s     71.65
 Kyber768   0.00839s    119.18  0.01222s     81.86  0.01938s     51.60
 Kyber1024  0.01275s     78.44  0.01774s     56.37  0.02774s     36.05

@GiacomoPope
Copy link
Owner

GiacomoPope commented Jul 18, 2024

I would swap group with parameters or param set just because it's not a maths group and it feels weird to call it a group.

I would also report time in ms rather than seconds so something like:

-------------------------------------------------------------------
 Params    | keygen | keygen/s | encap | encap/s | decap | decap/s 
-------------------------------------------------------------------
 Kyber512  |  5.84ms   171.21    8.67ms  115.28   13.96ms     71.65
 Kyber768  |  8.39ms   119.18   12.22ms   81.86   19.38ms     51.60
 Kyber1024 | 12.75ms    78.44   17.74ms   56.37   27.74ms     36.05

Signed-off-by: Hubert Kario <hkario@redhat.com>
@tomato42
Copy link
Collaborator Author

I've used "group" as that's the name in use in TLS: "key exchange group", to encompass both the FFDH prime, ECDH curve, and now PQC parameters. Changed to params

@GiacomoPope
Copy link
Owner

Cool. Looks good to me. I'll update the benchmark in the README tonight

@GiacomoPope GiacomoPope merged commit 4080796 into GiacomoPope:main Jul 18, 2024
@tomato42 tomato42 deleted the speed-inverse branch July 18, 2024 21: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.

2 participants