From 6f52fcb9f09dc067f2bf2a3889aabccc6891cb46 Mon Sep 17 00:00:00 2001 From: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> Date: Mon, 14 Feb 2022 11:08:53 -0600 Subject: [PATCH] Various Benchmark Fixes for AzCore (#7611) Fixed crash in Allocators Benchmark multithreaded test due to the HPHA schema not having proper multithread protection around the `mFreeTree` member in `tree_get_unused_memory` function. The `mFreeTree intrusive set is able to modified on multiple threads. Replaced the custom intrusive_list implementation n HPHA schema with AZStd::intrusive_list Added a `ScopedAllocatorBenchmarkEnvironment` class to provide an RAI mechanism for initializing the SystemAllocator in Benchmark Test Rermoved the `AzCoreBenchmarkEnvironment` in lieu of the `ScopedAllocatorBenchmarkEnvironment` class Fixed assert when running Allocator Benchmarks in debug due to mismatch PauseTiming/ResumeTiming in Allocator Benchmark Fixtures Added `ScopedRegisterBenchmarkEnvironment` RAII class to provide lifetime guarantees on BenchmarkEnvironments registered via the `AZ_UNIT_TEST_HOOK` Initialized the intrusive_multiset_node members to nullptr in all build configurations instead of only debug as the cost negligible and it is useful for debugging. fixes LYN-10210 Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com> --- .../AzCore/AzCore/Memory/HphaSchema.cpp | 257 +++--------------- .../AzCore/AzCore/UnitTest/TestTypes.h | 30 ++ .../AzCore/std/containers/intrusive_set.h | 26 +- Code/Framework/AzCore/Tests/Main.cpp | 20 +- .../Tests/Memory/AllocatorBenchmarks.cpp | 10 +- Code/Framework/AzTest/AzTest/AzTest.h | 42 ++- 6 files changed, 110 insertions(+), 275 deletions(-) diff --git a/Code/Framework/AzCore/AzCore/Memory/HphaSchema.cpp b/Code/Framework/AzCore/AzCore/Memory/HphaSchema.cpp index 5e0e3e2de8..9af99a05b9 100644 --- a/Code/Framework/AzCore/AzCore/Memory/HphaSchema.cpp +++ b/Code/Framework/AzCore/AzCore/Memory/HphaSchema.cpp @@ -13,6 +13,7 @@ #include // required by certain platforms #include #include +#include #include #ifdef _DEBUG @@ -56,212 +57,6 @@ namespace AZ { // Enabled mutex per bucket #define USE_MUTEX_PER_BUCKET - ////////////////////////////////////////////////////////////////////////// - // TODO: Replace with AZStd::intrusive_list - class intrusive_list_base - { - public: - class node_base - { - node_base* mPrev; - node_base* mNext; - public: - node_base* next() const {return mNext; } - node_base* prev() const {return mPrev; } - void reset() - { - mPrev = this; - mNext = this; - } - void unlink() - { - mNext->mPrev = mPrev; - mPrev->mNext = mNext; - } - void link(node_base* node) - { - mPrev = node->mPrev; - mNext = node; - node->mPrev = this; - mPrev->mNext = this; - } - }; - intrusive_list_base() - { - mHead.reset(); - } - intrusive_list_base(const intrusive_list_base&) - { - mHead.reset(); - } - bool empty() const {return mHead.next() == &mHead; } - void swap(intrusive_list_base& other) - { - node_base* node = &other.mHead; - if (!empty()) - { - node = mHead.next(); - mHead.unlink(); - mHead.reset(); - } - node_base* other_node = &mHead; - if (!other.empty()) - { - other_node = other.mHead.next(); - other.mHead.unlink(); - other.mHead.reset(); - } - mHead.link(other_node); - other.mHead.link(node); - } - protected: - node_base mHead; - }; - - ////////////////////////////////////////////////////////////////////////// - // TODO: Replace with AZStd::intrusive_list - template - class intrusive_list - : public intrusive_list_base - { - intrusive_list(const intrusive_list& rhs); - intrusive_list& operator=(const intrusive_list& rhs); - public: - class node - : public node_base - { - public: - T* next() const {return static_cast(node_base::next()); } - T* prev() const {return static_cast(node_base::prev()); } - const T& data() const {return *static_cast(this); } - T& data() {return *static_cast(this); } - }; - - class const_iterator; - class iterator - { - using reference = T&; - using pointer = T*; - friend class const_iterator; - T* mPtr; - public: - iterator() - : mPtr(0) {} - explicit iterator(T* ptr) - : mPtr(ptr) {} - reference operator*() const {return mPtr->data(); } - pointer operator->() const {return &mPtr->data(); } - operator pointer() const { - return &mPtr->data(); - } - iterator& operator++() - { - mPtr = mPtr->next(); - return *this; - } - iterator& operator--() - { - mPtr = mPtr->prev(); - return *this; - } - bool operator==(const iterator& rhs) const {return mPtr == rhs.mPtr; } - bool operator!=(const iterator& rhs) const {return mPtr != rhs.mPtr; } - T* ptr() const {return mPtr; } - }; - - class const_iterator - { - using reference = const T &; - using pointer = const T *; - const T* mPtr; - public: - const_iterator() - : mPtr(0) {} - explicit const_iterator(const T* ptr) - : mPtr(ptr) {} - const_iterator(const iterator& it) - : mPtr(it.mPtr) {} - reference operator*() const {return mPtr->data(); } - pointer operator->() const {return &mPtr->data(); } - operator pointer() const { - return &mPtr->data(); - } - const_iterator& operator++() - { - mPtr = mPtr->next(); - return *this; - } - const_iterator& operator--() - { - mPtr = mPtr->prev(); - return *this; - } - bool operator==(const const_iterator& rhs) const {return mPtr == rhs.mPtr; } - bool operator!=(const const_iterator& rhs) const {return mPtr != rhs.mPtr; } - const T* ptr() const {return mPtr; } - }; - - intrusive_list() - : intrusive_list_base() {} - ~intrusive_list() {clear(); } - - const_iterator begin() const {return const_iterator((const T*)mHead.next()); } - iterator begin() {return iterator((T*)mHead.next()); } - const_iterator end() const {return const_iterator((const T*)&mHead); } - iterator end() {return iterator((T*)&mHead); } - - const T& front() const - { - HPPA_ASSERT(!empty()); - return *begin(); - } - T& front() - { - HPPA_ASSERT(!empty()); - return *begin(); - } - const T& back() const - { - HPPA_ASSERT(!empty()); - return *(--end()); - } - T& back() - { - HPPA_ASSERT(!empty()); - return *(--end()); - } - - void push_front(T* v) {insert(this->begin(), v); } - void pop_front() {erase(this->begin()); } - void push_back(T* v) {insert(this->end(), v); } - void pop_back() {erase(--(this->end())); } - - iterator insert(iterator where, T* node) - { - T* newLink = node; - newLink->link(where.ptr()); - return iterator(newLink); - } - iterator erase(iterator where) - { - T* node = where.ptr(); - ++where; - node->unlink(); - return where; - } - void erase(T* node) - { - node->unlink(); - } - void clear() - { - while (!this->empty()) - { - this->pop_back(); - } - } - }; - ////////////////////////////////////////////////////////////////////////// class HpAllocator { @@ -376,7 +171,7 @@ namespace AZ { }; struct page : public block_header_proxy /* must be first */ - , public intrusive_list::node + , public AZStd::list_base_hook::node_type { page(size_t elemSize, size_t pageSize, size_t marker) : mBucketIndex((unsigned short)bucket_spacing_function_aligned(elemSize)) @@ -415,7 +210,7 @@ namespace AZ { void dec_ref() { HPPA_ASSERT(mUseCount > 0); mUseCount--; } bool check_marker(size_t marker) const { return mMarker == (marker ^ ((size_t)this)); } }; - using page_list = intrusive_list; + using page_list = AZStd::intrusive_list>; class bucket { page_list mPageList; @@ -442,16 +237,17 @@ namespace AZ { #endif #endif size_t marker() const {return mMarker; } - const page* page_list_begin() const {return mPageList.begin(); } - page* page_list_begin() {return mPageList.begin(); } - const page* page_list_end() const {return mPageList.end(); } - page* page_list_end() {return mPageList.end(); } + auto page_list_begin() const {return mPageList.begin(); } + auto page_list_begin() {return mPageList.begin(); } + auto page_list_end() const {return mPageList.end(); } + auto page_list_end() {return mPageList.end(); } bool page_list_empty() const {return mPageList.empty(); } - void add_free_page(page* p) {mPageList.push_front(p); } + void add_free_page(page* p) {mPageList.push_front(*p); } page* get_free_page(); const page* get_free_page() const; void* alloc(page* p); void free(page* p, void* ptr); + void unlink(page* p); }; void* bucket_system_alloc(); void bucket_system_free(void* ptr); @@ -1278,8 +1074,8 @@ namespace AZ { if (!next) { // if full, auto sort to back - p->unlink(); - mPageList.push_back(p); + mPageList.erase(*p); + mPageList.push_back(*p); } return (void*)free; } @@ -1295,11 +1091,16 @@ namespace AZ { if (!free) { // if the page was previously full, auto sort to front - p->unlink(); - mPageList.push_front(p); + mPageList.erase(*p); + mPageList.push_front(*p); } } + void HpAllocator::bucket::unlink(page* p) + { + mPageList.erase(*p); + } + void* HpAllocator::bucket_system_alloc() { void* ptr; @@ -1522,8 +1323,8 @@ namespace AZ { AZStd::lock_guard lock(m_mutex); #endif #endif - const page* pageEnd = mBuckets[i].page_list_end(); - for (const page* p = mBuckets[i].page_list_begin(); p != pageEnd; ) + auto pageEnd = mBuckets[i].page_list_end(); + for (auto p = mBuckets[i].page_list_begin(); p != pageEnd; ) { // early out if we reach fully occupied page (the remaining should all be full) if (p->mFreeList == nullptr) @@ -1537,7 +1338,7 @@ namespace AZ { { AZ_TracePrintf("System", "Unused Bucket %d page %p elementSize: %d available: %d elements\n", i, p, elementSize, availableMemory / elementSize); } - p = p->next(); + p = p->m_next; } } return unusedMemory; @@ -1554,21 +1355,21 @@ namespace AZ { AZStd::lock_guard lock(m_mutex); #endif #endif - page* pageEnd = mBuckets[i].page_list_end(); - for (page* p = mBuckets[i].page_list_begin(); p != pageEnd; ) + auto pageEnd = mBuckets[i].page_list_end(); + for (auto p = mBuckets[i].page_list_begin(); p != pageEnd; ) { // early out if we reach fully occupied page (the remaining should all be full) if (p->mFreeList == nullptr) { break; } - page* next = p->next(); + page* next = p->m_next; if (p->empty()) { HPPA_ASSERT(p->mFreeList); - p->unlink(); + mBuckets[i].unlink(AZStd::to_address(p)); p->setInvalid(); - bucket_system_free(p); + bucket_system_free(AZStd::to_address(p)); } p = next; } @@ -2239,11 +2040,17 @@ namespace AZ { size_t HpAllocator::tree_get_max_allocation() const { +#ifdef MULTITHREADED + AZStd::lock_guard lock(mTreeMutex); +#endif return mFreeTree.maximum()->get_block()->size(); } size_t HpAllocator::tree_get_unused_memory(bool isPrint) const { +#ifdef MULTITHREADED + AZStd::lock_guard lock(mTreeMutex); +#endif size_t unusedMemory = 0; for (free_node_tree::const_iterator it = mFreeTree.begin(); it != mFreeTree.end(); ++it) { diff --git a/Code/Framework/AzCore/AzCore/UnitTest/TestTypes.h b/Code/Framework/AzCore/AzCore/UnitTest/TestTypes.h index db8aaadaa1..51838a9626 100644 --- a/Code/Framework/AzCore/AzCore/UnitTest/TestTypes.h +++ b/Code/Framework/AzCore/AzCore/UnitTest/TestTypes.h @@ -159,6 +159,36 @@ namespace UnitTest TeardownAllocator(); } }; + + /** + * RAII wrapper around a BenchmarkEnvironmentBase to encapsulate + * the creation and destuction of the SystemAllocator + * SetUpBenchmark and TearDownBenchmark can still be used for custom + * benchmark environment setup + */ + class ScopedAllocatorBenchmarkEnvironment + : public AZ::Test::BenchmarkEnvironmentBase + { + public: + ScopedAllocatorBenchmarkEnvironment() + { + // Only create the allocator if it doesn't exist + if (!AZ::AllocatorInstance::IsReady()) + { + AZ::AllocatorInstance::Create(); + m_ownsAllocator = true; + } + } + ~ScopedAllocatorBenchmarkEnvironment() override + { + if (m_ownsAllocator) + { + AZ::AllocatorInstance::Destroy(); + } + } + private: + bool m_ownsAllocator{}; + }; #endif class DLLTestVirtualClass diff --git a/Code/Framework/AzCore/AzCore/std/containers/intrusive_set.h b/Code/Framework/AzCore/AzCore/std/containers/intrusive_set.h index 3e6dae84d3..4a00d2a081 100644 --- a/Code/Framework/AzCore/AzCore/std/containers/intrusive_set.h +++ b/Code/Framework/AzCore/AzCore/std/containers/intrusive_set.h @@ -42,20 +42,13 @@ namespace AZStd template friend class intrusive_multiset; -#ifdef AZ_DEBUG_BUILD - intrusive_multiset_node() - { - m_children[0] = nullptr; - m_children[1] = nullptr; - m_neighbours[0] = nullptr; - m_neighbours[1] = nullptr; - m_parentColorSide = nullptr; - } + intrusive_multiset_node() = default; ~intrusive_multiset_node() { +#if defined(AZ_DEBUG_BUILD) AZSTD_CONTAINER_ASSERT(m_children[0] == nullptr && m_children[1] == nullptr && m_parentColorSide == nullptr, "AZStd::intrusive_multiset_node - intrusive list node is being destroyed while still in a container!"); - } #endif + } // We use the lower 2 bits of the parent pointer to store enum Bits { @@ -95,9 +88,9 @@ namespace AZStd AZ_FORCE_INLINE T* next() const { return m_neighbours[AZSTD_RBTREE_RIGHT]; } protected: - T* m_children[2]; - T* m_neighbours[2]; - T* m_parentColorSide; + T* m_children[2]{}; + T* m_neighbours[2]{}; + T* m_parentColorSide{}; } AZ_MAY_ALIAS; // we access the object in such way that we potentially can break strict aliasing rules /** @@ -659,12 +652,10 @@ namespace AZStd --m_numElements; -#ifdef AZ_DEBUG_BUILD nodeHook->m_children[0] = nodeHook->m_children[1] = nullptr; nodeHook->m_parentColorSide = nullptr; -#ifdef DEBUG_INTRUSIVE_MULTISET +#if defined(AZ_DEBUG_BUILD) && defined(DEBUG_INTRUSIVE_MULTISET) validate(); -#endif #endif } @@ -955,8 +946,11 @@ namespace AZStd hookNode->setParentSide(side); } + //! pre-condition + //! @param node must be non-nullptr inline static node_ptr_type MinOrMax(const_node_ptr_type node, SideType side) { + AZSTD_CONTAINER_ASSERT(node != nullptr, "MinOrMax can only be called with a non-nullptr node") const_node_ptr_type cur = node; const_node_ptr_type minMax = cur; while (!iterator_impl::isNil(cur)) diff --git a/Code/Framework/AzCore/Tests/Main.cpp b/Code/Framework/AzCore/Tests/Main.cpp index 3458f85546..ee23d4ceab 100644 --- a/Code/Framework/AzCore/Tests/Main.cpp +++ b/Code/Framework/AzCore/Tests/Main.cpp @@ -7,29 +7,13 @@ */ #include +#include #include -#include #if defined(HAVE_BENCHMARK) -namespace AzCore -{ - class AzCoreBenchmarkEnvironment - : public AZ::Test::BenchmarkEnvironmentBase - { - void SetUpBenchmark() override - { - AZ::AllocatorInstance::Create(); - } - void TearDownBenchmark() override - { - AZ::AllocatorInstance::Destroy(); - } - }; -} - -AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV, AzCore::AzCoreBenchmarkEnvironment) +AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV, UnitTest::ScopedAllocatorBenchmarkEnvironment) #else diff --git a/Code/Framework/AzCore/Tests/Memory/AllocatorBenchmarks.cpp b/Code/Framework/AzCore/Tests/Memory/AllocatorBenchmarks.cpp index e027b03a49..9ddd6ebf6b 100644 --- a/Code/Framework/AzCore/Tests/Memory/AllocatorBenchmarks.cpp +++ b/Code/Framework/AzCore/Tests/Memory/AllocatorBenchmarks.cpp @@ -291,7 +291,7 @@ namespace Benchmark for (auto _ : state) { state.PauseTiming(); - + AZStd::vector& perThreadAllocations = base::GetPerThreadAllocations(state.thread_index); const size_t numberOfAllocations = perThreadAllocations.size(); size_t totalAllocationSize = 0; @@ -300,7 +300,7 @@ namespace Benchmark const AllocationSizeArray& allocationArray = s_allocationSizes[TAllocationSize]; const size_t allocationSize = allocationArray[allocationIndex % allocationArray.size()]; totalAllocationSize += allocationSize; - + state.ResumeTiming(); perThreadAllocations[allocationIndex] = TestAllocatorType::Allocate(allocationSize, 0); state.PauseTiming(); @@ -319,6 +319,8 @@ namespace Benchmark TestAllocatorType::GarbageCollect(); state.SetItemsProcessed(numberOfAllocations); + + state.ResumeTiming(); } } }; @@ -364,6 +366,8 @@ namespace Benchmark state.SetItemsProcessed(numberOfAllocations); TestAllocatorType::GarbageCollect(); + + state.ResumeTiming(); } } }; @@ -519,6 +523,8 @@ namespace Benchmark state.SetItemsProcessed(itemsProcessed); TestAllocatorType::GarbageCollect(); + + state.ResumeTiming(); } } }; diff --git a/Code/Framework/AzTest/AzTest/AzTest.h b/Code/Framework/AzTest/AzTest/AzTest.h index 4b038cabd4..d40eb0eb75 100644 --- a/Code/Framework/AzTest/AzTest/AzTest.h +++ b/Code/Framework/AzTest/AzTest/AzTest.h @@ -164,6 +164,16 @@ namespace AZ m_envs.push_back(std::move(env)); } + // Remove a registered benchmark from the registry + void RemoveBenchmarkEnvironment(BenchmarkEnvironmentBase* env) + { + auto RemoveBenchmarkFunc = [env](const std::unique_ptr& envElement) + { + return envElement.get() == env; + }; + m_envs.erase(std::remove_if(m_envs.begin(), m_envs.end(), std::move(RemoveBenchmarkFunc))); + } + std::vector>& GetBenchmarkEnvironments() { return m_envs; @@ -194,21 +204,26 @@ namespace AZ return *benchmarkEnv; } - template - std::array RegisterBenchmarkEnvironments() + /* + * An RAII wrapper about registering a BenchmarkEnvironment with the BenchmarkRegistry + * It will unregister the BenchmarkEnvironment with the BenchmarkRegistry on destruction + */ + struct ScopedRegisterBenchmarkEnvironment { - constexpr size_t EnvironmentCount{ sizeof...(Ts) }; - if constexpr (EnvironmentCount) - { - std::array benchmarkEnvs{ { &RegisterBenchmarkEnvironment()... } }; - return benchmarkEnvs; - } - else + template + ScopedRegisterBenchmarkEnvironment(T& benchmarkEnv) + : m_benchmarkEnv(benchmarkEnv) + {} + ~ScopedRegisterBenchmarkEnvironment() { - std::array benchmarkEnvs{}; - return benchmarkEnvs; + if (auto benchmarkRegistry = AZ::Environment::FindVariable(s_benchmarkEnvironmentName); + benchmarkRegistry != nullptr) + { + benchmarkRegistry->RemoveBenchmarkEnvironment(&m_benchmarkEnv); + } } - } + BenchmarkEnvironmentBase& m_benchmarkEnv; + }; #endif //////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// //! listener class to capture and print test output for embedded platforms @@ -276,7 +291,7 @@ namespace AZ #define AZ_BENCHMARK_HOOK_ENV(TEST_ENV) \ AZTEST_EXPORT int AzRunBenchmarks(int argc, char** argv) \ { \ - AZ::Test::RegisterBenchmarkEnvironments(); \ + AZ::Test::ScopedRegisterBenchmarkEnvironment scopedBenchmarkEnv(AZ::Test::RegisterBenchmarkEnvironment()); \ auto benchmarkEnvRegistry = AZ::Environment::FindVariable(AZ::Test::s_benchmarkEnvironmentName); \ std::vector>* benchmarkEnvs = benchmarkEnvRegistry ? &(benchmarkEnvRegistry->GetBenchmarkEnvironments()) : nullptr; \ if (benchmarkEnvs != nullptr) \ @@ -307,7 +322,6 @@ AZTEST_EXPORT int AzRunBenchmarks(int argc, char** argv) \ #define AZ_BENCHMARK_HOOK() \ AZTEST_EXPORT int AzRunBenchmarks(int argc, char** argv) \ { \ - AZ::Test::RegisterBenchmarkEnvironments<>(); \ auto benchmarkEnvRegistry = AZ::Environment::FindVariable(AZ::Test::s_benchmarkEnvironmentName); \ std::vector>* benchmarkEnvs = benchmarkEnvRegistry ? &(benchmarkEnvRegistry->GetBenchmarkEnvironments()) : nullptr; \ if (benchmarkEnvs != nullptr) \