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

Chacha20-Poly1305: use string instead of cstruct #203

Merged
merged 8 commits into from
Feb 28, 2024

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Feb 25, 2024

Performance improvement around 2.5x

Worth to gather some more statistics hereof, and the question how to move forward - I can see two ways:

  • preserve old API, add some functions (adds noise)
  • first reimplement stuff without Cstruct, preserve old API, then cut a release where the API doesn't include cstruct

WDYT?

main branch (28f8cde)
16: 11.686102 MB/s (2318663 iters in 3.028 s)
64: 44.914143 MB/s (2248341 iters in 3.055 s)
256: 111.408313 MB/s (1464243 iters in 3.209 s)
1024: 190.362636 MB/s (530561 iters in 2.722 s)
8192: 235.326363 MB/s (81843 iters in 2.717 s)

this PR (with using the ad-hoc API enc_auth_str):
16: 26.288283 MB/s (5247580 iters in 3.046 s)
64: 99.922308 MB/s (4995212 iters in 3.051 s)
256: 204.388046 MB/s (2405283 iters in 2.873 s)
1024: 278.060783 MB/s (863503 iters in 3.033 s)
8192: 294.746967 MB/s (113552 iters in 3.010 s)

Performance improvement from 8MB/s to 20MB/s (with 16 byte blocks, on my laptop)
@hannesm
Copy link
Member Author

hannesm commented Feb 25, 2024

note this could be split into pieces, such as introducing the xor_into_bytes function, then moving poly1305 to string, then adding macl, then moving chacha to string. on a separate note, in C we directly cast to uint32_t * to avoid shifting and reading byte-wise (this could also be done in a separate commit).

EDIT: ah, the big endian (s390x) fails due to that... fixed in the following commits

@palainp
Copy link
Member

palainp commented Feb 26, 2024

Neat! Thanks for that!
Here are my results (I confirm the x2 improvement on smallest sizes, and improvement everywhere!):

  • head
    16: 10.119168 MB/s (1347821 iters in 2.032 s)
    64: 38.610391 MB/s (1286782 iters in 2.034 s)
    256: 94.388170 MB/s (778949 iters in 2.015 s)
    1024: 144.286801 MB/s (296597 iters in 2.007 s)
    8192: 179.547401 MB/s (45952 iters in 1.999 s)
  • this pr
    16: 20.138118 MB/s (4008503 iters in 3.037 s)
    64: 74.277972 MB/s (3603763 iters in 2.961 s)
    256: 142.762819 MB/s (1758954 iters in 3.008 s)
    1024: 182.994248 MB/s (562805 iters in 3.003 s)
    8192: 189.376331 MB/s (72417 iters in 2.987 s)

src/chacha20.ml Outdated Show resolved Hide resolved
src/chacha20.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Member Author

hannesm commented Feb 27, 2024

with @reynir suggested changes 1e1d179

  • [chacha20-poly1305]
    16: 27.772872 MB/s (5554249 iters in 3.052 s)
    64: 105.891640 MB/s (5228603 iters in 3.014 s)
    256: 211.486396 MB/s (2608886 iters in 3.012 s)
    1024: 281.579622 MB/s (886396 iters in 3.074 s)
    8192: 299.032280 MB/s (116109 iters in 3.033 s)

@hannesm
Copy link
Member Author

hannesm commented Feb 28, 2024

I wondered about the CI failure, and the Bytes.unsafe_blit_string is only available from OCaml 4.09 on (fine with me to bump the lower OCaml bound)

@hannesm hannesm mentioned this pull request Feb 28, 2024
23 tasks
@hannesm hannesm merged commit 0a74d9e into mirage:main Feb 28, 2024
26 checks passed
@hannesm hannesm deleted the chacha-to-string branch February 28, 2024 21:15
hannesm added a commit to hannesm/opam-repository that referenced this pull request Jun 29, 2024
CHANGES:

### Breaking changes

* mirage-crypto: Poly1305 API now uses string (mirage/mirage-crypto#203 @hannesm)
* mirage-crypto: Poly1305 no longer has type alias "type mac = string"
  (mirage/mirage-crypto#232 @hannesm)
* mirage-crypto: the API uses string instead of cstruct (mirage/mirage-crypto#214 @reynir @hannesm)
* mirage-crypto: Hash module has been removed. Use digestif if you need hash
  functions (mirage/mirage-crypto#213 @hannesm)
* mirage-crypto: the Cipher_block and Cipher_stream modules have been removed,
  its contents is inlined:
  Mirage_crypto.Cipher_block.S -> Mirage_crypto.Block
  Mirage_crypto.Cipher_stream.S -> Mirage_crypto.Stream
  Mirage_crypto.Cipher_block.AES.CTR -> Mirage_crypto.AES.CTR
  (mirage/mirage-crypto#225 @hannesm, suggested in mirage/mirage-crypto#224 by @reynir)
* mirage-crypto-pk: s-expression conversions for private and public keys (Dh,
  Dsa, Rsa) have been removed. You can use PKCS8 for encoding and decoding
  `X509.{Private,Public}_key.{en,de}code_{der,pem}` (mirage/mirage-crypto#208 @hannesm)
* mirage-crypto-pk: in the API, Cstruct.t is no longer present. Instead,
  string is used (mirage/mirage-crypto#211 @reynir @hannesm)
* mirage-crypto-rng: the API uses string instead of Cstruct.t. A new function
  `generate_into : ?g -> bytes -> ?off:int -> int -> unit` is provided
  (mirage/mirage-crypto#212 @hannesm @reynir)
* mirage-crypto-ec: remove NIST P224 support (mirage/mirage-crypto#209 @hannesm @Firobe)
* mirage-crypto: in Uncommon.xor_into the arguments ~src_off and ~dst_off are
  required now (mirage/mirage-crypto#232 @hannesm), renamed to unsafe_xor_into
  (98f01b14f5ebf98ba0e7e9c2ba97ec518f90fddc)
* mirage-crypto-pk, mirage-crypto-rng: remove type alias "type bits = int"
  (mirage/mirage-crypto#236 @hannesm)

### Bugfixes

* mirage-crypto (32 bit systems): CCM with long adata (mirage/mirage-crypto#207 @reynir)
* mirage-crypto-ec: fix K_gen for bitlen mod 8 != 0 (reported in mirage/mirage-crypto#105 that
  P521 test vectors don't pass, re-reported mirage/mirage-crypto#228, fixed mirage/mirage-crypto#230 @Firobe)
* mirage-crypto-ec: zero out bytes allocated for Field_element.zero (reported
  mirleft/ocaml-x509#167, fixed mirage/mirage-crypto#226 @dinosaure)

### Data race free

* mirage-crypto (3DES): avoid global state in key derivation (mirage/mirage-crypto#223 @hannesm)
* mirage-crypto-rng: use atomic instead of reference to be domain-safe (mirage/mirage-crypto#221
  @dinosaure @reynir @hannesm)
* mirage-crypto, mirage-crypto-rng, mirage-crypto-pk, mirage-crypto-ec:
  avoid global buffers, use freshly allocated strings/bytes instead, avoids
  data races (mirage/mirage-crypto#186 mirage/mirage-crypto#219 @dinosaure @reynir @hannesm)

### Other changes

* mirage-crypto: add {de,en}crypt_into functions (and unsafe variants) to allow
  less buffer allocations (mirage/mirage-crypto#231 @hannesm)
* mirage-crypto-rng-miou: new package which adds rng support with miou
  (mirage/mirage-crypto#227 @dinosaure)
* PERFORMANCE mirage-crypto: ChaCha20/Poly1305 use string instead of Cstruct.t,
  ChaCha20 interface unchanged, performance improvement roughly 2x
  (mirage/mirage-crypto#203 @hannesm @reynir)
* mirage-crypto-ec, mirage-crypto-pk, mirage-crypto-rng: use digestif for
  hashes (mirage/mirage-crypto#212 mirage/mirage-crypto#215 @reynir @hannesm)
* mirage-crypto-rng: use a set for entropy sources instead of a list
  (mirage/mirage-crypto#218 @hannesm)
* mirage-crypto-rng-mirage: provide a module type S (for use instead of
  mirage-random in mirage) (mirage/mirage-crypto#234 @hannesm)
avsm pushed a commit to avsm/opam-repository that referenced this pull request Sep 5, 2024
CHANGES:

### Breaking changes

* mirage-crypto: Poly1305 API now uses string (mirage/mirage-crypto#203 @hannesm)
* mirage-crypto: Poly1305 no longer has type alias "type mac = string"
  (mirage/mirage-crypto#232 @hannesm)
* mirage-crypto: the API uses string instead of cstruct (mirage/mirage-crypto#214 @reynir @hannesm)
* mirage-crypto: Hash module has been removed. Use digestif if you need hash
  functions (mirage/mirage-crypto#213 @hannesm)
* mirage-crypto: the Cipher_block and Cipher_stream modules have been removed,
  its contents is inlined:
  Mirage_crypto.Cipher_block.S -> Mirage_crypto.Block
  Mirage_crypto.Cipher_stream.S -> Mirage_crypto.Stream
  Mirage_crypto.Cipher_block.AES.CTR -> Mirage_crypto.AES.CTR
  (mirage/mirage-crypto#225 @hannesm, suggested in mirage/mirage-crypto#224 by @reynir)
* mirage-crypto-pk: s-expression conversions for private and public keys (Dh,
  Dsa, Rsa) have been removed. You can use PKCS8 for encoding and decoding
  `X509.{Private,Public}_key.{en,de}code_{der,pem}` (mirage/mirage-crypto#208 @hannesm)
* mirage-crypto-pk: in the API, Cstruct.t is no longer present. Instead,
  string is used (mirage/mirage-crypto#211 @reynir @hannesm)
* mirage-crypto-rng: the API uses string instead of Cstruct.t. A new function
  `generate_into : ?g -> bytes -> ?off:int -> int -> unit` is provided
  (mirage/mirage-crypto#212 @hannesm @reynir)
* mirage-crypto-ec: remove NIST P224 support (mirage/mirage-crypto#209 @hannesm @Firobe)
* mirage-crypto: in Uncommon.xor_into the arguments ~src_off and ~dst_off are
  required now (mirage/mirage-crypto#232 @hannesm), renamed to unsafe_xor_into
  (98f01b14f5ebf98ba0e7e9c2ba97ec518f90fddc)
* mirage-crypto-pk, mirage-crypto-rng: remove type alias "type bits = int"
  (mirage/mirage-crypto#236 @hannesm)

### Bugfixes

* mirage-crypto (32 bit systems): CCM with long adata (mirage/mirage-crypto#207 @reynir)
* mirage-crypto-ec: fix K_gen for bitlen mod 8 != 0 (reported in mirage/mirage-crypto#105 that
  P521 test vectors don't pass, re-reported mirage/mirage-crypto#228, fixed mirage/mirage-crypto#230 @Firobe)
* mirage-crypto-ec: zero out bytes allocated for Field_element.zero (reported
  mirleft/ocaml-x509#167, fixed mirage/mirage-crypto#226 @dinosaure)

### Data race free

* mirage-crypto (3DES): avoid global state in key derivation (mirage/mirage-crypto#223 @hannesm)
* mirage-crypto-rng: use atomic instead of reference to be domain-safe (mirage/mirage-crypto#221
  @dinosaure @reynir @hannesm)
* mirage-crypto, mirage-crypto-rng, mirage-crypto-pk, mirage-crypto-ec:
  avoid global buffers, use freshly allocated strings/bytes instead, avoids
  data races (mirage/mirage-crypto#186 mirage/mirage-crypto#219 @dinosaure @reynir @hannesm)

### Other changes

* mirage-crypto: add {de,en}crypt_into functions (and unsafe variants) to allow
  less buffer allocations (mirage/mirage-crypto#231 @hannesm)
* mirage-crypto-rng-miou: new package which adds rng support with miou
  (mirage/mirage-crypto#227 @dinosaure)
* PERFORMANCE mirage-crypto: ChaCha20/Poly1305 use string instead of Cstruct.t,
  ChaCha20 interface unchanged, performance improvement roughly 2x
  (mirage/mirage-crypto#203 @hannesm @reynir)
* mirage-crypto-ec, mirage-crypto-pk, mirage-crypto-rng: use digestif for
  hashes (mirage/mirage-crypto#212 mirage/mirage-crypto#215 @reynir @hannesm)
* mirage-crypto-rng: use a set for entropy sources instead of a list
  (mirage/mirage-crypto#218 @hannesm)
* mirage-crypto-rng-mirage: provide a module type S (for use instead of
  mirage-random in mirage) (mirage/mirage-crypto#234 @hannesm)
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.

3 participants