argon2: add parallelism #547
Conversation
|
Could you benchmark the parallel implementation and compare it against the single threaded one? |
This comment was marked as outdated.
This comment was marked as outdated.
|
Some benchmarks: Note: these are outdated since the removal of 018c3e9 due to #568 (comment). Benchmarking master...HEAD with parallel featureNote: 6-core CPU with SMT. Also: Benchmarking master...HEAD without parallel feature, default param tests only |
|
@jonasmalacofilho if you can rebase I added |
jonasmalacofilho
force-pushed
the
add-parallelism-to-argon2
branch
from
264821d to
018c3e9
Compare
|
@tarcieri oh, i forgot about that one. Rebased, and thanks for pointing it out! That said, we should probably try also to add the very cheapest of tests and have it run in Miri in CI:
|
|
By the way, I think there are some things I can improve in the code, but I would really appreciate a review first. And so I've kept edits to a minimum for now, so that you can actually review it. |
|
Yeah, Miri is tricky specifically because you can't do anything computationally expensive under it. I think we could potentially gate expensive tests under |
I think the 2_8_2 (t=2,m=2,p=2) tests are the cheapest in the crate, and still quite expensive... I could try adding t=1,m=8,p=2 tests and see if they execute in acceptable time in CI. Additionally, maybe a few unit tests ensuring that allowed borrows pass in Miri, and that some known invalid borrow patterns are either impossible at compile time or caught at runtime. |
|
Quick update: I ended up getting stuck trying to remove the (apparently) unrelated warnings from Miri (a warning in crossbeam and a leak due to rayon), and then I couldn't get to this PR for a few weeks. EDIT: (easily) running the tests in Miri is not currently possible due to crossbeam-rs/crossbeam#1181. Once that fix is released, it's possible that only Tree Borrows may work due to crossbeam-rs/crossbeam#545. |
jonasmalacofilho
force-pushed
the
add-parallelism-to-argon2
branch
2 times, most recently
from
31cecde to
0f5355a
Compare
|
I removed the conflict, rebased the PR, fixed/updated the benchmarks and did some other minor cleanup. Crossbeam-epoch doesn't currently work in Miri (see my edited comment above). Between that and the fact that even the most minimal Miri test would be super slow on GitHub free runners, I just don't think they are worth it for now. (It should be still possible to get an older toolchain and Miri and run some specific tests locally). Is there's something else you would like me to add here? |
|
@jonasmalacofilho still need to go through it in detail, but there's nothing I see that's an immediate blocker |
|
Just interesting, this or is done and waiting for merge or need some work? |
|
I still need to review it. Sorry it's been on the backburner since it's difficult to review due to the use of unsafe code and because we're currently working on more fundamental crates which are a blocker for another stable In the meantime I appreciate any review others can offer. |
|
Apologies for not getting to this yet. I hope to get to it soon, especially as we get closer to final releases. @jonasmalacofilho in the meantime, can you fix the conflict? Thanks |
jonasmalacofilho
force-pushed
the
add-parallelism-to-argon2
branch
from
0fd407c to
dc29161
Compare
Coordinated shared access in the memory blocks is implemented with `SegmentViewIter` and associated types, which provide views into Argon2 memory that can be processed in parallel. These views alias in the regions that are read-only, but are disjoint in the regions where mutation happens. Effectively, they implement, with a combination of mutable borrowing and runtime checking, the cooperative contract outlined in RFC 9106. To avoid aliasing mutable references into the entire buffer of blocks (which would be UB), pointers are used up to the moment where a reference (shared or mutable) into a specific block is returned. At that point, aliasing is no longer possible, as argued in SAFETY comments and/or checked at runtime. Finally, add a `parallel` feature and parallelize filling the blocks using the memory views mentioned above and rayon.
This was cause by having multiple different versions of criterion, and therefore the train, in use: we specified ^0.4, but pprof 0.14.0 already required ^0.5.
Additionally, use a set instead of trying to avoid repeating a particular set of params by hand.
jonasmalacofilho
force-pushed
the
add-parallelism-to-argon2
branch
from
dc29161 to
3a380c3
Compare
There was a problem hiding this comment.
This seems plausible to me, and the benchmark improvements look impressive.
Would appreciate a second set of eyes from @newpavlov before merging.
There was a problem hiding this comment.
I will try to take a closer look at the implementation during this weekend.
Meanwhile, maybe we should introduce a separate feature-gated type (or generic parameter) for the parallel implementation instead of switching to it under the hood by enabling the feature?
|
@newpavlov IMO it should "just work" when the feature is enabled |
|
I don't have a strong opinion here, so I am fine with either, but my minor concern here is that |
|
Yeah, that's a reasonable concern. I guess my answer there would be to file an issue against those dependencies and have them propagate an off-by-default |
|
I simply followed what you guys did on Parallelism is also gated by the number of lanes, which the defender usually controls. And parallelism |
jonasmalacofilho
force-pushed
the
add-parallelism-to-argon2
branch
from
bcc9c03 to
cdeac38
Compare
To pass the newly enabled clippy lints, add a missing safety comment to the compress_avx2 call.
jonasmalacofilho
force-pushed
the
add-parallelism-to-argon2
branch
from
cdeac38 to
fb4b0f4
Compare
Adds a
default-enabledparallelfeature, with anotherwiseoptional dependency onrayon, and parallelize the filling of blocks using the memory views mentioned above.Coordinated shared access in the memory blocks is implemented with a
SegmentViewIteriterator, which implements eitherrayon::iter::ParallelIteratororcore::iter::Iteratorand returnsSegmentViewviews into the Argon2 blocks memory that are safe to be used in parallel.The views alias in the regions that are read-only, but are disjoint in the regions where mutation happens. Effectively, they implement, with a combination of mutable borrowing and runtime checking, the cooperative contract outlined in RFC 9106. This is similar to what was suggested in #380.
To avoid aliasing mutable references into the entire buffer of blocks (which would be UB), pointers are used up to the moment where a reference (shared or mutable) into a specific block is returned. At that point, aliasing is no longer possible.
The following tests have been tried in and pass Miri (modulo unrelated warnings):
(Running these in Miri is quite slow, taking ~5 minutes each, so I only ran the most obviously relevant tests for now).
Finally, the alignment ofBlocks increases to 128 bytes for better prevention of false sharing on modern platforms. The new value is based on notes oncrossbeam-utils::CachePadded.I also took some inspiration from an intermediate snapshot of #247, before the parallel implementation was removed, as well as from an implementation without any safe abstractions I just worked on for the rust-argon2 crate (sru-systems/rust-argon2#56).