HashTable: Fixed hang when repeatedly adding/removing elements (#1358)

* Added rehash function and using it to recover deleted buckets
* Added move assign operator, bucket_count, max_bucket_count, max_size and max_load_factor
* Added Natvis support for HashTable
* Fixed an UBSAN warning in PerlinNoise3

Jolt/Core/HashTable.h

@@ -112,13 +112,41 @@ private:
 		size_type			mIndex;
 	};
 
+	/// Get the maximum number of elements that we can support given a number of buckets
+	static constexpr size_type sGetMaxLoad(size_type inBucketCount)
+	{
+		return uint32((cMaxLoadFactorNumerator * inBucketCount) / cMaxLoadFactorDenominator);
+	}
+
+	/// Update the control value for a bucket
+	JPH_INLINE void			SetControlValue(size_type inIndex, uint8 inValue)
+	{
+		JPH_ASSERT(inIndex < mMaxSize);
+		mControl[inIndex] = inValue;
+
+		// Mirror the first 15 bytes at the end of the control values
+		if (inIndex < 15)
+			mControl[mMaxSize + inIndex] = inValue;
+	}
+
+	/// Get the index and control value for a particular key
+	JPH_INLINE void			GetIndexAndControlValue(const Key &inKey, size_type &outIndex, uint8 &outControl) const
+	{
+		// Calculate hash
+		uint64 hash_value = Hash { } (inKey);
+
+		// Split hash into index and control value
+		outIndex = size_type(hash_value >> 7) & (mMaxSize - 1);
+		outControl = cBucketUsed | uint8(hash_value);
+	}
+
 	/// Allocate space for the hash table
 	void					AllocateTable(size_type inMaxSize)
 	{
 		JPH_ASSERT(mData == nullptr);
 
 		mMaxSize = inMaxSize;
-		mMaxLoad = uint32((cMaxLoadFactorNumerator * inMaxSize) / cMaxLoadFactorDenominator);
+		mLoadLeft = sGetMaxLoad(inMaxSize);
 		size_t required_size = size_t(mMaxSize) * (sizeof(KeyValue) + 1) + 15; // Add 15 bytes to mirror the first 15 bytes of the control values
 		if constexpr (cNeedsAlignedAllocate)
 			mData = reinterpret_cast<KeyValue *>(AlignedAllocate(required_size, alignof(KeyValue)));
@@ -165,7 +193,7 @@ private:
 		mControl = nullptr;
 		mSize = 0;
 		mMaxSize = 0;
-		mMaxLoad = 0;
+		mLoadLeft = 0;
 
 		// Allocate new table
 		AllocateTable(new_max_size);
@@ -181,7 +209,7 @@ private:
 				{
 					size_type index;
 					KeyValue *element = old_data + i;
-					JPH_IF_ENABLE_ASSERTS(bool inserted =) InsertKey</* AllowDeleted= */ false>(HashTableDetail::sGetKey(*element), index);
+					JPH_IF_ENABLE_ASSERTS(bool inserted =) InsertKey</* InsertAfterGrow= */ true>(HashTableDetail::sGetKey(*element), index);
 					JPH_ASSERT(inserted);
 					::new (mData + index) KeyValue(std::move(*element));
 					element->~KeyValue();
@@ -204,24 +232,28 @@ protected:
 
 	/// Insert a key into the map, returns true if the element was inserted, false if it already existed.
 	/// outIndex is the index at which the element should be constructed / where it is located.
-	template <bool AllowDeleted = true>
+	template <bool InsertAfterGrow = false>
 	bool					InsertKey(const Key &inKey, size_type &outIndex)
 	{
 		// Ensure we have enough space
-		if (mSize + 1 >= mMaxLoad)
-			GrowTable();
-
-		// Calculate hash
-		uint64 hash_value = Hash { } (inKey);
-
-		// Split hash into control byte and index
-		uint8 control = cBucketUsed | uint8(hash_value);
-		size_type bucket_mask = mMaxSize - 1;
-		size_type index = size_type(hash_value >> 7) & bucket_mask;
+		if (mLoadLeft == 0)
+		{
+			// Should not be growing if we're already growing!
+			if constexpr (InsertAfterGrow)
+				JPH_ASSERT(false);
+
+			// Decide if we need to clean up all tombstones or if we need to grow the map
+			size_type num_deleted = sGetMaxLoad(mMaxSize) - mSize;
+			if (num_deleted * cMaxDeletedElementsDenominator > mMaxSize * cMaxDeletedElementsNumerator)
+				rehash(0);
+			else
+				GrowTable();
+		}
 
-		BVec16 control16 = BVec16::sReplicate(control);
-		BVec16 bucket_empty = BVec16::sZero();
-		BVec16 bucket_deleted = BVec16::sReplicate(cBucketDeleted);
+		// Split hash into index and control value
+		size_type index;
+		uint8 control;
+		GetIndexAndControlValue(inKey, index, control);
 
 		// Keeps track of the index of the first deleted bucket we found
 		constexpr size_type cNoDeleted = ~size_type(0);
@@ -229,40 +261,33 @@ protected:
 
 		// Linear probing
 		KeyEqual equal;
+		size_type bucket_mask = mMaxSize - 1;
+		BVec16 control16 = BVec16::sReplicate(control);
+		BVec16 bucket_empty = BVec16::sZero();
+		BVec16 bucket_deleted = BVec16::sReplicate(cBucketDeleted);
 		for (;;)
 		{
 			// Read 16 control values (note that we added 15 bytes at the end of the control values that mirror the first 15 bytes)
 			BVec16 control_bytes = BVec16::sLoadByte16(mControl + index);
 
-			// Check for the control value we're looking for
-			uint32 control_equal = uint32(BVec16::sEquals(control_bytes, control16).GetTrues());
+			// Check if we must find the element before we can insert
+			if constexpr (!InsertAfterGrow)
+			{
+				// Check for the control value we're looking for
+				// Note that when deleting we can create empty buckets instead of deleted buckets.
+				// This means we must unconditionally check all buckets in this batch for equality
+				// (also beyond the first empty bucket).
+				uint32 control_equal = uint32(BVec16::sEquals(control_bytes, control16).GetTrues());
 
-			// Check for empty buckets
-			uint32 control_empty = uint32(BVec16::sEquals(control_bytes, bucket_empty).GetTrues());
+				// Index within the 16 buckets
+				size_type local_index = index;
 
-			// Check if we're still scanning for deleted buckets
-			if constexpr (AllowDeleted)
-				if (first_deleted_index == cNoDeleted)
+				// Loop while there's still buckets to process
+				while (control_equal != 0)
 				{
-					// Check if any buckets have been deleted, if so store the first one
-					uint32 control_deleted = uint32(BVec16::sEquals(control_bytes, bucket_deleted).GetTrues());
-					if (control_deleted != 0)
-						first_deleted_index = index + CountTrailingZeros(control_deleted);
-				}
-
-			// Index within the 16 buckets
-			size_type local_index = index;
+					// Get the first equal bucket
+					uint first_equal = CountTrailingZeros(control_equal);
 
-			// Loop while there's still buckets to process
-			while ((control_equal | control_empty) != 0)
-			{
-				// Get the index of the first bucket that is either equal or empty
-				uint first_equal = CountTrailingZeros(control_equal);
-				uint first_empty = CountTrailingZeros(control_empty);
-
-				// Check if we first found a bucket with equal control value before an empty bucket
-				if (first_equal < first_empty)
-				{
 					// Skip to the bucket
 					local_index += first_equal;
 
@@ -278,35 +303,47 @@ protected:
 					}
 
 					// Skip past this bucket
+					control_equal >>= first_equal + 1;
 					local_index++;
-					uint shift = first_equal + 1;
-					control_equal >>= shift;
-					control_empty >>= shift;
 				}
-				else
+
+				// Check if we're still scanning for deleted buckets
+				if (first_deleted_index == cNoDeleted)
 				{
-					// An empty bucket was found, we can insert a new item
-					JPH_ASSERT(control_empty != 0);
+					// Check if any buckets have been deleted, if so store the first one
+					uint32 control_deleted = uint32(BVec16::sEquals(control_bytes, bucket_deleted).GetTrues());
+					if (control_deleted != 0)
+						first_deleted_index = index + CountTrailingZeros(control_deleted);
+				}
+			}
 
-					// Get the location of the first empty or deleted bucket
-					local_index += first_empty;
-					if constexpr (AllowDeleted)
-						if (first_deleted_index < local_index)
-							local_index = first_deleted_index;
+			// Check for empty buckets
+			uint32 control_empty = uint32(BVec16::sEquals(control_bytes, bucket_empty).GetTrues());
+			if (control_empty != 0)
+			{
+				// If we found a deleted bucket, use it.
+				// It doesn't matter if it is before or after the first empty bucket we found
+				// since we will always be scanning in batches of 16 buckets.
+				if (first_deleted_index == cNoDeleted || InsertAfterGrow)
+				{
+					index += CountTrailingZeros(control_empty);
+					--mLoadLeft; // Using an empty bucket decreases the load left
+				}
+				else
+				{
+					index = first_deleted_index;
+				}
 
-					// Make sure that our index is not beyond the end of the table
-					local_index &= bucket_mask;
+				// Make sure that our index is not beyond the end of the table
+				index &= bucket_mask;
 
-					// Update control byte
-					mControl[local_index] = control;
-					if (local_index < 15)
-						mControl[mMaxSize + local_index] = control; // Mirror the first 15 bytes at the end of the control values
-					++mSize;
+				// Update control byte
+				SetControlValue(index, control);
+				++mSize;
 
-					// Return index to newly allocated bucket
-					outIndex = local_index;
-					return true;
-				}
+				// Return index to newly allocated bucket
+				outIndex = index;
+				return true;
 			}
 
 			// Move to next batch of 16 buckets
@@ -388,13 +425,13 @@ public:
 		mControl(ioRHS.mControl),
 		mSize(ioRHS.mSize),
 		mMaxSize(ioRHS.mMaxSize),
-		mMaxLoad(ioRHS.mMaxLoad)
+		mLoadLeft(ioRHS.mLoadLeft)
 	{
 		ioRHS.mData = nullptr;
 		ioRHS.mControl = nullptr;
 		ioRHS.mSize = 0;
 		ioRHS.mMaxSize = 0;
-		ioRHS.mMaxLoad = 0;
+		ioRHS.mLoadLeft = 0;
 	}
 
 	/// Assignment operator
@@ -410,6 +447,29 @@ public:
 		return *this;
 	}
 
+	/// Move assignment operator
+	HashTable &				operator = (HashTable &&ioRHS) noexcept
+	{
+		if (this != &ioRHS)
+		{
+			clear();
+
+			mData = ioRHS.mData;
+			mControl = ioRHS.mControl;
+			mSize = ioRHS.mSize;
+			mMaxSize = ioRHS.mMaxSize;
+			mLoadLeft = ioRHS.mLoadLeft;
+
+			ioRHS.mData = nullptr;
+			ioRHS.mControl = nullptr;
+			ioRHS.mSize = 0;
+			ioRHS.mMaxSize = 0;
+			ioRHS.mLoadLeft = 0;
+		}
+
+		return *this;
+	}
+
 	/// Destructor
 							~HashTable()
 	{
@@ -454,7 +514,7 @@ public:
 			mControl = nullptr;
 			mSize = 0;
 			mMaxSize = 0;
-			mMaxLoad = 0;
+			mLoadLeft = 0;
 		}
 	}
 
@@ -494,6 +554,18 @@ public:
 		return const_iterator(this, mMaxSize);
 	}
 
+	/// Number of buckets in the table
+	size_type				bucket_count() const
+	{
+		return mMaxSize;
+	}
+
+	/// Max number of buckets that the table can have
+	constexpr size_type		max_bucket_count() const
+	{
+		return size_type(1) << (sizeof(size_type) * 8 - 1);
+	}
+
 	/// Check if there are no elements in the table
 	bool					empty() const
 	{
@@ -506,6 +578,18 @@ public:
 		return mSize;
 	}
 
+	/// Max number of elements that the table can hold
+	constexpr size_type		max_size() const
+	{
+		return size_type((uint64(max_bucket_count()) * cMaxLoadFactorNumerator) / cMaxLoadFactorDenominator);
+	}
+
+	/// Get the max load factor for this table (max number of elements / number of buckets)
+	constexpr float			max_load_factor() const
+	{
+		return float(cMaxLoadFactorNumerator) / float(cMaxLoadFactorDenominator);
+	}
+
 	/// Insert a new element, returns iterator and if the element was inserted
 	std::pair<iterator, bool> insert(const value_type &inValue)
 	{
@@ -523,68 +607,61 @@ public:
 		if (empty())
 			return cend();
 
-		// Calculate hash
-		uint64 hash_value = Hash { } (inKey);
+		// Split hash into index and control value
+		size_type index;
+		uint8 control;
+		GetIndexAndControlValue(inKey, index, control);
 
-		// Split hash into control byte and index
-		uint8 control = cBucketUsed | uint8(hash_value);
+		// Linear probing
+		KeyEqual equal;
 		size_type bucket_mask = mMaxSize - 1;
-		size_type index = size_type(hash_value >> 7) & bucket_mask;
-
 		BVec16 control16 = BVec16::sReplicate(control);
 		BVec16 bucket_empty = BVec16::sZero();
-
-		// Linear probing
-		KeyEqual equal;
 		for (;;)
 		{
-			// Read 16 control values (note that we added 15 bytes at the end of the control values that mirror the first 15 bytes)
+			// Read 16 control values
+			// (note that we added 15 bytes at the end of the control values that mirror the first 15 bytes)
 			BVec16 control_bytes = BVec16::sLoadByte16(mControl + index);
 
 			// Check for the control value we're looking for
+			// Note that when deleting we can create empty buckets instead of deleted buckets.
+			// This means we must unconditionally check all buckets in this batch for equality
+			// (also beyond the first empty bucket).
 			uint32 control_equal = uint32(BVec16::sEquals(control_bytes, control16).GetTrues());
 
-			// Check for empty buckets
-			uint32 control_empty = uint32(BVec16::sEquals(control_bytes, bucket_empty).GetTrues());
-
 			// Index within the 16 buckets
 			size_type local_index = index;
 
 			// Loop while there's still buckets to process
-			while ((control_equal | control_empty) != 0)
+			while (control_equal != 0)
 			{
-				// Get the index of the first bucket that is either equal or empty
+				// Get the first equal bucket
 				uint first_equal = CountTrailingZeros(control_equal);
-				uint first_empty = CountTrailingZeros(control_empty);
-
-				// Check if we first found a bucket with equal control value before an empty bucket
-				if (first_equal < first_empty)
-				{
-					// Skip to the bucket
-					local_index += first_equal;
 
-					// Make sure that our index is not beyond the end of the table
-					local_index &= bucket_mask;
+				// Skip to the bucket
+				local_index += first_equal;
 
-					// We found a bucket with same control value
-					if (equal(HashTableDetail::sGetKey(mData[local_index]), inKey))
-					{
-						// Element found
-						return const_iterator(this, local_index);
-					}
+				// Make sure that our index is not beyond the end of the table
+				local_index &= bucket_mask;
 
-					// Skip past this bucket
-					local_index++;
-					uint shift = first_equal + 1;
-					control_equal >>= shift;
-					control_empty >>= shift;
-				}
-				else
+				// We found a bucket with same control value
+				if (equal(HashTableDetail::sGetKey(mData[local_index]), inKey))
 				{
-					// An empty bucket was found, we didn't find the element
-					JPH_ASSERT(control_empty != 0);
-					return cend();
+					// Element found
+					return const_iterator(this, local_index);
 				}
+
+				// Skip past this bucket
+				control_equal >>= first_equal + 1;
+				local_index++;
+			}
+
+			// Check for empty buckets
+			uint32 control_empty = uint32(BVec16::sEquals(control_bytes, bucket_empty).GetTrues());
+			if (control_empty != 0)
+			{
+				// An empty bucket was found, we didn't find the element
+				return cend();
 			}
 
 			// Move to next batch of 16 buckets
@@ -597,14 +674,30 @@ public:
 	{
 		JPH_ASSERT(inIterator.IsValid());
 
-		// Mark the bucket as deleted
-		mControl[inIterator.mIndex] = cBucketDeleted;
-		if (inIterator.mIndex < 15)
-			mControl[inIterator.mIndex + mMaxSize] = cBucketDeleted;
+		// Read 16 control values before and after the current index
+		// (note that we added 15 bytes at the end of the control values that mirror the first 15 bytes)
+		BVec16 control_bytes_before = BVec16::sLoadByte16(mControl + ((inIterator.mIndex - 16) & (mMaxSize - 1)));
+		BVec16 control_bytes_after = BVec16::sLoadByte16(mControl + inIterator.mIndex);
+		BVec16 bucket_empty = BVec16::sZero();
+		uint32 control_empty_before = uint32(BVec16::sEquals(control_bytes_before, bucket_empty).GetTrues());
+		uint32 control_empty_after = uint32(BVec16::sEquals(control_bytes_after, bucket_empty).GetTrues());
+
+		// If (this index including) there exist 16 consecutive non-empty slots (represented by a bit being 0) then
+		// a probe looking for some element needs to continue probing so we cannot mark the bucket as empty
+		// but must mark it as deleted instead.
+		// Note that we use: CountLeadingZeros(uint16) = CountLeadingZeros(uint32) - 16.
+		uint8 control_value = CountLeadingZeros(control_empty_before) - 16 + CountTrailingZeros(control_empty_after) < 16? cBucketEmpty : cBucketDeleted;
+
+		// Mark the bucket as empty/deleted
+		SetControlValue(inIterator.mIndex, control_value);
 
 		// Destruct the element
 		mData[inIterator.mIndex].~KeyValue();
 
+		// If we marked the bucket as empty we can increase the load left
+		if (control_value == cBucketEmpty)
+			++mLoadLeft;
+
 		// Decrease size
 		--mSize;
 	}
@@ -627,7 +720,97 @@ public:
 		std::swap(mControl, ioRHS.mControl);
 		std::swap(mSize, ioRHS.mSize);
 		std::swap(mMaxSize, ioRHS.mMaxSize);
-		std::swap(mMaxLoad, ioRHS.mMaxLoad);
+		std::swap(mLoadLeft, ioRHS.mLoadLeft);
+	}
+
+	/// In place re-hashing of all elements in the table. Removes all cBucketDeleted elements
+	/// The std version takes a bucket count, but we just re-hash to the same size.
+	void					rehash(size_type)
+	{
+		// Update the control value for all buckets
+		for (size_type i = 0; i < mMaxSize; ++i)
+		{
+			uint8 &control = mControl[i];
+			switch (control)
+			{
+			case cBucketDeleted:
+				// Deleted buckets become empty
+				control = cBucketEmpty;
+				break;
+			case cBucketEmpty:
+				// Remains empty
+				break;
+			default:
+				// Mark all occupied as deleted, to indicate it needs to move to the correct place
+				control = cBucketDeleted;
+				break;
+			}
+		}
+
+		// Replicate control values to the last 15 entries
+		for (size_type i = 0; i < 15; ++i)
+			mControl[mMaxSize + i] = mControl[i];
+
+		// Loop over all elements that have been 'deleted' and move them to their new spot
+		BVec16 bucket_used = BVec16::sReplicate(cBucketUsed);
+		size_type bucket_mask = mMaxSize - 1;
+		uint32 probe_mask = bucket_mask & ~uint32(0b1111); // Mask out lower 4 bits because we test 16 buckets at a time
+		for (size_type src = 0; src < mMaxSize; ++src)
+			if (mControl[src] == cBucketDeleted)
+				for (;;)
+				{
+					// Split hash into index and control value
+					size_type src_index;
+					uint8 src_control;
+					GetIndexAndControlValue(HashTableDetail::sGetKey(mData[src]), src_index, src_control);
+
+					// Linear probing
+					size_type dst = src_index;
+					for (;;)
+					{
+						// Check if any buckets are free
+						BVec16 control_bytes = BVec16::sLoadByte16(mControl + dst);
+						uint32 control_free = uint32(BVec16::sAnd(control_bytes, bucket_used).GetTrues()) ^ 0xffff;
+						if (control_free != 0)
+						{
+							// Select this bucket as destination
+							dst += CountTrailingZeros(control_free);
+							dst &= bucket_mask;
+							break;
+						}
+
+						// Move to next batch of 16 buckets
+						dst = (dst + 16) & bucket_mask;
+					}
+
+					// Check if we stay in the same probe group
+					if (((dst - src_index) & probe_mask) == ((src - src_index) & probe_mask))
+					{
+						// We stay in the same group, we can stay where we are
+						SetControlValue(src, src_control);
+						break;
+					}
+					else if (mControl[dst] == cBucketEmpty)
+					{
+						// There's an empty bucket, move us there
+						SetControlValue(dst, src_control);
+						SetControlValue(src, cBucketEmpty);
+						::new (mData + dst) KeyValue(std::move(mData[src]));
+						mData[src].~KeyValue();
+						break;
+					}
+					else
+					{
+						// There's an element in the bucket we want to move to, swap them
+						JPH_ASSERT(mControl[dst] == cBucketDeleted);
+						SetControlValue(dst, src_control);
+						std::swap(mData[src], mData[dst]);
+						// Iterate again with the same source bucket
+					}
+				}
+
+		// Reinitialize load left
+		mLoadLeft = sGetMaxLoad(mMaxSize) - mSize;
 	}
 
 private:
@@ -638,6 +821,10 @@ private:
 	static constexpr uint64	cMaxLoadFactorNumerator = 7;
 	static constexpr uint64	cMaxLoadFactorDenominator = 8;
 
+	/// If we can recover this fraction of deleted elements, we'll reshuffle the buckets in place rather than growing the table
+	static constexpr uint64 cMaxDeletedElementsNumerator = 1;
+	static constexpr uint64 cMaxDeletedElementsDenominator = 8;
+
 	/// Values that the control bytes can have
 	static constexpr uint8	cBucketEmpty = 0;
 	static constexpr uint8	cBucketDeleted = 0x7f;
@@ -655,8 +842,8 @@ private:
 	/// Max number of elements that can be stored in the table
 	size_type				mMaxSize = 0;
 
-	/// Max number of elements in the table before it should grow
-	size_type				mMaxLoad = 0;
+	/// Number of elements we can add to the table before we need to grow
+	size_type				mLoadLeft = 0;
 };
 
 JPH_NAMESPACE_END

Jolt/Jolt.natvis

@@ -24,6 +24,9 @@
 	<Type Name="JPH::UVec4">
 		<DisplayString>{mU32[0]}, {mU32[1]}, {mU32[2]}, {mU32[3]}</DisplayString>
 	</Type>
+	<Type Name="JPH::BVec16">
+		<DisplayString>{uint(mU8[0])}, {uint(mU8[1])}, {uint(mU8[2])}, {uint(mU8[3])}, {uint(mU8[4])}, {uint(mU8[5])}, {uint(mU8[6])}, {uint(mU8[7])}, {uint(mU8[8])}, {uint(mU8[9])}, {uint(mU8[10])}, {uint(mU8[11])}, {uint(mU8[12])}, {uint(mU8[13])}, {uint(mU8[14])}, {uint(mU8[15])}</DisplayString>
+	</Type>
 	<Type Name="JPH::Quat">
 		<DisplayString>{mValue}</DisplayString>
 	</Type>
@@ -94,6 +97,19 @@
 			</ArrayItems>
 		</Expand>
 	</Type>
+	<Type Name="JPH::HashTable&lt;*&gt;">
+		<DisplayString>size={mSize}</DisplayString>
+		<Expand>
+			<Item Name="[size]" ExcludeView="simple">mSize</Item>
+			<Item Name="[bucket_count]" ExcludeView="simple">mMaxSize</Item>
+			<IndexListItems Condition="mData != nullptr">
+				<Size>mMaxSize</Size>
+				<ValueNode Condition="mControl[$i] &amp; 0x80">mData[$i]</ValueNode>
+				<ValueNode Condition="mControl[$i] == 0">"--Empty--"</ValueNode>
+				<ValueNode Condition="mControl[$i] == 0x7f">"--Deleted--"</ValueNode>
+			</IndexListItems>
+		</Expand>
+	</Type>
 	<Type Name="JPH::StridedPtr&lt;*&gt;">
 		<DisplayString>{(value_type *)mPtr}, stride={mStride}</DisplayString>
 	</Type>

TestFramework/Math/Perlin.cpp

@@ -99,9 +99,9 @@ float PerlinNoise3(float x, float y, float z, int x_wrap, int y_wrap, int z_wrap
    float n00,n01,n10,n11;
    float n0,n1;
 
-   unsigned int x_mask = (x_wrap-1) & 255;
-   unsigned int y_mask = (y_wrap-1) & 255;
-   unsigned int z_mask = (z_wrap-1) & 255;
+   int x_mask = (x_wrap-1) & 255;
+   int y_mask = (y_wrap-1) & 255;
+   int z_mask = (z_wrap-1) & 255;
    int px = stb_perlin_fastfloor(x);
    int py = stb_perlin_fastfloor(y);
    int pz = stb_perlin_fastfloor(z);

UnitTests/Core/UnorderedSetTest.cpp

@@ -11,7 +11,13 @@ TEST_SUITE("UnorderedSetTest")
 	TEST_CASE("TestUnorderedSet")
 	{
 		UnorderedSet<int> set;
+		CHECK(set.bucket_count() == 0);
 		set.reserve(10);
+		CHECK(set.bucket_count() == 16);
+
+		// Check system limits
+		CHECK(set.max_bucket_count() == 0x80000000);
+		CHECK(set.max_size() == uint64(0x80000000) * 7 / 8);
 
 		// Insert some entries
 		CHECK(*set.insert(1).first == 1);
@@ -63,6 +69,16 @@ TEST_SUITE("UnorderedSetTest")
 		CHECK(*set4.find(3) == 3);
 		CHECK(set4.find(5) == set4.end());
 		CHECK(set3.empty());
+
+		// Move assign
+		UnorderedSet<int> set5;
+		set5.insert(999);
+		CHECK(*set5.find(999) == 999);
+		set5 = std::move(set4);
+		CHECK(set5.find(999) == set5.end());
+		CHECK(*set5.find(1) == 1);
+		CHECK(*set5.find(3) == 3);
+		CHECK(set4.empty());
 	}
 
 	TEST_CASE("TestUnorderedSetGrow")
@@ -152,4 +168,166 @@ TEST_SUITE("UnorderedSetTest")
 
 		CHECK(set.find(11) == set.cend());
 	}
+
+	TEST_CASE("TestUnorderedSetAddRemoveCyles")
+	{
+		UnorderedSet<int> set;
+		constexpr int cBucketCount = 64;
+		set.reserve(int(set.max_load_factor() * cBucketCount));
+		CHECK(set.bucket_count() == cBucketCount);
+
+		// Repeatedly add and remove elements to see if the set cleans up tombstones
+		constexpr int cNumElements = 64 * 6 / 8; // We make sure that the map is max 6/8 full to ensure that we never grow the map but rehash it instead
+		int add_counter = 0;
+		int remove_counter = 0;
+		for (int i = 0; i < 100; ++i)
+		{
+			for (int j = 0; j < cNumElements; ++j)
+			{
+				CHECK(set.find(add_counter) == set.end());
+				CHECK(set.insert(add_counter).second);
+				CHECK(set.find(add_counter) != set.end());
+				++add_counter;
+			}
+
+			CHECK(set.size() == cNumElements);
+
+			for (int j = 0; j < cNumElements; ++j)
+			{
+				CHECK(set.find(remove_counter) != set.end());
+				CHECK(set.erase(remove_counter) == 1);
+				CHECK(set.erase(remove_counter) == 0);
+				CHECK(set.find(remove_counter) == set.end());
+				++remove_counter;
+			}
+
+			CHECK(set.size() == 0);
+			CHECK(set.empty());
+		}
+
+		// Test that adding and removing didn't resize the set
+		CHECK(set.bucket_count() == cBucketCount);
+	}
+
+	TEST_CASE("TestUnorderedSetManyTombStones")
+	{
+		// A hash function that makes sure that consecutive ints end up in consecutive buckets starting at bucket 63
+		class MyBadHash
+		{
+		public:
+			size_t operator () (int inValue) const
+			{
+				return (inValue + 63) << 7;
+			}
+		};
+
+		UnorderedSet<int, MyBadHash> set;
+		constexpr int cBucketCount = 64;
+		set.reserve(int(set.max_load_factor() * cBucketCount));
+		CHECK(set.bucket_count() == cBucketCount);
+
+		// Fill 32 buckets
+		int add_counter = 0;
+		for (int i = 0; i < 32; ++i)
+			CHECK(set.insert(add_counter++).second);
+
+		// Since we control the hash, we know in which order we'll visit the elements
+		// The first element was inserted in bucket 63, so we start at 1
+		int expected = 1;
+		for (int i : set)
+		{
+			CHECK(i == expected);
+			expected = (expected + 1) & 31;
+		}
+		expected = 1;
+		for (int i : set)
+		{
+			CHECK(i == expected);
+			expected = (expected + 1) & 31;
+		}
+
+		// Remove a bucket in the middle with so that the number of occupied slots
+		// surrounding the bucket exceed 16 to force creating a tombstone,
+		// then add one at the end
+		int remove_counter = 16;
+		for (int i = 0; i < 100; ++i)
+		{
+			CHECK(set.find(remove_counter) != set.end());
+			CHECK(set.erase(remove_counter) == 1);
+			CHECK(set.find(remove_counter) == set.end());
+
+			CHECK(set.find(add_counter) == set.end());
+			CHECK(set.insert(add_counter).second);
+			CHECK(set.find(add_counter) != set.end());
+
+			++add_counter;
+			++remove_counter;
+		}
+
+		// Check that the elements we inserted are still there
+		CHECK(set.size() == 32);
+		for (int i = 0; i < 16; ++i)
+			CHECK(*set.find(i) == i);
+		for (int i = 0; i < 16; ++i)
+			CHECK(*set.find(add_counter - 1 - i) == add_counter - 1 - i);
+
+		// Test that adding and removing didn't resize the set
+		CHECK(set.bucket_count() == cBucketCount);
+	}
+
+	static bool sReversedHash = false;
+
+	TEST_CASE("TestUnorderedSetRehash")
+	{
+		// A hash function for which we can switch the hashing algorithm
+		class MyBadHash
+		{
+		public:
+			size_t operator () (int inValue) const
+			{
+				return (sReversedHash? 127 - inValue : inValue) << 7;
+			}
+		};
+
+		using Set = UnorderedSet<int, MyBadHash>;
+		Set set;
+		constexpr int cBucketCount = 128;
+		set.reserve(int(set.max_load_factor() * cBucketCount));
+		CHECK(set.bucket_count() == cBucketCount);
+
+		// Fill buckets
+		sReversedHash = false;
+		constexpr int cNumElements = 96;
+		for (int i = 0; i < cNumElements; ++i)
+			CHECK(set.insert(i).second);
+
+		// Check that we get the elements in the expected order
+		int expected = 0;
+		for (int i : set)
+			CHECK(i == expected++);
+
+		// Change the hashing algorithm so that a rehash is forced to move elements.
+		// The test is designed in such a way that it will both need to move elements to empty slots
+		// and to move elements to slots that currently already have another element.
+		sReversedHash = true;
+		set.rehash(0);
+
+		// Check that all elements are still there
+		for (int i = 0; i < cNumElements; ++i)
+			CHECK(*set.find(i) == i);
+
+		// The hash went from filling buckets 0 .. 95 with values 0 .. 95 to bucket 127 .. 31 with values 0 .. 95
+		// However, we don't move elements if they still fall within the same batch, this means that the first 8
+		// elements didn't move
+		Set::const_iterator it = set.begin();
+		for (int i = 0; i < 8; ++i, ++it)
+			CHECK(*it == i);
+
+		// The rest will have been reversed
+		for (int i = 95; i > 7; --i, ++it)
+			CHECK(*it == i);
+
+		// Test that adding and removing didn't resize the set
+		CHECK(set.bucket_count() == cBucketCount);
+	}
 }