From 13853156fc38518c2f637790b9f1807c1ad9a517 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Fri, 1 May 2020 20:27:22 -0500 Subject: [PATCH 1/9] Add "contains" function to parallel_hash_map that also returns the value is a thread-safe manner Fixed a bug in the call to lazy_emplace --- parallel_hashmap/phmap.h | 35 ++++++++++++++++++++++++++++------- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index 1635172..f2b596f 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -2866,7 +2866,7 @@ public: Inner& inner = sets_[subidx(hashval)]; auto& set = inner.set_; typename Lockable::UniqueLock m(inner); - return make_iterator(&inner, set.lazy_emplace(key, hashval, std::forward(f))); + return make_iterator(&inner, set.lazy_emplace_with_hash(key, hashval, std::forward(f))); } // Extension API: support for heterogeneous keys. @@ -3032,11 +3032,8 @@ public: // -------------------------------------------------------------------- template iterator find(const key_arg& key, size_t hashval) { - Inner& inner = sets_[subidx(hashval)]; - auto& set = inner.set_; - typename Lockable::SharedLock m(inner); - auto it = set.find(key, hashval); - return make_iterator(&inner, it); + typename Lockable::SharedLock m; + return find(key, hashval, m); } template @@ -3194,6 +3191,15 @@ private: } protected: + template + iterator find(const key_arg& key, size_t hashval, typename Lockable::SharedLock &mutexlock) { + Inner& inner = sets_[subidx(hashval)]; + auto& set = inner.set_; + mutexlock = std::move(typename Lockable::SharedLock(inner)); + auto it = set.find(key, hashval); + return make_iterator(&inner, it); + } + template std::tuple find_or_prepare_insert(const K& key, typename Lockable::UniqueLock &mutexlock) { @@ -3219,7 +3225,7 @@ protected: } template - size_t hash(const K& key) { + size_t hash(const K& key) const { return HashElement{hash_ref()}(key); } @@ -3387,6 +3393,21 @@ public: return Policy::value(&*it); } + template + bool contains(const key_arg& key) const { + return Base::contains(key); + } + + template + bool contains(const key_arg& key, V& v) const { + typename Lockable::SharedLock m; + auto it = const_cast(this)->find(key, hash(key), m); + if (it == this->end()) + return false; + v = Policy::value(&*it); + return true; + } + template MappedReference

operator[](key_arg&& key) { return Policy::value(&*try_emplace(std::forward(key)).first); From 0e723143fa87766ee944b69900b6a65e8bc88555 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 16:47:33 -0500 Subject: [PATCH 2/9] Add "if_contains" to parallel_hash_map so the value does not need to be copied to be read in a thread-safe manner Add tests for parallel_hash_map thread-safe functions. --- parallel_hashmap/phmap.h | 10 +++++++++ tests/flat_hash_map_test.cc | 1 + tests/parallel_flat_hash_map_test.cc | 1 + tests/parallel_hash_map_test.cc | 32 ++++++++++++++++++++++++++++ tests/parallel_node_hash_map_test.cc | 1 + 5 files changed, 45 insertions(+) create mode 100644 tests/parallel_hash_map_test.cc diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index f2b596f..8ae2ca6 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3408,6 +3408,16 @@ public: return true; } + template + bool if_contains(const key_arg& key, F&& f) { + typename Lockable::SharedLock m; + auto it = const_cast(this)->find(key, hash(key), m); + if (it == this->end()) + return false; + std::forward(f)(Policy::value(&*it)); + return true; + } + template MappedReference

operator[](key_arg&& key) { return Policy::value(&*try_emplace(std::forward(key)).first); diff --git a/tests/flat_hash_map_test.cc b/tests/flat_hash_map_test.cc index 316c9f7..5f0e57f 100644 --- a/tests/flat_hash_map_test.cc +++ b/tests/flat_hash_map_test.cc @@ -244,6 +244,7 @@ TEST(THIS_TEST_NAME, MergeExtractInsert) { m.insert(std::move(node)); EXPECT_THAT(m, UnorderedElementsAre(Pair(1, 17), Pair(2, 9))); } + #if !defined(__ANDROID__) && !defined(__APPLE__) && !defined(__EMSCRIPTEN__) && defined(PHMAP_HAVE_STD_ANY) TEST(THIS_TEST_NAME, Any) { ThisMap m; diff --git a/tests/parallel_flat_hash_map_test.cc b/tests/parallel_flat_hash_map_test.cc index d1e6288..500bc3c 100644 --- a/tests/parallel_flat_hash_map_test.cc +++ b/tests/parallel_flat_hash_map_test.cc @@ -2,3 +2,4 @@ #define THIS_TEST_NAME ParallelFlatHashMap #include "flat_hash_map_test.cc" +#include "parallel_hash_map_test.cc" diff --git a/tests/parallel_hash_map_test.cc b/tests/parallel_hash_map_test.cc new file mode 100644 index 0000000..b729118 --- /dev/null +++ b/tests/parallel_hash_map_test.cc @@ -0,0 +1,32 @@ +#ifndef THIS_HASH_MAP + #define THIS_HASH_MAP parallel_flat_hash_map + #define THIS_TEST_NAME ParallelFlatHashMap +#endif + +namespace phmap { +namespace container_internal { +namespace { + +TEST(THIS_TEST_NAME, ThreadSafeContains) { + // We can't test mutable keys, or non-copyable keys with ThisMap. + // Test that the nodes have the proper API. + ThisMap m = { {1, 7}, {2, 9} }; + auto val = 0; + EXPECT_TRUE(m.contains(1, val)); + EXPECT_EQ(val, 7); + + EXPECT_FALSE(m.contains(3, val)); + +#if __cplusplus > 199711L + auto func = [&val](int& v) { val = v; }; + EXPECT_TRUE(m.if_contains(2, func)); + EXPECT_EQ(val, 9); + + EXPECT_FALSE(m.if_contains(3, func)); +#endif + +} + +} // namespace +} // namespace container_internal +} // namespace phmap diff --git a/tests/parallel_node_hash_map_test.cc b/tests/parallel_node_hash_map_test.cc index 615a0ba..ea51222 100644 --- a/tests/parallel_node_hash_map_test.cc +++ b/tests/parallel_node_hash_map_test.cc @@ -2,3 +2,4 @@ #define THIS_TEST_NAME ParallelNodeHashMap #include "flat_hash_map_test.cc" +#include "parallel_hash_map_test.cc" From adc918235e129cd0a417fb5af86fd11f8da44e65 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 17:09:10 -0500 Subject: [PATCH 3/9] Add missing const --- parallel_hashmap/phmap.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index 8ae2ca6..dca52bf 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3409,7 +3409,7 @@ public: } template - bool if_contains(const key_arg& key, F&& f) { + bool if_contains(const key_arg& key, F&& f) const { typename Lockable::SharedLock m; auto it = const_cast(this)->find(key, hash(key), m); if (it == this->end()) From 131a84f6ed35fa30faa13285fb20044898f18e60 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 18:25:02 -0500 Subject: [PATCH 4/9] Fix compiler error on clang/gcc --- parallel_hashmap/phmap.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index dca52bf..406697a 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3401,7 +3401,7 @@ public: template bool contains(const key_arg& key, V& v) const { typename Lockable::SharedLock m; - auto it = const_cast(this)->find(key, hash(key), m); + auto it = const_cast(this)->find(key, Base::hash(key), m); if (it == this->end()) return false; v = Policy::value(&*it); @@ -3411,7 +3411,7 @@ public: template bool if_contains(const key_arg& key, F&& f) const { typename Lockable::SharedLock m; - auto it = const_cast(this)->find(key, hash(key), m); + auto it = const_cast(this)->find(key, Base::hash(key), m); if (it == this->end()) return false; std::forward(f)(Policy::value(&*it)); From bf7203b3c82c414be5a2fe8afe355f89004377b6 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 20:45:46 -0500 Subject: [PATCH 5/9] Renamed the thread-safe "contains" to if_contains. Add tests to parallel_flat_hash_map_mutex_test --- parallel_hashmap/phmap.h | 15 ++++++--------- tests/parallel_flat_hash_map_mutex_test.cc | 2 +- tests/parallel_flat_hash_map_test.cc | 1 - tests/parallel_hash_map_test.cc | 6 ++++-- tests/parallel_node_hash_map_test.cc | 1 - 5 files changed, 11 insertions(+), 14 deletions(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index 406697a..b89393e 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3393,15 +3393,11 @@ public: return Policy::value(&*it); } - template - bool contains(const key_arg& key) const { - return Base::contains(key); - } - template - bool contains(const key_arg& key, V& v) const { + std::enable_if_t::value, + bool> if_contains(const key_arg& key, V& v) const { typename Lockable::SharedLock m; - auto it = const_cast(this)->find(key, Base::hash(key), m); + auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) return false; v = Policy::value(&*it); @@ -3409,9 +3405,10 @@ public: } template - bool if_contains(const key_arg& key, F&& f) const { + std::enable_if_t::value && std::is_invocable::value, + bool> if_contains(const key_arg& key, F&& f) const { typename Lockable::SharedLock m; - auto it = const_cast(this)->find(key, Base::hash(key), m); + auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) return false; std::forward(f)(Policy::value(&*it)); diff --git a/tests/parallel_flat_hash_map_mutex_test.cc b/tests/parallel_flat_hash_map_mutex_test.cc index 9c20932..73dbf7b 100644 --- a/tests/parallel_flat_hash_map_mutex_test.cc +++ b/tests/parallel_flat_hash_map_mutex_test.cc @@ -9,4 +9,4 @@ #define THIS_EXTRA_TPL_PARAMS , 4, boost::upgrade_mutex #endif -#include "flat_hash_map_test.cc" +#include "parallel_hash_map_test.cc" diff --git a/tests/parallel_flat_hash_map_test.cc b/tests/parallel_flat_hash_map_test.cc index 500bc3c..a1a8149 100644 --- a/tests/parallel_flat_hash_map_test.cc +++ b/tests/parallel_flat_hash_map_test.cc @@ -1,5 +1,4 @@ #define THIS_HASH_MAP parallel_flat_hash_map #define THIS_TEST_NAME ParallelFlatHashMap -#include "flat_hash_map_test.cc" #include "parallel_hash_map_test.cc" diff --git a/tests/parallel_hash_map_test.cc b/tests/parallel_hash_map_test.cc index b729118..61de5d3 100644 --- a/tests/parallel_hash_map_test.cc +++ b/tests/parallel_hash_map_test.cc @@ -3,6 +3,8 @@ #define THIS_TEST_NAME ParallelFlatHashMap #endif +#include "flat_hash_map_test.cc" + namespace phmap { namespace container_internal { namespace { @@ -12,10 +14,10 @@ TEST(THIS_TEST_NAME, ThreadSafeContains) { // Test that the nodes have the proper API. ThisMap m = { {1, 7}, {2, 9} }; auto val = 0; - EXPECT_TRUE(m.contains(1, val)); + EXPECT_TRUE(m.if_contains(1, val)); EXPECT_EQ(val, 7); - EXPECT_FALSE(m.contains(3, val)); + EXPECT_FALSE(m.if_contains(3, val)); #if __cplusplus > 199711L auto func = [&val](int& v) { val = v; }; diff --git a/tests/parallel_node_hash_map_test.cc b/tests/parallel_node_hash_map_test.cc index ea51222..f589d69 100644 --- a/tests/parallel_node_hash_map_test.cc +++ b/tests/parallel_node_hash_map_test.cc @@ -1,5 +1,4 @@ #define THIS_HASH_MAP parallel_node_hash_map #define THIS_TEST_NAME ParallelNodeHashMap -#include "flat_hash_map_test.cc" #include "parallel_hash_map_test.cc" From 3bfc1784ad7609e6d3aec2f5e422528f7b0a67e5 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 21:23:59 -0500 Subject: [PATCH 6/9] Fix compiler error for is_invocable is a c++17 feature --- parallel_hashmap/phmap.h | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index b89393e..0a38a7d 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3394,8 +3394,8 @@ public: } template - std::enable_if_t::value, - bool> if_contains(const key_arg& key, V& v) const { + std::enable_if_t::value + , bool> if_contains(const key_arg& key, V& v) const { typename Lockable::SharedLock m; auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) @@ -3405,8 +3405,11 @@ public: } template - std::enable_if_t::value && std::is_invocable::value, - bool> if_contains(const key_arg& key, F&& f) const { + std::enable_if_t::value +#if __cplusplus >= 201703L + && std::is_invocable::value +#endif + , bool> if_contains(const key_arg& key, F&& f) const { typename Lockable::SharedLock m; auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) From 73b0e6344565cf921718272a265d7c088cbf6c96 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 21:39:23 -0500 Subject: [PATCH 7/9] Fixed enable_if_t is a c++14 feature --- parallel_hashmap/phmap.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index 0a38a7d..4791996 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3394,8 +3394,8 @@ public: } template - std::enable_if_t::value - , bool> if_contains(const key_arg& key, V& v) const { + typename std::enable_if::value + , bool>::type if_contains(const key_arg& key, V& v) const { typename Lockable::SharedLock m; auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) @@ -3405,11 +3405,11 @@ public: } template - std::enable_if_t::value + typename std::enable_if::value #if __cplusplus >= 201703L && std::is_invocable::value #endif - , bool> if_contains(const key_arg& key, F&& f) const { + , bool>::type if_contains(const key_arg& key, F&& f) const { typename Lockable::SharedLock m; auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) From fa7a5cc4449cb1d42fac9bf34f600a27e4191537 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sat, 2 May 2020 22:07:51 -0500 Subject: [PATCH 8/9] Update the documentation to include information about the "if_contains" function --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index b6c4576..77f2003 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,7 @@ Parallel Hashmap containers follow the thread safety rules of the Standard C++ l - It is safe to read and write to one instance of a type even if another thread is reading or writing to a different instance of the same type. For example, given hash tables A and B of the same type, it is safe if A is being written in thread 1 and B is being read in thread 2. -- The *parallel* tables can be made internally thread-safe for concurrent write access, by providing a synchronization type (for example [std::mutex](https://en.cppreference.com/w/cpp/thread/mutex)) as the last template argument. Because locking is performed at the *submap* level, a high level of concurrency can still be achieved. However please be aware that returned iterators are not protected by the mutex, so they cannot be used reliably on a hash map which can be changed by another thread. Again, the internal synchronization does not allow to retrieve data from a table that is being modified in another thread. +- The *parallel* tables can be made internally thread-safe for concurrent read and write access, by providing a synchronization type (for example [std::mutex](https://en.cppreference.com/w/cpp/thread/mutex)) as the last template argument. Because locking is performed at the *submap* level, a high level of concurrency can still be achieved. Read access can be done safely using `if_contains()`, which either copies the value or passes it by reference to a callback while holding the *submap* lock. However, please be aware that returned iterators are not protected by the mutex, so they cannot be used reliably on a hash map which can be changed by another thread. - Examples on how to use various mutex types, including boost::mutex, boost::shared_mutex and absl::Mutex can be found in `examples/bench.cc` From a1349d2b65982fa74e7a04306277f2c963602009 Mon Sep 17 00:00:00 2001 From: Brian McKinnon Date: Sun, 3 May 2020 10:32:36 -0500 Subject: [PATCH 9/9] Make a single if_contains function to simplify the interface --- README.md | 2 +- parallel_hashmap/phmap.h | 16 ++-------------- tests/parallel_hash_map_test.cc | 7 ------- 3 files changed, 3 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index 77f2003..2f38906 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,7 @@ Parallel Hashmap containers follow the thread safety rules of the Standard C++ l - It is safe to read and write to one instance of a type even if another thread is reading or writing to a different instance of the same type. For example, given hash tables A and B of the same type, it is safe if A is being written in thread 1 and B is being read in thread 2. -- The *parallel* tables can be made internally thread-safe for concurrent read and write access, by providing a synchronization type (for example [std::mutex](https://en.cppreference.com/w/cpp/thread/mutex)) as the last template argument. Because locking is performed at the *submap* level, a high level of concurrency can still be achieved. Read access can be done safely using `if_contains()`, which either copies the value or passes it by reference to a callback while holding the *submap* lock. However, please be aware that returned iterators are not protected by the mutex, so they cannot be used reliably on a hash map which can be changed by another thread. +- The *parallel* tables can be made internally thread-safe for concurrent read and write access, by providing a synchronization type (for example [std::mutex](https://en.cppreference.com/w/cpp/thread/mutex)) as the last template argument. Because locking is performed at the *submap* level, a high level of concurrency can still be achieved. Read access can be done safely using `if_contains()`, which passes a reference value to the callback while holding the *submap* lock. However, please be aware that returned iterators are not protected by the mutex, so they cannot be used reliably on a hash map which can be changed by another thread. - Examples on how to use various mutex types, including boost::mutex, boost::shared_mutex and absl::Mutex can be found in `examples/bench.cc` diff --git a/parallel_hashmap/phmap.h b/parallel_hashmap/phmap.h index 4791996..7c1cd95 100644 --- a/parallel_hashmap/phmap.h +++ b/parallel_hashmap/phmap.h @@ -3393,23 +3393,11 @@ public: return Policy::value(&*it); } - template - typename std::enable_if::value - , bool>::type if_contains(const key_arg& key, V& v) const { - typename Lockable::SharedLock m; - auto it = const_cast(this)->find(key, this->hash(key), m); - if (it == this->end()) - return false; - v = Policy::value(&*it); - return true; - } - template - typename std::enable_if::value + bool if_contains(const key_arg& key, F&& f) const { #if __cplusplus >= 201703L - && std::is_invocable::value + static_assert(std::is_invocable::value); #endif - , bool>::type if_contains(const key_arg& key, F&& f) const { typename Lockable::SharedLock m; auto it = const_cast(this)->find(key, this->hash(key), m); if (it == this->end()) diff --git a/tests/parallel_hash_map_test.cc b/tests/parallel_hash_map_test.cc index 61de5d3..c08b578 100644 --- a/tests/parallel_hash_map_test.cc +++ b/tests/parallel_hash_map_test.cc @@ -14,19 +14,12 @@ TEST(THIS_TEST_NAME, ThreadSafeContains) { // Test that the nodes have the proper API. ThisMap m = { {1, 7}, {2, 9} }; auto val = 0; - EXPECT_TRUE(m.if_contains(1, val)); - EXPECT_EQ(val, 7); - EXPECT_FALSE(m.if_contains(3, val)); - -#if __cplusplus > 199711L auto func = [&val](int& v) { val = v; }; EXPECT_TRUE(m.if_contains(2, func)); EXPECT_EQ(val, 9); EXPECT_FALSE(m.if_contains(3, func)); -#endif - } } // namespace