birkeh/o3de
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Fixed bug in hash_table that made rehash() function run forever (#2745)

Browse Source 
* Fixed bug in hash_table that made rehash() function to run infinitely on specific conditions when inserting an already existing element

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Replaced erasing to happen in the source list instead

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* minor comment improvement

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Small commment improvement

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Small comment fix

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Added assert and fixed code with incorrect hashing

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* .

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Addressed PR comments, reverted to void* as it size_t hash is different

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Fixed build on linux

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

* Addressed PR comments

Signed-off-by: Garcia Ruiz <aljanru@amazon.co.uk>

Co-authored-by: Garcia Ruiz <aljanru@amazon.co.uk>
This commit is contained in:
AMZN-AlexOteiza
2021-08-04 02:38:18 +01:00
committed by GitHub
parent 24740b3f86
commit 45ebf57d3f
4 changed files with 95 additions and 27 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/std/hash.cpp
+1
View File
@@ -21,6 +21,7 @@ namespace AZStd
1610612741ul, 3221225473ul, 4294967291ul
};
// Bucket size suitable to hold n elements.
AZStd::size_t hash_next_bucket_size(AZStd::size_t n)
{
const AZStd::size_t* first = prime_list;
Code/Framework/AzCore/AzCore/std/hash_table.h
+42 -26
View File
@@ -134,6 +134,7 @@ namespace AZStd
void rehash(HashTable* table, size_type numBucketsMin)
{
size_type num_buckets = 0;
numBucketsMin = (AZStd::max)(numBucketsMin, (size_type)ceilf((float)m_list.size() / m_max_load_factor));
if (numBucketsMin != 0)
@@ -143,7 +144,7 @@ namespace AZStd
if (num_buckets == m_numBuckets)
{
return; // no point
return; // no need yet to rehash
}
m_numBuckets = num_buckets;
@@ -165,32 +166,43 @@ namespace AZStd
while (!m_list.empty())
{
cur = m_list.begin();
typename list_type::iterator insertIter, curEnd(cur);
const typename HashTable::key_type& valueKey = Traits::key_from_value(*cur);
typename list_type::iterator newIter, iter(cur);
size_type numValues = 1;
for (++iter; iter != last && table->m_keyEqual(Traits::key_from_value(*cur), Traits::key_from_value(*iter)); ++iter, ++numValues)
// Get the number of same consecutive elements in the table with same key,
// this allows range insertion of elements at once
for (++curEnd; curEnd != last && table->m_keyEqual(valueKey, Traits::key_from_value(*curEnd)); ++curEnd, ++numValues)
{
}
;
const typename HashTable::key_type& valueKey = Traits::key_from_value(*cur);
size_type newBucketIndex = table->bucket_from_hash(table->m_hasher(valueKey));
// newBucket.first holds the total number of elements in the bucket
// newBucket.second contains the pointer to the first element in the bucket
vector_value_type& newBucket = newBuckets[newBucketIndex];
size_type numElements = newBucket.first;
newIter = newBucket.second;
insertIter = newBucket.second;
// If we don't have elements in the bucket yet, transfer the elements directly
if (numElements == 0)
{
newList.splice(newList.begin(), m_list, cur, iter);
newList.splice(newList.begin(), m_list, cur, curEnd);
newBucket.second = newList.begin();
}
else
{
if (!table->find_insert_position(valueKey, table->m_keyEqual, newIter, numElements, integral_constant<bool, Traits::has_multi_elements>()))
// Since there are elements already in the bucket, update `insertIter` to where the elements will need to be inserted.
if (!table->find_insert_position(valueKey, table->m_keyEqual, insertIter, numElements, integral_constant<bool, Traits::has_multi_elements>()))
{
continue;
// An element was found but we don't allow for duplicate elements in this table.
// This happens when there was an insertion of two elements that are equal but have different hashes,
// which is undefined behavior for a hash table: ISO C++ N4713, section 23.14.15 - 5.3
AZ_Assert(false, "Found a duplicate element when rehashing. "
"Review the hashing function for this type and make sure two equal elements always have the same hash");
}
newList.splice(newIter, m_list, cur, iter);
newList.splice(insertIter, m_list, cur, curEnd);
}
newBucket.first += numValues;
@@ -251,15 +263,15 @@ namespace AZStd
m_vector.set_allocator(typename vector_type::allocator_type(&m_allocator));
}
allocator_type m_allocator; ///< The single instance of the allocator shared between list and vector containers.
list_type m_list; ///< List with elements.
vector_type m_vector; ///< Buckets with list iterators.
allocator_type m_allocator; //!< The single instance of the allocator shared between list and vector containers.
list_type m_list; //!< List with elements.
vector_type m_vector; //!< Buckets with list iterators.
private:
vector_value_type* m_buckets; ///< Current buckets array. (can point to the m_vector or m_startBucket).
size_type m_numBuckets; ///< Current number of buckets.
float m_max_load_factor;
vector_value_type m_startBucket; ///< Start bucket used for before we start dynamically allocate memory from m_vector.
vector_value_type* m_buckets; //!< Current buckets array. (can point to the m_vector or m_startBucket).
size_type m_numBuckets; //!< Current number of buckets.
float m_max_load_factor; //!< Maximum load (elements/buckets) before rehashing.
vector_value_type m_startBucket; //!< Start bucket used for before we start dynamically allocate memory from m_vector.
};
/**
@@ -321,8 +333,8 @@ namespace AZStd
template<class HashTable>
AZ_FORCE_INLINE void rehash(HashTable*, size_type) {}
vector_type m_vector; ///< Buckets with list iterators.
list_type m_list; ///< List with elements.
vector_type m_vector; //!< Buckets with list iterators.
list_type m_list; //!< List with elements.
};
}
@@ -972,28 +984,32 @@ namespace AZStd
rhs.clear();
}
// find_insert_position sets insertIter to where the element should be inserted
// and returns true if the element should be inserted, otherwise false
template<class ComparableToKey, class KeyEq>
bool find_insert_position(const ComparableToKey& keyCmp, const KeyEq& keyEq, iterator& iter, size_type numElements, const true_type& /* is multi elements */)
bool find_insert_position(const ComparableToKey& keyCmp, const KeyEq& keyEq, iterator& insertIter, size_type numElements, const true_type& /* is multi elements */)
{
for (size_type i = 0; i < numElements; ++i, ++iter)
for (size_type i = 0; i < numElements; ++i, ++insertIter)
{
if (keyEq(keyCmp, Traits::key_from_value(*iter)))
if (keyEq(keyCmp, Traits::key_from_value(*insertIter)))
{
++iter;
++insertIter;
break;
}
}
// always return true since multi elements (like multiset) allow repeated elements
return true;
}
template<class ComparableToKey, class KeyEq>
bool find_insert_position(const ComparableToKey& keyCmp, const KeyEq& keyEq, iterator& iter, size_type numElements, const false_type& /* !is multi elements */)
bool find_insert_position(const ComparableToKey& keyCmp, const KeyEq& keyEq, iterator& insertIter, size_type numElements, const false_type& /* !is multi elements */)
{
for (size_type i = 0; i < numElements; ++i, ++iter)
for (size_type i = 0; i < numElements; ++i, ++insertIter)
{
if (keyEq(keyCmp, Traits::key_from_value(*iter)))
if (keyEq(keyCmp, Traits::key_from_value(*insertIter)))
{
// Element already exists, it shouldn't be inserted as we don't allow more than one repeated element for this specialization
return false;
}
}
Code/Framework/AzCore/Tests/AZStd/Hashed.cpp
+49
View File
@@ -287,6 +287,55 @@ namespace UnitTest
}
}
TEST_F(HashedContainers, HashTable_InsertionDuplicateOnRehash)
{
struct TwoPtrs
{
void* m_ptr1;
void* m_ptr2;
bool operator==(const TwoPtrs& other) const
{
if (m_ptr1 == other.m_ptr1)
{
return m_ptr2 == other.m_ptr2;
}
else if (m_ptr1 == other.m_ptr2)
{
return m_ptr2 == other.m_ptr1;
}
return false;
}
};
// This hashing function produces different hashes for two equal values,
// which violates the requirement for hashing functions.
// The test makes sure that this does not reproduce an issue that caused the insert() function to loop infinitely.
struct TwoPtrsHasher
{
size_t operator()(const TwoPtrs& p) const
{
size_t hash{ 0 };
AZStd::hash_combine(hash, p.m_ptr1, p.m_ptr2);
return hash;
}
};
using PairSet = AZStd::unordered_set<TwoPtrs, TwoPtrsHasher>;
PairSet set;
set.insert({ (void*)1, (void*)2 });
set.insert({ (void*)3, (void*)4 });
set.insert({ (void*)5, (void*)6 });
set.insert({ (void*)7, (void*)8 });
// Elements with different hashes, but equal
set.insert({ (void*)0x000001ceddd9ca20, (void*)0x000001ceddd9cba0 }); // hash(148335135725641)
set.insert({ (void*)0x000001ceddd9cba0, (void*)0x000001ceddd9ca20 }); // hash(148335135764189)
AZ_TEST_START_TRACE_SUPPRESSION;
// This will trigger the assertion of duplicated elements found
// A bucket size of 23 since is where the collision between different hashes happens
set.rehash(23);
AZ_TEST_STOP_TRACE_SUPPRESSION(1); // 1 assertion
}
TEST_F(HashedContainers, HashTable_Fixed)
{
array<int, 5> elements = {
Gems/PhysX/Code/Source/Scene/PhysXSceneSimulationFilterCallback.cpp
+3 -1
View File
@@ -55,7 +55,9 @@ namespace PhysX
size_t SceneSimulationFilterCallback::CollisionPairHasher::operator()(const CollisionActorPair& collisionPair) const
{
size_t hash{ 0 };
AZStd::hash_combine(hash, collisionPair.m_actorA, collisionPair.m_actorB);
// Order elements so {1,2} and {2,1} would generate the same hash
auto [smallerVal, biggerVal] = AZStd::minmax(collisionPair.m_actorA, collisionPair.m_actorB);
AZStd::hash_combine(hash, smallerVal, biggerVal);
return hash;
}