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

Add lock free data structures to wsv #984

Merged
merged 1 commit into from
Apr 28, 2021
Merged

Conversation

i1i1
Copy link

@i1i1 i1i1 commented Apr 20, 2021

https://jira.hyperledger.org/browse/IR-1065
https://jira.hyperledger.org/browse/IR-1066
Signed-off-by: i1i1 [email protected]

Description of the Change

Remove lock from WSV and make maps and sets mutable concurrently. It uses dashmap crate for that.

Benefits

This change removes racing for whole wsv and adds locks for its subparts.

Possible Drawbacks

Lockfree actually caused several panics and needs for rework.

Usage Examples or Tests [optional]

Alternate Designs [optional]

dashmap is also good, it offers entry api and some lock free functions, but there are cautions about deadlocks. It is probably a good candidate, but should be used wisely.
lockfree is very generic and good crate, but it seems very unsafe to use it, as it can panic. Also it is quiete slow
flurry is a port of java hashmap, so it seems to be very robust, but it requires using crossbeam epoch in api, so we need to drastically change it in order to use it.

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Apr 20, 2021
@i1i1 i1i1 marked this pull request as ready for review April 20, 2021 15:12
@i1i1 i1i1 force-pushed the lockfree branch 9 times, most recently from b7386ae to ba58d73 Compare April 21, 2021 12:35
rkharisov
rkharisov previously approved these changes Apr 23, 2021
@e-ivkov
Copy link
Contributor

e-ivkov commented Apr 23, 2021

@i1i1 I remember you were doing performance tests for both of the crates, can you publish the results here?

iroha_structs/Cargo.toml Outdated Show resolved Hide resolved
iroha_structs/src/rwlock.rs Show resolved Hide resolved
iroha_data_model/src/lib.rs Outdated Show resolved Hide resolved
@i1i1
Copy link
Author

i1i1 commented Apr 24, 2021

Benchmark of lockfree vs dashmap:

Insert scenario (50000 Inserts per thread):

Number of threads Lockfree time(ms) Dashmap time(ms) Mutexmap* (ms)
1 27.926 4.7712 5.5090
2 60.090 6.3390 21.185
3 98.009 7.9950 36.675
4 116.96 8.6376 67.014
5 152.93 16.902 130.57
6 191.50 19.881 206.81
7 229.63 23.266 282.35
8 267.37 28.643 328.59

Generally dashmap 5-10x faster than lockfree.

More benchmarks here:
https://github.com/i1i1/dashmap_vs_lockfree

  • Mutexmap is actually async_std::sync::Mutex<std::collections::HashMap>

rkharisov
rkharisov previously approved these changes Apr 26, 2021
@i1i1 i1i1 force-pushed the lockfree branch 2 times, most recently from f3f0ff4 to 2158004 Compare April 26, 2021 11:47
iroha_data_model/src/lib.rs Outdated Show resolved Hide resolved
iroha_data_model/src/lib.rs Show resolved Hide resolved
iroha_data_model/src/lib.rs Show resolved Hide resolved
iroha_data_model/src/lib.rs Show resolved Hide resolved
iroha_data_model/src/lib.rs Show resolved Hide resolved
iroha_data_model/src/lib.rs Outdated Show resolved Hide resolved
@e-ivkov
Copy link
Contributor

e-ivkov commented Apr 26, 2021

@i1i1 in the benchmarks mutexmap name is a bit confusing. Consider changing it to std HashMap behind async_std Mutex

@e-ivkov
Copy link
Contributor

e-ivkov commented Apr 26, 2021

Or you can mark it with * and describe it below.

@i1i1
Copy link
Author

i1i1 commented Apr 26, 2021

@eadventurous looked through every RwLock in iroha_data_model. Unfortunately using HashSet/HashMap there would require removing Ord everywhere. I guess it is better to leave BTreeMap/BTreeSet for this small cases.

@i1i1
Copy link
Author

i1i1 commented Apr 26, 2021

Possible design for wsv (the same as before but with some lifetime wrangling)
https://gist.github.com/i1i1/cd84e1c5d5f29a1175033ff17ce4fe1c

@i1i1 i1i1 force-pushed the lockfree branch 3 times, most recently from bd2d22e to c11ecad Compare April 27, 2021 09:20
@e-ivkov
Copy link
Contributor

e-ivkov commented Apr 27, 2021

I was concerned about using dashmap in the async context because of the spinlocks (I thought they might steal computation time from other tasks as they are not cooperating). But I investigated the issue more and it seems they do cooperate. I was testing with ASYNC_STD_THREAD_COUNT=1 - the most extreme case, and other tasks can still proceed even if one of the tasks was locked there, waiting for write access.

Also there is an issue for async support for dashmap, and it is mentioned there that it is fully compatible xacrimon/dashmap#25

@i1i1
Copy link
Author

i1i1 commented Apr 27, 2021

@eadventurous Yeah it won't make sense as it would require locking and removing spinlocks from datastructure. Let's just keep that in mind and never take mutable refence for dashmap as it might deadlock whole datastructure.

@e-ivkov
Copy link
Contributor

e-ivkov commented Apr 27, 2021

Mutable references will sometimes be taken, as we do mutate WSV it is just a matter of for how long, but anyway, I checked this and it seems working even in this extreme case, and it does not block the whole thread.

@i1i1
Copy link
Author

i1i1 commented Apr 27, 2021

@eadventurous we don't need to take mutable reference (&self for rust) as we can mutate it through datastructures like rwlock or dashmap/dashset. We have variety of options

@i1i1 i1i1 requested review from rkharisov and e-ivkov April 27, 2021 12:11
Copy link
Contributor

@e-ivkov e-ivkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finally :)

@i1i1
Copy link
Author

i1i1 commented Apr 27, 2021

@eadventurous It will live here https://jira.hyperledger.org/browse/IR-1078

@i1i1 i1i1 merged commit 1db9e69 into hyperledger:iroha2-dev Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants