From d79f5b2835be71a767949635798dd027ac4533fa Mon Sep 17 00:00:00 2001 From: Piotr Doan Date: Sun, 5 Jan 2025 19:38:39 +0100 Subject: [PATCH] Make inline allocations behave as initially allocated - Fix unnecessary character destruction on shrinking Resize() and Clear() - Optimize reallocations from primary to secondary allocations by providing used capacity parameter - Proper move implementation for inline allocation - HeapString can now handle having empty allocation --- Engine/Common/Containers/Array.hpp | 13 +- Engine/Common/Containers/String.hpp | 118 ++++++++++-------- Engine/Memory/Allocators/DefaultAllocator.hpp | 19 ++- Engine/Memory/Allocators/InlineAllocator.hpp | 51 ++++---- Tests/Common/TestString.cpp | 2 + Tests/Memory/TestAllocators.cpp | 25 ++++ 6 files changed, 127 insertions(+), 101 deletions(-) diff --git a/Engine/Common/Containers/Array.hpp b/Engine/Common/Containers/Array.hpp index 587405d..a830a62 100644 --- a/Engine/Common/Containers/Array.hpp +++ b/Engine/Common/Containers/Array.hpp @@ -65,7 +65,7 @@ class Array final void ShrinkToFit() { - m_allocation.Resize(m_size); + m_allocation.Resize(m_size, m_size); } void Reserve(const u64 capacity, const bool exact = true) @@ -73,7 +73,7 @@ class Array final if(capacity > m_allocation.GetCapacity()) { const u64 newCapacity = exact ? capacity : CalculateCapacity(capacity); - m_allocation.Resize(newCapacity); + m_allocation.Resize(newCapacity, m_size); } } @@ -196,12 +196,6 @@ class Array final { ASSERT(newCapacity != 0); - // Determine if we should use initial capacity recommended by allocator. - if(newCapacity <= Allocation::GetInitialCapacity()) - { - return Allocation::GetInitialCapacity(); - } - // Find the next power of two capacity (unless already power of two), // but not smaller than some predefined minimum starting capacity. return std::max(4ull, NextPow2(newCapacity - 1ull)); @@ -215,4 +209,7 @@ static_assert(sizeof(Array) == 24); template using InlineArray = Array>; +template +using HeapArray = Array; + // #todo: Add static array with inline allocator and no backing allocator (or always asserting one). diff --git a/Engine/Common/Containers/String.hpp b/Engine/Common/Containers/String.hpp index f882a9f..b2e91ae 100644 --- a/Engine/Common/Containers/String.hpp +++ b/Engine/Common/Containers/String.hpp @@ -12,6 +12,7 @@ class StringViewBase; template class StringBase { + static_assert(std::is_trivial_v); using Allocation = typename Allocator::template TypedAllocation; Allocation m_allocation; u64 m_length = 0; @@ -20,11 +21,10 @@ class StringBase static constexpr u64 CharSize = sizeof(CharType); static constexpr u64 NullCount = 1; static constexpr CharType NullChar = '\0'; - static constexpr CharType EmptyString[NullCount] = { NullChar }; + static constexpr CharType* EmptyString = ""; StringBase() { - // #todo: Empty heap string will always allocate. Find a way to avoid this. ConstructFromText(EmptyString, 0); } @@ -101,12 +101,13 @@ class StringBase void ShrinkToFit() { - m_allocation.Resize(m_length + NullCount); + const u64 usedCapacity = m_length + NullCount; + m_allocation.Resize(m_length ? usedCapacity : 0, usedCapacity); } void Reserve(const u64 length, const bool exact = true) { - u64 newCapacity = length + NullCount; + u64 newCapacity = length ? length + NullCount : 0; if(newCapacity > m_allocation.GetCapacity()) { if(!exact) @@ -114,13 +115,14 @@ class StringBase newCapacity = CalculateCapacity(newCapacity); } - m_allocation.Resize(newCapacity); + const u64 usedCapacity = m_length + NullCount; + m_allocation.Resize(newCapacity, usedCapacity); } } void Resize(const u64 length, const CharType fillCharacter = ' ') { - if(length > m_length) // Grow length + if(length > m_length) { Reserve(length); Memory::ConstructRange( @@ -128,40 +130,45 @@ class StringBase m_allocation.GetPointer() + length, fillCharacter); } - else if(length < m_length) // Shrink length + + if(CharType* data = m_allocation.GetPointer()) { - Memory::DestructRange( - m_allocation.GetPointer() + length + NullCount, - m_allocation.GetPointer() + m_length + NullCount); + data[length] = NullChar; } - m_allocation.GetPointer()[length] = NullChar; m_length = length; } void Clear() { - if(m_length > 0) + if(CharType* data = m_allocation.GetPointer()) { - Memory::DestructRange( - m_allocation.GetPointer() + NullCount, - m_allocation.GetPointer() + m_length + NullCount); - - m_allocation.GetPointer()[0] = NullChar; - m_length = 0; + data[0] = NullChar; } + + m_length = 0; } CharType* GetData() { - ASSERT(m_allocation.GetPointer()); - return m_allocation.GetPointer(); + if(CharType* data = m_allocation.GetPointer()) + { + return data; + } + + ASSERT_SLOW(EmptyString[0] == NullChar); + return const_cast(EmptyString); } const CharType* GetData() const { - ASSERT(m_allocation.GetPointer()); - return m_allocation.GetPointer(); + if(const CharType* data = m_allocation.GetPointer()) + { + return data; + } + + ASSERT_SLOW(EmptyString[0] == NullChar); + return EmptyString; } u64 GetLength() const @@ -177,19 +184,17 @@ class StringBase bool IsEmpty() const { - return GetLength() == 0; + return m_length == 0; } CharType* operator*() { - ASSERT(m_allocation.GetPointer()); - return m_allocation.GetPointer(); + return GetData(); } const CharType* operator*() const { - ASSERT(m_allocation.GetPointer()); - return m_allocation.GetPointer(); + return GetData(); } CharType& operator[](u64 index) @@ -213,11 +218,13 @@ class StringBase StringBase result; result.Reserve(length); + if(CharType* resultData = result.m_allocation.GetPointer()) + { + std::memcpy(resultData, GetData(), m_length * sizeof(CharType)); + std::memcpy(resultData + m_length, other.GetData(), other.GetLength() * sizeof(CharType)); + resultData[length] = NullChar; + } - std::memcpy(result.GetData(), GetData(), m_length * sizeof(CharType)); - std::memcpy(result.GetData() + m_length, other.GetData(), other.GetLength() * sizeof(CharType)); - - result.GetData()[length] = NullChar; result.m_length = length; return result; } @@ -230,10 +237,13 @@ class StringBase StringBase result; result.Reserve(length); - std::memcpy(result.GetData(), GetData(), m_length * sizeof(CharType)); - std::memcpy(result.GetData() + m_length, other, otherLength * sizeof(CharType)); + if(CharType* resultData = result.m_allocation.GetPointer()) + { + std::memcpy(resultData, GetData(), m_length * sizeof(CharType)); + std::memcpy(resultData + m_length, other, otherLength * sizeof(CharType)); + resultData[length] = NullChar; + } - result.GetData()[length] = NullChar; result.m_length = length; return result; } @@ -245,9 +255,12 @@ class StringBase const u64 newLength = m_length + other.GetLength(); Reserve(newLength, false); - std::memcpy(GetData() + oldLength, other.GetData(), other.GetLength() * sizeof(CharType)); + if(CharType* data = m_allocation.GetPointer()) + { + std::memcpy(data + oldLength, other.GetData(), other.GetLength() * sizeof(CharType)); + data[newLength] = NullChar; + } - GetData()[newLength] = NullChar; m_length = newLength; } @@ -259,9 +272,12 @@ class StringBase const u64 newLength = m_length + otherLength; Reserve(newLength, false); - std::memcpy(GetData() + oldLength, other, otherLength * sizeof(CharType)); + if(CharType* data = m_allocation.GetPointer()) + { + std::memcpy(data + oldLength, other, otherLength * sizeof(CharType)); + data[newLength] = NullChar; + } - GetData()[newLength] = NullChar; m_length = newLength; } @@ -273,10 +289,13 @@ class StringBase StringBase result; result.Reserve(length); - std::snprintf(result.GetData(), result.GetCapacity() + NullCount, - format, std::forward(arguments)...); + if(CharType* resultData = result.m_allocation.GetPointer()) + { + std::snprintf(resultData, result.GetCapacity() + NullCount, + format, std::forward(arguments)...); + resultData[length] = NullChar; + } - result.GetData()[length] = NullChar; result.m_length = length; return result; } @@ -284,10 +303,13 @@ class StringBase private: void ConstructFromText(const CharType* text, const u64 length) { - ASSERT(text); Reserve(length); - std::memcpy(m_allocation.GetPointer(), text, length * sizeof(CharType)); - m_allocation.GetPointer()[length] = NullChar; + if(CharType* data = m_allocation.GetPointer()) + { + std::memcpy(data, text, length * sizeof(CharType)); + data[length] = NullChar; + } + m_length = length; } @@ -295,12 +317,6 @@ class StringBase { ASSERT(newCapacity != 0); - // Determine if we should use initial capacity recommended by allocator. - if(newCapacity <= Allocation::GetInitialCapacity()) - { - return Allocation::GetInitialCapacity(); - } - // Find the next power of two capacity (unless already power of two), // but not smaller than some predefined minimum starting capacity. return std::max(16ull, NextPow2(newCapacity - 1ull)); @@ -311,7 +327,7 @@ using DefaultStringAllocator = Memory::InlineAllocator<16>; using String = StringBase; static_assert(sizeof(String) == 32); -template +template using InlineString = StringBase>; static_assert(sizeof(InlineString<16>) == 32); diff --git a/Engine/Memory/Allocators/DefaultAllocator.hpp b/Engine/Memory/Allocators/DefaultAllocator.hpp index 21d645f..9656df5 100644 --- a/Engine/Memory/Allocators/DefaultAllocator.hpp +++ b/Engine/Memory/Allocators/DefaultAllocator.hpp @@ -64,13 +64,13 @@ namespace Memory m_capacity = capacity; } - void Reallocate(const u64 capacity) + void Reallocate(const u64 newCapacity, const u64 usedCapacity) { ASSERT(m_pointer != nullptr); ASSERT_SLOW(m_capacity != 0); - m_pointer = Memory::Reallocate(m_pointer, capacity, m_capacity); + m_pointer = Memory::Reallocate(m_pointer, newCapacity, m_capacity); ASSERT_SLOW(m_pointer != nullptr); - m_capacity = capacity; + m_capacity = newCapacity; } void Deallocate() @@ -82,22 +82,22 @@ namespace Memory m_capacity = 0; } - void Resize(const u64 capacity) + void Resize(const u64 newCapacity, const u64 usedCapacity) { if(m_capacity != 0) { - if(capacity == 0) + if(newCapacity == 0) { Deallocate(); } else { - Reallocate(capacity); + Reallocate(newCapacity, usedCapacity); } } else { - Allocate(capacity); + Allocate(newCapacity); } } @@ -110,11 +110,6 @@ namespace Memory { return m_capacity; } - - static u64 GetInitialCapacity() - { - return 0; - } }; }; } diff --git a/Engine/Memory/Allocators/InlineAllocator.hpp b/Engine/Memory/Allocators/InlineAllocator.hpp index 89c91b2..11ded59 100644 --- a/Engine/Memory/Allocators/InlineAllocator.hpp +++ b/Engine/Memory/Allocators/InlineAllocator.hpp @@ -57,7 +57,7 @@ namespace Memory }; using SecondaryAllocation = typename SecondaryAllocator::template TypedAllocation; - using StorageType = std::variant; + using StorageType = std::variant; StorageType m_storage; @@ -68,31 +68,30 @@ namespace Memory TypedAllocation(const TypedAllocation&) = delete; TypedAllocation& operator=(const TypedAllocation&) = delete; - TypedAllocation(TypedAllocation&& other) noexcept = default; - TypedAllocation& operator=(TypedAllocation&& other) noexcept = default; + TypedAllocation(TypedAllocation&& other) noexcept + { + *this = std::move(other); + } - void Allocate(u64 capacity) + TypedAllocation& operator=(TypedAllocation&& other) noexcept { - ASSERT(capacity > 0); - ASSERT(GetCapacity() == 0); + m_storage = std::move(other.m_storage); + other.m_storage.template emplace(); + return *this; + } - if(IsInlineCapacity(capacity)) - { - m_storage.template emplace(); - } - else - { - auto& secondary = m_storage.template emplace(); - secondary.Allocate(capacity); - } + void Allocate(const u64 capacity) + { + ASSERT(false, "Inline allocator is always in allocated state!"); } - void Reallocate(const u64 newCapacity) + void Reallocate(const u64 newCapacity, const u64 usedCapacity) { const u64 oldCapacity = GetCapacity(); ASSERT(newCapacity > 0); ASSERT(oldCapacity != 0); + ASSERT(usedCapacity <= oldCapacity); if(newCapacity == oldCapacity) return; @@ -108,13 +107,13 @@ namespace Memory { // Grown inline to secondary ObjectStorage elements[ElementCount]; - std::memcpy(elements, primary.elements, sizeof(ElementType) * ElementCount); + std::memcpy(elements, primary.elements, sizeof(ElementType) * usedCapacity); auto& secondary = m_storage.template emplace(); secondary.Allocate(newCapacity); ASSERT_SLOW(secondary.GetCapacity() >= ElementCount); - std::memcpy(secondary.GetPointer(), elements, sizeof(ElementType) * ElementCount); + std::memcpy(secondary.GetPointer(), elements, sizeof(ElementType) * usedCapacity); } } else @@ -131,24 +130,21 @@ namespace Memory } else { - secondary.Reallocate(newCapacity); + secondary.Reallocate(newCapacity, usedCapacity); } } } void Deallocate() { - ASSERT(!std::holds_alternative(m_storage)); - if(auto* secondary = std::get_if(&m_storage)) { secondary->Deallocate(); + m_storage.template emplace(); } - - m_storage = Empty{}; } - void Resize(const u64 newCapacity) + void Resize(const u64 newCapacity, const u64 usedCapacity) { if(GetCapacity() != 0) { @@ -158,7 +154,7 @@ namespace Memory } else { - Reallocate(newCapacity); + Reallocate(newCapacity, usedCapacity); } } else @@ -203,11 +199,6 @@ namespace Memory return 0; } - static u64 GetInitialCapacity() - { - return ElementCount; - } - private: static bool IsInlineCapacity(const u64 capacity) { diff --git a/Tests/Common/TestString.cpp b/Tests/Common/TestString.cpp index 27b4174..61a95d0 100644 --- a/Tests/Common/TestString.cpp +++ b/Tests/Common/TestString.cpp @@ -12,6 +12,7 @@ Test::Result Common::TestString() TEST_TRUE(memoryStats.ValidateAllocations(0, 0)); TEST_TRUE(string.GetLength() == 0); + TEST_TRUE(string.GetCapacity() == 15); TEST_TRUE(string.IsEmpty()); TEST_TRUE(string.GetData() != nullptr); TEST_TRUE(strcmp(string.GetData(), "") == 0); @@ -35,6 +36,7 @@ Test::Result Common::TestString() TEST_TRUE(memoryStats.ValidateAllocations(0, 0)); TEST_TRUE(string.GetLength() == 0); + TEST_TRUE(string.GetCapacity() == 15); TEST_TRUE(string.IsEmpty()); TEST_TRUE(string.GetData() != nullptr); TEST_TRUE(strcmp(string.GetData(), "") == 0); diff --git a/Tests/Memory/TestAllocators.cpp b/Tests/Memory/TestAllocators.cpp index 1778f10..f116f5a 100644 --- a/Tests/Memory/TestAllocators.cpp +++ b/Tests/Memory/TestAllocators.cpp @@ -37,5 +37,30 @@ Test::Result Memory::TestAllocators() TEST_TRUE(memoryStats.ValidateAllocations(1, sizeof(u32) * 4)); } + // Test initial container capacities + { + InlineString<16> string; + TEST_TRUE(memoryStats.ValidateAllocations(0, 0)); + TEST_TRUE(string.GetCapacity() == 15); + } + + { + HeapString string; + TEST_TRUE(memoryStats.ValidateAllocations(0, 0)); + TEST_TRUE(string.GetCapacity() == 0); + } + + { + InlineArray array; + TEST_TRUE(memoryStats.ValidateAllocations(0, 0)); + TEST_TRUE(array.GetCapacity() == 16); + } + + { + HeapArray array; + TEST_TRUE(memoryStats.ValidateAllocations(0, 0)); + TEST_TRUE(array.GetCapacity() == 0); + } + return Test::Result::Success; }