diff --git a/src/include/robin_hood.h b/src/include/robin_hood.h index 77d3c591..70aeea6f 100644 --- a/src/include/robin_hood.h +++ b/src/include/robin_hood.h @@ -1106,7 +1106,7 @@ class Table using Node = DataNode; - // helpers for doInsert: extract first entry (only const required) + // helpers for insertKeyPrepareEmptySpot: extract first entry (only const required) ROBIN_HOOD(NODISCARD) key_type const& getFirstConst(Node const& n) const noexcept { return n.getFirst(); } @@ -1433,12 +1433,12 @@ class Table } // inserts a keyval that is guaranteed to be new, e.g. when the hashmap is resized. - // @return index where the element was created - size_t insert_move(Node&& keyval) { + // @return True on success, false if something went wrong + bool insert_move(Node&& keyval) { // we don't retry, fail if overflowing // don't need to check max num elements if (0 == mMaxNumElementsAllowed && !try_increase_info()) { - throwOverflowError(); // impossible to reach LCOV_EXCL_LINE + return false; } size_t idx{}; @@ -1475,7 +1475,7 @@ class Table mInfo[insertion_idx] = insertion_info; ++mNumElements; - return insertion_idx; + return true; } public: @@ -1712,13 +1712,54 @@ class Table template typename std::enable_if::value, Q&>::type operator[](const key_type& key) { ROBIN_HOOD_TRACE(this) - return doCreateByKey(key); + auto idxAndState = insertKeyPrepareEmptySpot(key); + switch (idxAndState.second) { + case InsertionState::key_found: + break; + + case InsertionState::new_node: + ::new (static_cast(&mKeyVals[idxAndState.first])) + Node(*this, std::piecewise_construct, std::forward_as_tuple(key), + std::forward_as_tuple()); + break; + + case InsertionState::overwrite_node: + mKeyVals[idxAndState.first] = Node(*this, std::piecewise_construct, + std::forward_as_tuple(key), std::forward_as_tuple()); + break; + + case InsertionState::overflow_error: + throwOverflowError(); + } + + return mKeyVals[idxAndState.first].getSecond(); } template typename std::enable_if::value, Q&>::type operator[](key_type&& key) { ROBIN_HOOD_TRACE(this) - return doCreateByKey(std::move(key)); + auto idxAndState = insertKeyPrepareEmptySpot(key); + switch (idxAndState.second) { + case InsertionState::key_found: + break; + + case InsertionState::new_node: + ::new (static_cast(&mKeyVals[idxAndState.first])) + Node(*this, std::piecewise_construct, std::forward_as_tuple(std::move(key)), + std::forward_as_tuple()); + break; + + case InsertionState::overwrite_node: + mKeyVals[idxAndState.first] = + Node(*this, std::piecewise_construct, std::forward_as_tuple(std::move(key)), + std::forward_as_tuple()); + break; + + case InsertionState::overflow_error: + throwOverflowError(); + } + + return mKeyVals[idxAndState.first].getSecond(); } template @@ -1733,13 +1774,28 @@ class Table std::pair emplace(Args&&... args) { ROBIN_HOOD_TRACE(this) Node n{*this, std::forward(args)...}; - auto r = doInsert(std::move(n)); - if (!r.second) { - // insertion not possible: destroy node - // NOLINTNEXTLINE(bugprone-use-after-move) + auto idxAndState = insertKeyPrepareEmptySpot(getFirstConst(n)); + switch (idxAndState.second) { + case InsertionState::key_found: + n.destroy(*this); + break; + + case InsertionState::new_node: + ::new (static_cast(&mKeyVals[idxAndState.first])) Node(*this, std::move(n)); + break; + + case InsertionState::overwrite_node: + mKeyVals[idxAndState.first] = std::move(n); + break; + + case InsertionState::overflow_error: n.destroy(*this); + throwOverflowError(); + break; } - return r; + + return std::make_pair(iterator(mKeyVals + idxAndState.first, mInfo + idxAndState.first), + InsertionState::key_found != idxAndState.second); } template @@ -1767,34 +1823,34 @@ class Table template std::pair insert_or_assign(const key_type& key, Mapped&& obj) { - return insert_or_assign_impl(key, std::forward(obj)); + return insertOrAssignImpl(key, std::forward(obj)); } template std::pair insert_or_assign(key_type&& key, Mapped&& obj) { - return insert_or_assign_impl(std::move(key), std::forward(obj)); + return insertOrAssignImpl(std::move(key), std::forward(obj)); } template std::pair insert_or_assign(const_iterator hint, const key_type& key, Mapped&& obj) { (void)hint; - return insert_or_assign_impl(key, std::forward(obj)); + return insertOrAssignImpl(key, std::forward(obj)); } template std::pair insert_or_assign(const_iterator hint, key_type&& key, Mapped&& obj) { (void)hint; - return insert_or_assign_impl(std::move(key), std::forward(obj)); + return insertOrAssignImpl(std::move(key), std::forward(obj)); } std::pair insert(const value_type& keyval) { ROBIN_HOOD_TRACE(this) - return doInsert(keyval); + return emplace(keyval); } std::pair insert(value_type&& keyval) { - return doInsert(std::move(keyval)); + return emplace(std::move(keyval)); } // Returns 1 if key is found, 0 otherwise. @@ -2006,7 +2062,9 @@ class Table // only actually do anything when the new size is bigger than the old one. This prevents to // continuously allocate for each reserve() call. if (newSize < mMask + 1) { - rehashPowerOfTwo(newSize, true); + if (!rehashPowerOfTwo(newSize, true)) { + throwOverflowError(); + } } } @@ -2114,13 +2172,16 @@ class Table // only actually do anything when the new size is bigger than the old one. This prevents to // continuously allocate for each reserve() call. if (forceRehash || newSize > mMask + 1) { - rehashPowerOfTwo(newSize, false); + if (!rehashPowerOfTwo(newSize, false)) { + throwOverflowError(); + } } } // reserves space for at least the specified number of elements. // only works if numBuckets if power of two - void rehashPowerOfTwo(size_t numBuckets, bool forceFree) { + // True on success, false otherwise + bool rehashPowerOfTwo(size_t numBuckets, bool forceFree) { ROBIN_HOOD_TRACE(this) Node* const oldKeyVals = mKeyVals; @@ -2129,11 +2190,13 @@ class Table const size_t oldMaxElementsWithBuffer = calcNumElementsWithBuffer(mMask + 1); // resize operation: move stuff - init_data(numBuckets); + initData(numBuckets); if (oldMaxElementsWithBuffer > 1) { for (size_t i = 0; i < oldMaxElementsWithBuffer; ++i) { if (oldInfo[i] != 0) { - insert_move(std::move(oldKeyVals[i])); + if (!insert_move(std::move(oldKeyVals[i]))) { + return false; + } // destroy the node but DON'T destroy the data. oldKeyVals[i].~Node(); } @@ -2151,6 +2214,7 @@ class Table } } } + return true; } ROBIN_HOOD(NOINLINE) void throwOverflowError() const { @@ -2164,27 +2228,63 @@ class Table template std::pair try_emplace_impl(OtherKey&& key, Args&&... args) { ROBIN_HOOD_TRACE(this) - auto it = find(key); - if (it == end()) { - return emplace(std::piecewise_construct, - std::forward_as_tuple(std::forward(key)), - std::forward_as_tuple(std::forward(args)...)); + auto idxAndState = insertKeyPrepareEmptySpot(key); + switch (idxAndState.second) { + case InsertionState::key_found: + break; + + case InsertionState::new_node: + ::new (static_cast(&mKeyVals[idxAndState.first])) Node( + *this, std::piecewise_construct, std::forward_as_tuple(std::forward(key)), + std::forward_as_tuple(std::forward(args)...)); + break; + + case InsertionState::overwrite_node: + mKeyVals[idxAndState.first] = Node(*this, std::piecewise_construct, + std::forward_as_tuple(std::forward(key)), + std::forward_as_tuple(std::forward(args)...)); + break; + + case InsertionState::overflow_error: + throwOverflowError(); + break; } - return {it, false}; + + return std::make_pair(iterator(mKeyVals + idxAndState.first, mInfo + idxAndState.first), + InsertionState::key_found != idxAndState.second); } template - std::pair insert_or_assign_impl(OtherKey&& key, Mapped&& obj) { + std::pair insertOrAssignImpl(OtherKey&& key, Mapped&& obj) { ROBIN_HOOD_TRACE(this) - auto it = find(key); - if (it == end()) { - return emplace(std::forward(key), std::forward(obj)); + auto idxAndState = insertKeyPrepareEmptySpot(key); + switch (idxAndState.second) { + case InsertionState::key_found: + mKeyVals[idxAndState.first].getSecond() = std::forward(obj); + break; + + case InsertionState::new_node: + ::new (static_cast(&mKeyVals[idxAndState.first])) Node( + *this, std::piecewise_construct, std::forward_as_tuple(std::forward(key)), + std::forward_as_tuple(std::forward(obj))); + break; + + case InsertionState::overwrite_node: + mKeyVals[idxAndState.first] = Node(*this, std::piecewise_construct, + std::forward_as_tuple(std::forward(key)), + std::forward_as_tuple(std::forward(obj))); + break; + + case InsertionState::overflow_error: + throwOverflowError(); + break; } - it->second = std::forward(obj); - return {it, false}; + + return std::make_pair(iterator(mKeyVals + idxAndState.first, mInfo + idxAndState.first), + InsertionState::key_found != idxAndState.second); } - void init_data(size_t max_elements) { + void initData(size_t max_elements) { mNumElements = 0; mMask = max_elements - 1; mMaxNumElementsAllowed = calcMaxNumElementsAllowed(max_elements); @@ -2206,86 +2306,34 @@ class Table mInfoHashShift = InitialInfoHashShift; } - template - typename std::enable_if::value, Q&>::type doCreateByKey(Arg&& key) { - while (true) { - size_t idx{}; - InfoType info{}; - keyToIdx(key, &idx, &info); - nextWhileLess(&info, &idx); - - // while we potentially have a match. Can't do a do-while here because when mInfo is - // 0 we don't want to skip forward - while (info == mInfo[idx]) { - if (WKeyEqual::operator()(key, mKeyVals[idx].getFirst())) { - // key already exists, do not insert. - return mKeyVals[idx].getSecond(); - } - next(&info, &idx); - } - - // unlikely that this evaluates to true - if (ROBIN_HOOD_UNLIKELY(mNumElements >= mMaxNumElementsAllowed)) { - increase_size(); - continue; - } - - // key not found, so we are now exactly where we want to insert it. - auto const insertion_idx = idx; - auto const insertion_info = info; - if (ROBIN_HOOD_UNLIKELY(insertion_info + mInfoInc > 0xFF)) { - mMaxNumElementsAllowed = 0; - } - - // find an empty spot - while (0 != mInfo[idx]) { - next(&info, &idx); - } - - auto& l = mKeyVals[insertion_idx]; - if (idx == insertion_idx) { - // put at empty spot. This forwards all arguments into the node where the object - // is constructed exactly where it is needed. - ::new (static_cast(&l)) - Node(*this, std::piecewise_construct, - std::forward_as_tuple(std::forward(key)), std::forward_as_tuple()); - } else { - shiftUp(idx, insertion_idx); - l = Node(*this, std::piecewise_construct, - std::forward_as_tuple(std::forward(key)), std::forward_as_tuple()); - } + enum class InsertionState { overflow_error, key_found, new_node, overwrite_node }; - // mKeyVals[idx].getFirst() = std::move(key); - mInfo[insertion_idx] = static_cast(insertion_info); - - ++mNumElements; - return mKeyVals[insertion_idx].getSecond(); - } - } - - // This is exactly the same code as operator[], except for the return values - template - std::pair doInsert(Arg&& keyval) { + // Finds key, and if not already present prepares a spot where to pot the key & value. + // This potentially shifts nodes out of the way, updates mInfo and number of inserted elements, + // so the only operation left to do is create/assign a new node at that spot. + template + std::pair insertKeyPrepareEmptySpot(OtherKey&& key) { while (true) { size_t idx{}; InfoType info{}; - keyToIdx(getFirstConst(keyval), &idx, &info); + keyToIdx(key, &idx, &info); nextWhileLess(&info, &idx); // while we potentially have a match while (info == mInfo[idx]) { - if (WKeyEqual::operator()(getFirstConst(keyval), mKeyVals[idx].getFirst())) { + if (WKeyEqual::operator()(key, mKeyVals[idx].getFirst())) { // key already exists, do NOT insert. // see http://en.cppreference.com/w/cpp/container/unordered_map/insert - return std::make_pair(iterator(mKeyVals + idx, mInfo + idx), - false); + return std::make_pair(idx, InsertionState::key_found); } next(&info, &idx); } // unlikely that this evaluates to true if (ROBIN_HOOD_UNLIKELY(mNumElements >= mMaxNumElementsAllowed)) { - increase_size(); + if (!increase_size()) { + return std::make_pair(size_t(0), InsertionState::overflow_error); + } continue; } @@ -2301,19 +2349,15 @@ class Table next(&info, &idx); } - auto& l = mKeyVals[insertion_idx]; - if (idx == insertion_idx) { - ::new (static_cast(&l)) Node(*this, std::forward(keyval)); - } else { + if (idx != insertion_idx) { shiftUp(idx, insertion_idx); - l = Node(*this, std::forward(keyval)); } - // put at empty spot mInfo[insertion_idx] = static_cast(insertion_info); - ++mNumElements; - return std::make_pair(iterator(mKeyVals + insertion_idx, mInfo + insertion_idx), true); + return std::make_pair(insertion_idx, idx == insertion_idx + ? InsertionState::new_node + : InsertionState::overwrite_node); } } @@ -2345,16 +2389,17 @@ class Table return true; } - void increase_size() { + // True if resize was possible, false otherwise + bool increase_size() { // nothing allocated yet? just allocate InitialNumElements if (0 == mMask) { - init_data(InitialNumElements); - return; + initData(InitialNumElements); + return true; } auto const maxNumElementsAllowed = calcMaxNumElementsAllowed(mMask + 1); if (mNumElements < maxNumElementsAllowed && try_increase_info()) { - return; + return true; } ROBIN_HOOD_LOG("mNumElements=" << mNumElements << ", maxNumElementsAllowed=" @@ -2363,10 +2408,10 @@ class Table (static_cast(mMask) + 1))) // it seems we have a really bad hash function! don't try to resize again if (mNumElements * 2 < calcMaxNumElementsAllowed(mMask + 1)) { - throwOverflowError(); + return false; } - rehashPowerOfTwo((mMask + 1) * 2, false); + return rehashPowerOfTwo((mMask + 1) * 2, false); } void destroy() { diff --git a/src/test/unit/unit_overflow_collisions.cpp b/src/test/unit/unit_overflow_collisions.cpp index 5db50cf7..cb456245 100644 --- a/src/test/unit/unit_overflow_collisions.cpp +++ b/src/test/unit/unit_overflow_collisions.cpp @@ -33,6 +33,7 @@ TEST_CASE_TEMPLATE( } REQUIRE(CtorDtorVerifier::mapSize() == 0); + // produce overflow with insert { Map m; for (uint64_t i = 0; i < max_val; ++i) { @@ -46,6 +47,21 @@ TEST_CASE_TEMPLATE( if (0 != CtorDtorVerifier::mapSize()) { CtorDtorVerifier::printMap(); } + + // produce overflow with emplace + { + Map m; + for (uint64_t i = 0; i < max_val; ++i) { + REQUIRE(m.insert(typename Map::value_type(i, i)).second); + } + REQUIRE(m.size() == max_val); + REQUIRE_THROWS_AS(m.emplace(typename Map::value_type(max_val, max_val)), + std::overflow_error); + REQUIRE(m.size() == max_val); + } + if (0 != CtorDtorVerifier::mapSize()) { + CtorDtorVerifier::printMap(); + } REQUIRE(CtorDtorVerifier::mapSize() == 0); }