Switched to concurrent HashMap implementation in the resource-manager [ECR-2934] #727

skletsun · 2019-02-21T08:55:31Z

Overview

Switched from RwLock to chashmap::CHashMap for the handles storage in the resources manager.

See: https://jira.bf.local/browse/ECR-XYZ

Definition of Done

There are no TODOs left in the code
Change is covered by automated tests
The coding guidelines are followed
Public API has Javadoc
Method preconditions are checked and documented in the Javadoc of the method
Changelog is updated if needed (in case of notable or breaking changes)
The continuous integration build passes

…anager.

vitvakatu · 2019-02-21T09:18:12Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

- .write()
- .expect("Unable to obtain write-lock")
- .remove(&handle);
+ HANDLES_MAP.remove(&handle);


Interesting, CHashMap provides methods with internal mutability

skletsun · 2019-02-21T09:30:57Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs


 use utils::Handle;

+const MAP_CAPACITY: usize = 512;


I've chosen this value with the idea that reasonably big value will keep the load factor small enough spending not much memory to the initial pre-allocation.

Wait, how the map can determine the load factor from the capacity (for an empty map)? These are supposedly different things. Why don't we either pass a load factor (if you have a really good intuition of which is right) or don't pass anything?

Sorry for confusing you, I just meant that CHashMap::with_capacity(N) pre-allocates some amount of buckets depending on N, thus I supposed that defining some reasonable value here could prevent it from expensive re-allocations in future. In other words, it's rather a question if it is possible to estimate this or it is fully unpredictable and there is no sense in such optimizations.

coveralls · 2019-02-21T09:37:13Z

Coverage remained the same at 85.53% when pulling 6bbb072 on skletsun:concurrent-hashmap-resource-manager-ecr-2934 into 89f15e6 on exonum:master.

dmitry-timofeev

The code does not seem to be properly synchronized.

dmitry-timofeev · 2019-02-21T10:37:49Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs


 use utils::Handle;

+const MAP_CAPACITY: usize = 512;


Wait, how the map can determine the load factor from the capacity (for an empty map)? These are supposedly different things. Why don't we either pass a load factor (if you have a really good intuition of which is right) or don't pass anything?

dmitry-timofeev · 2019-02-21T10:39:19Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

- .expect("Unable to obtain write-lock")
 .insert(handle, HandleInfo::new(TypeId::of::<T>(), ownership))
 .is_none(),
 "Trying to add the same handle for the second time: {:X}",


Assuming insert returns the previous value, does there happen an overwrite of the previous value (i.e., the change of the state of the map in case of an invalid argument)?

Re-written using CHashMap.alter()

FYI, I bumped into another library, DashMap, it has proper get_or_insert in its API.

Hovewer, even though it is 1.0+, there are no concurrent tests and even complete functional tests 👎

dmitry-timofeev · 2019-02-21T10:49:00Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

 /// # Panics
 ///
 /// See `check_handle_impl` for details.
 fn remove_handle_impl<T: 'static>(handle: Handle, ownership: HandleOwnershipType) {


This method is not correctly synchronized, as it won't detect concurrent double-free:

T1 T2 c->OK c->OK r->removed r->nothing to remove (OK)

dmitry-timofeev · 2019-02-21T13:12:58Z

Why don't we also push all these operations in a struct that can be instantiated in both lazy_static and unit tests? Otherwise it is quite hard to test operations on a static shared state.

dmitry-timofeev

Thanks, such implementation, given all operations in chashmap are properly synchronized, would work. However, I think tests need improvements. Also, shan't there be any concurrent tests?

dmitry-timofeev · 2019-02-27T09:58:19Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

+ fn check_handle_type_and_ownership<T: 'static>(
+ handle: Handle,
+ handle_info: &HandleInfo,
+ ownership: Option<HandleOwnershipType>,


Why is it an Option? I see that the only usage with None comes from check_handle — why is that?

dmitry-timofeev · 2019-02-27T10:05:23Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

- MANAGE_HANDLES_FIRST_HANDLE,
- Some(HandleOwnershipType::JavaOwned),
+ fn check_handle_type_and_ownership_ok() {
+ let handle_info = prepare_handle_info();


Here and elsewhere: I do not think prepare_handle_info works in such cases because subsequent assertions do rely on what it puts there. Either

This method must be parameterized.

Converted to a builder, relevant to the test data must be set up in each test that depends on that data

If no parameterization needed — given a name that reflects what it creates.

Currently it is not clear what all these tests using prepare_handle_info test.

Please see also https://testing.googleblog.com/2018/02/testing-on-toilet-cleanly-create-test.html

dmitry-timofeev · 2019-02-27T10:08:33Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

- Some(HandleOwnershipType::JavaOwned),
+ fn check_handle_type_and_ownership_ok() {
+ let handle_info = prepare_handle_info();
+ HandlesStorage::check_handle_type_and_ownership::<HandleFooType>(


Do we need tests using this internal static method? Wouldn't it be simpler to

Create a fresh HandleStorage.

Setup the test data using public API.

Perform the action we'd like to test.

Verify.
?

dmitry-timofeev · 2019-02-27T10:10:20Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

- Some(HandleOwnershipType::NativeOwned),
- );
+ #[test]
+ fn manage_handles() {


Please split into more focused tests for operations of rm. There are already some good focused tests below — I'd consider identifying which cases are currently missing and extending the suite of such focused tests.

dmitry-timofeev · 2019-02-27T10:11:54Z

exonum-java-binding/core/rust/src/utils/resource_manager/imp.rs

- add_handle::<T>(WRONG_TYPE_HANDLE);
- enum OtherT {}
- check_handle::<OtherT>(WRONG_TYPE_HANDLE);
+ fn prepare_storage() -> HandlesStorage {


create_storage?

Switched from RwLock<HashMap> to chashmap::CHashMap in the resource-m…

3ba5e7f

…anager.

skletsun requested review from dmitry-timofeev and vitvakatu February 21, 2019 08:55

vitvakatu reviewed Feb 21, 2019

View reviewed changes

vitvakatu approved these changes Feb 21, 2019

View reviewed changes

skletsun commented Feb 21, 2019

View reviewed changes

dmitry-timofeev suggested changes Feb 21, 2019

View reviewed changes

Fixed missing handle.

6bbb072

skletsun added 2 commits February 26, 2019 17:32

Refactoring, enhancements in the code that changes the state of map.

3386e35

Merge branch 'master' into concurrent-hashmap-resource-manager-ecr-2934

434cb9b

dmitry-timofeev requested review from bullet-tooth and vitvakatu February 27, 2019 08:52

dmitry-timofeev suggested changes Feb 27, 2019

View reviewed changes

Updated chashmap dependency.

5501236

dmitry-timofeev added the work-in-progress 👷‍♂️ Do not expect reviewers to provide any feedback on WIP PRs — please ask for it explicitly! label May 20, 2019

dmitry-timofeev force-pushed the master branch from 0f784e4 to fae6873 Compare May 23, 2019 15:52

vitvakatu removed their request for review August 16, 2019 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Switched to concurrent HashMap implementation in the resource-manager [ECR-2934] #727

Switched to concurrent HashMap implementation in the resource-manager [ECR-2934] #727

Uh oh!

skletsun commented Feb 21, 2019 •

edited

Loading

vitvakatu Feb 21, 2019

skletsun Feb 21, 2019

dmitry-timofeev Feb 21, 2019

skletsun Feb 26, 2019

coveralls commented Feb 21, 2019 •

edited

Loading

dmitry-timofeev left a comment

dmitry-timofeev Feb 21, 2019

dmitry-timofeev Feb 21, 2019

skletsun Feb 26, 2019

dmitry-timofeev Dec 6, 2019

dmitry-timofeev Feb 21, 2019

dmitry-timofeev commented Feb 21, 2019

dmitry-timofeev left a comment

dmitry-timofeev Feb 27, 2019

dmitry-timofeev Feb 27, 2019

dmitry-timofeev Feb 27, 2019

dmitry-timofeev Feb 27, 2019

dmitry-timofeev Feb 27, 2019

dmitry-timofeev Feb 27, 2019

Labels

4 participants

Switched to concurrent HashMap implementation in the resource-manager [ECR-2934] #727

Are you sure you want to change the base?

Switched to concurrent HashMap implementation in the resource-manager [ECR-2934] #727

Uh oh!

Conversation

skletsun commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Overview

Definition of Done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitry-timofeev commented Feb 21, 2019

dmitry-timofeev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

skletsun commented Feb 21, 2019 •

edited

Loading

coveralls commented Feb 21, 2019 •

edited

Loading