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

Salsa20 SSE2 version #328

Merged
merged 5 commits into from
Jan 7, 2024
Merged

Salsa20 SSE2 version #328

merged 5 commits into from
Jan 7, 2024

Conversation

oxarbitrage
Copy link
Contributor

Part of #50

Hello, i had been trying to optimize salsa20 for sse2 since a while so i decided to publish a PR even if:

  • I had 0 experience with sse2 before so i might be doing some things wrong.
  • I have some experience with salsa20 cipher but not with the RustCrypto implementations.
  • The performance increase is not that great.

With that said, feel free to discard the PR if you think it will not worth doing. Otherwise, i am totally open to suggestions and feedback to make this mergable to the project. Please review carefully if so, i might be missing important stuff. Thanks!

With the current salsa20 implementation i get this output from benches on my current computer:

running 4 tests
test salsa20_bench1_16b   ... bench:          25 ns/iter (+/- 0) = 640 MB/s
test salsa20_bench2_256b  ... bench:         348 ns/iter (+/- 11) = 735 MB/s
test salsa20_bench3_1kib  ... bench:       1,375 ns/iter (+/- 113) = 744 MB/s
test salsa20_bench4_16kib ... bench:      23,348 ns/iter (+/- 2,823) = 701 MB/s

While with this PR:

running 4 tests
test salsa20_bench1_16b   ... bench:          22 ns/iter (+/- 0) = 727 MB/s
test salsa20_bench2_256b  ... bench:         304 ns/iter (+/- 3) = 842 MB/s
test salsa20_bench3_1kib  ... bench:       1,203 ns/iter (+/- 34) = 851 MB/s
test salsa20_bench4_16kib ... bench:      20,548 ns/iter (+/- 2,458) = 797 MB/s

This numbers can vary from run to run a bit but the performance increase is consistent.

@tarcieri
Copy link
Member

Apologies on the delayed review on this. I've glanced through it a few times and it seems mostly reasonable. I'd like to do a more detailed review before merging though, and it's been hard to justify prioritizing given the relatively meager performance gains.

@tarcieri
Copy link
Member

tarcieri commented Jan 7, 2024

Reviewed this again. It seems reasonable enough, and we're about to start making breaking changes anyway so I'd like to get it in before that.

@tarcieri tarcieri merged commit a8463f2 into RustCrypto:master Jan 7, 2024
56 checks passed
baloo added a commit to baloo/stream-ciphers that referenced this pull request Mar 4, 2024
This reverts RustCrypto#328.

The changes introduced here generate failure when used in `scrypt`. The
`scrypt_block_mix` would generate a different value.

I'm not able to figure out why that change breaks scrypt. Reverting
until we can figure out why.
baloo added a commit to baloo/stream-ciphers that referenced this pull request Mar 4, 2024
This reverts RustCrypto#328.

The changes introduced here generate failure when used in `scrypt`. The
`scrypt_block_mix` would generate a different value.

I'm not able to figure out why that change breaks scrypt. Reverting
until we can figure out why.
@baloo baloo mentioned this pull request Mar 4, 2024
tarcieri pushed a commit that referenced this pull request Mar 5, 2024
This reverts #328.

The changes introduced here generate failure when used in `scrypt`. The
`scrypt_block_mix` would generate a different value.

Note that this change was never released, so there is no security impact.
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.

2 participants