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

mirage-crypto-ec: move API to string (instead of cstruct) #210

Merged
merged 2 commits into from
Mar 4, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Feb 29, 2024

No description provided.

@hannesm hannesm mentioned this pull request Feb 29, 2024
23 tasks
@@ -49,7 +49,7 @@ module type Dh = sig
than the group order meaning the public key cannot be the point at
inifinity. *)

val key_exchange : secret -> Cstruct.t -> (Cstruct.t, error) result
val key_exchange : secret -> string -> (string, error) result
Copy link
Member Author

Choose a reason for hiding this comment

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

note it may be useful to be able to pass a ?off in here (but that may as well be premature optimization) -- eventually you receive in some data stream the public you want to use in key_exchange, and with the revised API you always need to copy it out and create a fresh string. I'm rather undecided about that.

@hannesm
Copy link
Member Author

hannesm commented Mar 4, 2024

Asking @reynir @Firobe @dinosaure @palainp about opinions? I get some (10% - 20%) speedup for EC operations on my laptop with this.

@palainp
Copy link
Member

palainp commented Mar 4, 2024

Thank you @hannesm . I'm away from my computer for some days, but I'm in favor of that patch esp. as an improvement is observed :)

src/uncommon.ml Outdated Show resolved Hide resolved
@reynir
Copy link
Member

reynir commented Mar 4, 2024

On a Intel(R) Core(TM) i7-9700F CPU @ 3.00GHz I don't see any significant speedup (maybe <1% speedup). I'm unsure why not. Maybe it's the CPU architecture. On the other hand I don't observe a slowdown!

I'm in favor of this PR at least for the new interface.

@hannesm
Copy link
Member Author

hannesm commented Mar 4, 2024

it's not 10%, but here:

main branch (61721c2):

  • [ecdsa-sign]
    P256: 7541.426 ops per second (81300 iters in 10.780)
    P384: 3121.375 ops per second (29994 iters in 9.609)
    P521: 443.783 ops per second (4373 iters in 9.854)
    Ed25519: 10687.773 ops per second (103734 iters in 9.706)

  • [ecdsa-verify]
    P256: 1267.141 ops per second (14961 iters in 11.807)
    P384: 544.710 ops per second (5587 iters in 10.257)
    P521: 217.304 ops per second (2211 iters in 10.175)
    Ed25519: 7964.725 ops per second (79113 iters in 9.933)

  • [ecdh-share]
    P256: 1759.999 ops per second (17793 iters in 10.110)
    P384: 663.540 ops per second (6988 iters in 10.531)
    P521: 238.729 ops per second (2773 iters in 11.616)
    X25519: 12318.587 ops per second (123762 iters in 10.047)

this PR (ff2b4d4):

  • [ecdsa-sign]
    P256: 7600.079 ops per second (79239 iters in 10.426)
    P384: 3198.844 ops per second (31426 iters in 9.824)
    P521: 434.125 ops per second (4339 iters in 9.995)
    Ed25519: 11074.989 ops per second (109170 iters in 9.857)

  • [ecdsa-verify]
    P256: 1502.747 ops per second (14318 iters in 9.528)
    P384: 552.742 ops per second (5490 iters in 9.932)
    P521: 215.123 ops per second (2249 iters in 10.454)
    Ed25519: 8069.081 ops per second (85178 iters in 10.556)

  • [ecdh-share]
    P256: 1876.072 ops per second (17458 iters in 9.306)
    P384: 694.453 ops per second (6801 iters in 9.793)
    P521: 267.895 ops per second (2816 iters in 10.512)
    X25519: 12567.524 ops per second (142045 iters in 11.303)

so, likely it is noise in the end. but I prefer the interface over the cstruct one ;)

@hannesm hannesm merged commit 7a68208 into mirage:main Mar 4, 2024
23 of 26 checks passed
@hannesm hannesm deleted the ec-no-cstruct branch March 4, 2024 15:52
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.

4 participants