From: Dave Watson Date: Tue, 7 Nov 2017 18:27:36 +0000 (-0800) Subject: Support movable keys X-Git-Tag: v2017.11.13.00~25 X-Git-Url: http://demsky.eecs.uci.edu/git/?a=commitdiff_plain;h=103ad3dac0bac41b1bfcc86db8499592c855119f;p=folly.git Support movable keys Summary: Use the same trick as the values, so that non-copyable keys can be used in ConcurrentHashMap. Reviewed By: yfeldblum Differential Revision: D6252711 fbshipit-source-id: f0f168c4eb361d372bdfc3417f32222d66c11aaf --- diff --git a/folly/concurrency/ConcurrentHashMap.h b/folly/concurrency/ConcurrentHashMap.h index 4d8bd6e2..b4d2fc3a 100644 --- a/folly/concurrency/ConcurrentHashMap.h +++ b/folly/concurrency/ConcurrentHashMap.h @@ -201,25 +201,27 @@ class ConcurrentHashMap { return res; } - std::pair insert(const KeyType& k, const ValueType& v) { + template + std::pair insert(Key&& k, Value&& v) { auto segment = pickSegment(k); std::pair res( std::piecewise_construct, std::forward_as_tuple(this, segment), std::forward_as_tuple(false)); - res.second = ensureSegment(segment)->insert(res.first.it_, k, v); + res.second = ensureSegment(segment)->insert( + res.first.it_, std::forward(k), std::forward(v)); return res; } - template - std::pair try_emplace(const KeyType& k, Args&&... args) { + template + std::pair try_emplace(Key&& k, Args&&... args) { auto segment = pickSegment(k); std::pair res( std::piecewise_construct, std::forward_as_tuple(this, segment), std::forward_as_tuple(false)); res.second = ensureSegment(segment)->try_emplace( - res.first.it_, k, std::forward(args)...); + res.first.it_, std::forward(k), std::forward(args)...); return res; } @@ -242,26 +244,28 @@ class ConcurrentHashMap { return res; } - std::pair insert_or_assign( - const KeyType& k, - const ValueType& v) { + template + std::pair insert_or_assign(Key&& k, Value&& v) { auto segment = pickSegment(k); std::pair res( std::piecewise_construct, std::forward_as_tuple(this, segment), std::forward_as_tuple(false)); - res.second = ensureSegment(segment)->insert_or_assign(res.first.it_, k, v); + res.second = ensureSegment(segment)->insert_or_assign( + res.first.it_, std::forward(k), std::forward(v)); return res; } - folly::Optional assign(const KeyType& k, const ValueType& v) { + template + folly::Optional assign(Key&& k, Value&& v) { auto segment = pickSegment(k); ConstIterator res(this, segment); auto seg = segments_[segment].load(std::memory_order_acquire); if (!seg) { return folly::Optional(); } else { - auto r = seg->assign(res.it_, k, v); + auto r = + seg->assign(res.it_, std::forward(k), std::forward(v)); if (!r) { return folly::Optional(); } @@ -270,17 +274,20 @@ class ConcurrentHashMap { } // Assign to desired if and only if key k is equal to expected - folly::Optional assign_if_equal( - const KeyType& k, - const ValueType& expected, - const ValueType& desired) { + template + folly::Optional + assign_if_equal(Key&& k, const ValueType& expected, Value&& desired) { auto segment = pickSegment(k); ConstIterator res(this, segment); auto seg = segments_[segment].load(std::memory_order_acquire); if (!seg) { return folly::Optional(); } else { - auto r = seg->assign_if_equal(res.it_, k, expected, desired); + auto r = seg->assign_if_equal( + res.it_, + std::forward(k), + expected, + std::forward(desired)); if (!r) { return folly::Optional(); } diff --git a/folly/concurrency/detail/ConcurrentHashMap-detail.h b/folly/concurrency/detail/ConcurrentHashMap-detail.h index 6af07fee..ecdf13a4 100644 --- a/folly/concurrency/detail/ConcurrentHashMap-detail.h +++ b/folly/concurrency/detail/ConcurrentHashMap-detail.h @@ -47,11 +47,11 @@ class ValueHolder { explicit ValueHolder(const ValueHolder& other) : item_(other.item_) {} - template - ValueHolder(const KeyType& k, Args&&... args) + template + ValueHolder(std::piecewise_construct_t, Arg&& k, Args&&... args) : item_( std::piecewise_construct, - std::forward_as_tuple(k), + std::forward_as_tuple(std::forward(k)), std::forward_as_tuple(std::forward(args)...)) {} value_type& getItem() { return item_; @@ -69,7 +69,9 @@ class ValueHolder< KeyType, ValueType, Allocator, - std::enable_if_t::value>> { + std::enable_if_t< + !std::is_nothrow_copy_constructible::value || + !std::is_nothrow_copy_constructible::value>> { public: typedef std::pair value_type; @@ -78,12 +80,12 @@ class ValueHolder< item_ = other.item_; } - template - ValueHolder(const KeyType& k, Args&&... args) { + template + ValueHolder(std::piecewise_construct_t, Arg&& k, Args&&... args) { item_ = (value_type*)Allocator().allocate(sizeof(value_type)); new (item_) value_type( std::piecewise_construct, - std::forward_as_tuple(k), + std::forward_as_tuple(std::forward(k)), std::forward_as_tuple(std::forward(args)...)); } @@ -116,9 +118,12 @@ class NodeT : public folly::hazptr::hazptr_obj_base< explicit NodeT(NodeT* other) : item_(other->item_) {} - template - NodeT(const KeyType& k, Args&&... args) - : item_(k, std::forward(args)...) {} + template + NodeT(Arg&& k, Args&&... args) + : item_( + std::piecewise_construct, + std::forward(k), + std::forward(args)...) {} /* Nodes are refcounted: If a node is retired() while a writer is traversing the chain, the rest of the chain must remain valid @@ -244,19 +249,20 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { } bool insert(Iterator& it, std::pair&& foo) { - return insert(it, foo.first, foo.second); + return insert(it, std::move(foo.first), std::move(foo.second)); } - bool insert(Iterator& it, const KeyType& k, const ValueType& v) { + template + bool insert(Iterator& it, Key&& k, Value&& v) { auto node = (Node*)Allocator().allocate(sizeof(Node)); - new (node) Node(k, v); + new (node) Node(std::forward(k), std::forward(v)); auto res = insert_internal( it, - k, + node->getItem().first, InsertType::DOES_NOT_EXIST, [](const ValueType&) { return false; }, node, - v); + node); if (!res) { node->~Node(); Allocator().deallocate((uint8_t*)node, sizeof(Node)); @@ -264,14 +270,17 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { return res; } - template - bool try_emplace(Iterator& it, const KeyType& k, Args&&... args) { + template + bool try_emplace(Iterator& it, Key&& k, Args&&... args) { + // Note: first key is only ever compared. Second is moved in to + // create the node, and the first key is never touched again. return insert_internal( it, - k, + std::forward(k), InsertType::DOES_NOT_EXIST, [](const ValueType&) { return false; }, nullptr, + std::forward(k), std::forward(args)...); } @@ -282,29 +291,39 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { k, InsertType::DOES_NOT_EXIST, [](const ValueType&) { return false; }, + node, node); } - bool insert_or_assign(Iterator& it, const KeyType& k, const ValueType& v) { - return insert_internal( + template + bool insert_or_assign(Iterator& it, Key&& k, Value&& v) { + auto node = (Node*)Allocator().allocate(sizeof(Node)); + new (node) Node(std::forward(k), std::forward(v)); + auto res = insert_internal( it, - k, + node->getItem().first, InsertType::ANY, [](const ValueType&) { return false; }, - nullptr, - v); + node, + node); + if (!res) { + node->~Node(); + Allocator().deallocate((uint8_t*)node, sizeof(Node)); + } + return res; } - bool assign(Iterator& it, const KeyType& k, const ValueType& v) { + template + bool assign(Iterator& it, Key&& k, Value&& v) { auto node = (Node*)Allocator().allocate(sizeof(Node)); - new (node) Node(k, v); + new (node) Node(std::forward(k), std::forward(v)); auto res = insert_internal( it, - k, + node->getItem().first, InsertType::MUST_EXIST, [](const ValueType&) { return false; }, node, - v); + node); if (!res) { node->~Node(); Allocator().deallocate((uint8_t*)node, sizeof(Node)); @@ -312,18 +331,26 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { return res; } + template bool assign_if_equal( Iterator& it, - const KeyType& k, + Key&& k, const ValueType& expected, - const ValueType& desired) { - return insert_internal( + Value&& desired) { + auto node = (Node*)Allocator().allocate(sizeof(Node)); + new (node) Node(std::forward(k), std::forward(desired)); + auto res = insert_internal( it, - k, + node->getItem().first, InsertType::MATCH, - [expected](const ValueType& v) { return v == expected; }, - nullptr, - desired); + [&expected](const ValueType& v) { return v == expected; }, + node, + node); + if (!res) { + node->~Node(); + Allocator().deallocate((uint8_t*)node, sizeof(Node)); + } + return res; } template @@ -369,7 +396,7 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { } else { if (!cur) { cur = (Node*)Allocator().allocate(sizeof(Node)); - new (cur) Node(k, std::forward(args)...); + new (cur) Node(std::forward(args)...); } auto next = node->next_.load(std::memory_order_relaxed); cur->next_.store(next, std::memory_order_relaxed); @@ -415,7 +442,7 @@ class FOLLY_ALIGNED(64) ConcurrentHashMapSegment { // OR DOES_NOT_EXIST, but only in the try_emplace case DCHECK(type == InsertType::ANY || type == InsertType::DOES_NOT_EXIST); cur = (Node*)Allocator().allocate(sizeof(Node)); - new (cur) Node(k, std::forward(args)...); + new (cur) Node(std::forward(args)...); } cur->next_.store(headnode, std::memory_order_relaxed); head->store(cur, std::memory_order_release); diff --git a/folly/concurrency/test/ConcurrentHashMapTest.cpp b/folly/concurrency/test/ConcurrentHashMapTest.cpp index 824d37da..b1b18e42 100644 --- a/folly/concurrency/test/ConcurrentHashMapTest.cpp +++ b/folly/concurrency/test/ConcurrentHashMapTest.cpp @@ -114,7 +114,8 @@ int foo::copied{0}; TEST(ConcurrentHashMap, EmplaceTest) { ConcurrentHashMap foomap(200); - foomap.insert(1, foo()); + foo bar; // Make sure to test copy + foomap.insert(1, bar); EXPECT_EQ(foo::moved, 0); EXPECT_EQ(foo::copied, 1); foo::copied = 0; @@ -151,12 +152,21 @@ TEST(ConcurrentHashMap, MapResizeTest) { // Ensure we can insert objects without copy constructors. TEST(ConcurrentHashMap, MapNoCopiesTest) { struct Uncopyable { + int i_; Uncopyable(int i) { - (void*)&i; + i_ = i; } Uncopyable(const Uncopyable& that) = delete; + bool operator==(const Uncopyable& o) const { + return i_ == o.i_; + } + }; + struct Hasher { + size_t operator()(const Uncopyable&) const { + return 0; + } }; - ConcurrentHashMap foomap(2); + ConcurrentHashMap foomap(2); EXPECT_TRUE(foomap.try_emplace(1, 1).second); EXPECT_TRUE(foomap.try_emplace(2, 2).second); auto res = foomap.find(2); @@ -169,6 +179,37 @@ TEST(ConcurrentHashMap, MapNoCopiesTest) { EXPECT_EQ(&(res->second), &(res2->second)); } +TEST(ConcurrentHashMap, MapMovableKeysTest) { + struct Movable { + int i_; + Movable(int i) { + i_ = i; + } + Movable(const Movable&) = delete; + Movable(Movable&& o) { + i_ = o.i_; + o.i_ = 0; + } + bool operator==(const Movable& o) const { + return i_ == o.i_; + } + }; + struct Hasher { + size_t operator()(const Movable&) const { + return 0; + } + }; + ConcurrentHashMap foomap(2); + EXPECT_TRUE(foomap.insert(std::make_pair(Movable(10), Movable(1))).second); + EXPECT_TRUE(foomap.assign(Movable(10), Movable(2))); + EXPECT_TRUE(foomap.insert(Movable(11), Movable(1)).second); + EXPECT_TRUE(foomap.emplace(Movable(12), Movable(1)).second); + EXPECT_TRUE(foomap.insert_or_assign(Movable(10), Movable(3)).second); + EXPECT_TRUE(foomap.assign_if_equal(Movable(10), Movable(3), Movable(4))); + EXPECT_FALSE(foomap.try_emplace(Movable(10), Movable(3)).second); + EXPECT_TRUE(foomap.try_emplace(Movable(13), Movable(3)).second); +} + TEST(ConcurrentHashMap, MapUpdateTest) { ConcurrentHashMap foomap(2); EXPECT_TRUE(foomap.insert(1, 10).second);