RustCrypto / asm-hashes

Assembly implementations of cryptographic hash functions
46 stars 26 forks source link

fix: use slices in benches #31

Closed haraldh closed 3 years ago

tarcieri commented 3 years ago

Not sure I understand the purpose of this change?

haraldh commented 3 years ago

Hmm, with the current master:

❯ cargo +nightly bench
   Compiling sha2-asm v0.6.0 (/home/harald/git/asm-hashes/sha2)
   Compiling whirlpool-asm v0.6.0 (/home/harald/git/asm-hashes/whirlpool)
   Compiling sha1-asm v0.5.0 (/home/harald/git/asm-hashes/sha1)
   Compiling md5-asm v0.5.0 (/home/harald/git/asm-hashes/md5)
error[E0308]: mismatched types
  --> whirlpool/benches/lib.rs:14:33
   |
14 |         whirlpool_asm::compress(&mut state, &data);
   |                                 ^^^^^^^^^^ expected `u64`, found `u8`
   |
   = note: expected mutable reference `&mut [u64; 8]`
              found mutable reference `&mut [u8; 64]`

error[E0308]: mismatched types
  --> md5/benches/lib.rs:14:39
   |
14 |         md5_asm::compress(&mut state, &data);
   |                                       ^^^^^ expected slice `[[u8; 64]]`, found array `[u8; 64]`
   |
   = note: expected reference `&[[u8; 64]]`
              found reference `&[u8; 64]`

error[E0308]: mismatched types
  --> sha1/benches/lib.rs:14:40
   |
14 |         sha1_asm::compress(&mut state, &data);
   |                                        ^^^^^ expected slice `[[u8; 64]]`, found array `[u8; 64]`
   |
   = note: expected reference `&[[u8; 64]]`
              found reference `&[u8; 64]`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `sha1-asm`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error[E0308]: mismatched types
  --> whirlpool/benches/lib.rs:14:45
   |
14 |         whirlpool_asm::compress(&mut state, &data);
   |                                             ^^^^^ expected slice `[[u8; 64]]`, found array `[u8; 64]`
   |
   = note: expected reference `&[[u8; 64]]`
              found reference `&[u8; 64]`

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0308`.
error: build failed
tarcieri commented 3 years ago

Huh strange. It builds for me with:

rustc 1.52.0-nightly (476acbf1e 2021-03-03)

and just to check there wasn't a recent regression, I also updated:

rustc 1.52.0-nightly (36f1f04f1 2021-03-17)

Seems to be working here:

   Compiling whirlpool v0.9.0 (/Users/bascule/src/RustCrypto/hashes/whirlpool)
    Finished bench [optimized] target(s) in 4.88s

running 0 tests

test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

running 4 tests
test bench1_10    ... bench:          94 ns/iter (+/- 27) = 106 MB/s
test bench2_100   ... bench:         820 ns/iter (+/- 154) = 121 MB/s
test bench3_1000  ... bench:       7,907 ns/iter (+/- 1,421) = 126 MB/s

What nightly are you using?

haraldh commented 3 years ago
❯ rustc +nightly --version
rustc 1.52.0-nightly (8f349be27 2021-03-08)
haraldh commented 3 years ago

still fails with:

❯ rustc +nightly --version
rustc 1.52.0-nightly (f5d8117c3 2021-03-16)

Are you sure the github master is in sync?

tarcieri commented 3 years ago

Yep. Working for me here.

Maybe try updating to the latest nightly?

newpavlov commented 3 years ago

Yeah, probably @tarcieri has not updated the master branch. It looks I've forgot to update benchmarks in #30 (and we do not check them in CI). I think it could be worth to pass not a single-element slice, but with many elements (e.g. 1024).

haraldh commented 3 years ago

@tarcieri you are in the wrong repo.. try asm-hashes :-)

tarcieri commented 3 years ago

Oh whoops! My bad, heh

tarcieri commented 3 years ago

Looks like this regressed due to #30 /cc @newpavlov

We should probably add checks the benchmarks build in CI.

That said, it looks like this may be needed after all.

newpavlov commented 3 years ago

I guess we can merge it and update benchmarks further in a separate PR.