Colfenor / classic-mceliece-rust

This is a pure-rust safe-rust implementation of the Classic McEliece post-quantum scheme
MIT License
24 stars 11 forks source link

Use native int load and store instead of homegrown bitshifts #18

Closed faern closed 2 years ago

faern commented 2 years ago

I found that the util module contains a bunch of bit shifting code that has equivalents in the standard library. The standard library versions probably compile to more optimized code, and this crate does not have to take responsibility for the correctness. I find that this change makes the code easier to read, and less likely to introduce errors.

I could also see a performance improvement thanks to this. When I ran the benchmarks locally on my computer the kem_enc benchmark reports this:

kem_enc                 time:   [238068.6381 cycles 243812.5489 cycles 249344.6288 cycles]
                        change: [-9.6026% -7.2295% -4.5918%] (p = 0.00 < 0.05)
                        Performance has improved.

:partying_face:

prokls commented 2 years ago

This was certainly just overlooked when porting the C code. I am totally in favor of merging. I will run the testsuite and then merge it :smiley:

faern commented 2 years ago

I fully respect sticking to the reference impl 100% initially when implementing and proving correctness. Optimizations like these can come slowly over time :)

faern commented 2 years ago

Should I rebase this on top of the v2 branch and have it merged there instead? I forgot that existed. I of course want this included in the coming v2 branch. This optimization is not very important to get into the 1.0 releases anyway. I aim for 2.0 here :)

Colfenor commented 2 years ago

heyo @faern thank you for the PR :) ! I'm happy that the project is interesting to you and has some use <3

I think a rebase on v2 sounds good, sorry for being idle for quite some time I had other stuff to do.

Concerning general improvements of the project, i'll try to get a little time to fix more trivial tasks (e.g. clippy errors) since @dkales & @prokls probably have a more in-depth understanding of (rust) cryptography architecture than I do currently.

But I am following your discussions and try to make sense of the decisions

faern commented 2 years ago

Awesome! I look forward to help improve this bit by bit over the coming weeks as well.