Refactor clmul to use Union

[000wan]

Summary

It resolves #44.
Struct Clmul is modified to:

pub struct Clmul {
    inner: Inner,
    token: mul_intrinsics::InitToken,
}

with Union type Inner:

union Inner {
    intrinsics: intrinsics::Clmul,
    soft: soft::Clmul,
}

It follows the implementation of polyval using InitToken.

Results

#73 (comment)

References

https://doc.rust-lang.org/reference/items/unions.html
https://github.com/RustCrypto/universal-hashes/blob/master/polyval/src/backend/autodetect.rs
https://github.com/RustCrypto/utils/blob/master/cpufeatures/src/lib.rs

[sinui0]

Thanks for the PR!

Would it be possible to remove the match blocks all-together? The cpufeatures macro generates a ZST InitToken which can be used to replace is_intr in Clmul.

https://github.com/RustCrypto/universal-hashes/blob/fc463be29521177527377f02ada141c1ec329ae9/polyval/src/backend/autodetect.rs#L25-L56

This essentially moves the is_intr to a global atomic instead of storing a bool in every instance of Clmul. We can also avoid checking both instances in this case.

[000wan]

Oh, I see. Do you mean declaring is_intr for global state using Atomic module?

[000wan]

[Updated after 570d042]

So I've benched these both for intrinsics and soft, and here are the results:
(Compared to the dev branch, running $ cargo bench --bench clmul)

intrinsics:

clmul                   time:   [2.0842 ns 2.0942 ns 2.1036 ns]                   
                        change: [-80.054% -79.865% -79.674%] (p = 0.00 < 0.05)
                        Performance has improved.

clmul_reuse             time:   [6.7801 ns 6.8201 ns 6.8617 ns]                         
                        change: [-18.986% -14.868% -11.035%] (p = 0.00 < 0.05)
                        Performance has improved.

reduce                  time:   [668.48 ps 686.49 ps 705.57 ps]                    
                        change: [-94.706% -94.544% -94.380%] (p = 0.00 < 0.05)

soft:

clmul                   time:   [34.806 ns 35.014 ns 35.233 ns]                   
                        change: [-10.990% -9.9367% -8.8572%] (p = 0.00 < 0.05)
                        Performance has improved.

clmul_reuse             time:   [34.388 ns 34.735 ns 35.132 ns]                         
                        change: [-18.041% -16.826% -15.262%] (p = 0.00 < 0.05)
                        Performance has improved.

reduce                  time:   [1.7560 ns 1.7783 ns 1.8069 ns]                    
                        change: [-83.553% -83.150% -82.735%] (p = 0.00 < 0.05)
                        Performance has improved.

It seems clmul using intrinsics has been considerably improved, and using soft has a little.

[sinui0]

Nice results!

I think we can avoid depending on lazy_static, we already use cpufeatures.

You can do:

pub struct Clmul {
  inner: Inner,
  token: mul_instrinsics::InitToken
}

impl Clmul {
  pub fn clmul(self, other: Self) -> (Self, Self) {
    if self.token.get() {
      // self.inner is intrinsics
    } else {
      // self.inner is soft
    }
  }
}

InitToken is a ZST so this should be effectively the same, but 1 less dep

[sinui0]

Good work!

If you have a moment maybe you could check if inlining helps perf too

Thanks!

[themighty1]

Thank you, looks good!!!
Do you plan to add anything else, or can I merge this @000wan ?

[000wan]

I think we can merge this.
Thank you for kind reviews !! :)