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

Various Benchmark Fixes for AzCore (#7611)

Browse Source 
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>
This commit is contained in:
lumberyard-employee-dm
2022-02-14 11:08:53 -06:00
committed by GitHub
parent ceb61d143c
commit 6f52fcb9f0
6 changed files with 110 additions and 275 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Code/Framework/AzCore/AzCore/Memory/HphaSchema.cpp
+32 -225
View File
@@ -13,6 +13,7 @@
#include <AzCore/Memory/OSAllocator.h> // required by certain platforms
#include <AzCore/std/parallel/mutex.h>
#include <AzCore/std/parallel/lock.h>
#include <AzCore/std/containers/intrusive_list.h>
#include <AzCore/std/containers/intrusive_set.h>
#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 T>
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<T*>(node_base::next()); }
T* prev() const {return static_cast<T*>(node_base::prev()); }
const T& data() const {return *static_cast<const T*>(this); }
T& data() {return *static_cast<T*>(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<page>::node
, public AZStd::list_base_hook<page>::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<page>;
using page_list = AZStd::intrusive_list<page, AZStd::list_base_hook<page>>;
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<AZStd::mutex> 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<AZStd::mutex> 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<AZStd::recursive_mutex> lock(mTreeMutex);
#endif
return mFreeTree.maximum()->get_block()->size();
}
size_t HpAllocator::tree_get_unused_memory(bool isPrint) const
{
#ifdef MULTITHREADED
AZStd::lock_guard<AZStd::recursive_mutex> lock(mTreeMutex);
#endif
size_t unusedMemory = 0;
for (free_node_tree::const_iterator it = mFreeTree.begin(); it != mFreeTree.end(); ++it)
{
Code/Framework/AzCore/AzCore/UnitTest/TestTypes.h
+30
View File
@@ -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<AZ::SystemAllocator>::IsReady())
{
AZ::AllocatorInstance<AZ::SystemAllocator>::Create();
m_ownsAllocator = true;
}
}
~ScopedAllocatorBenchmarkEnvironment() override
{
if (m_ownsAllocator)
{
AZ::AllocatorInstance<AZ::SystemAllocator>::Destroy();
}
}
private:
bool m_ownsAllocator{};
};
#endif
class DLLTestVirtualClass
Code/Framework/AzCore/AzCore/std/containers/intrusive_set.h
+10 -16
View File
@@ -42,20 +42,13 @@ namespace AZStd
template<class U, class Hook, class Compare>
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))
Code/Framework/AzCore/Tests/Main.cpp
+2 -18
View File
@@ -7,29 +7,13 @@
*/
#include <AzCore/UnitTest/UnitTest.h>
#include <AzCore/UnitTest/TestTypes.h>
#include <AzTest/AzTest.h>
#include <AzCore/Memory/SystemAllocator.h>
#if defined(HAVE_BENCHMARK)
namespace AzCore
{
class AzCoreBenchmarkEnvironment
: public AZ::Test::BenchmarkEnvironmentBase
{
void SetUpBenchmark() override
{
AZ::AllocatorInstance<AZ::SystemAllocator>::Create();
}
void TearDownBenchmark() override
{
AZ::AllocatorInstance<AZ::SystemAllocator>::Destroy();
}
};
}
AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV, AzCore::AzCoreBenchmarkEnvironment)
AZ_UNIT_TEST_HOOK(DEFAULT_UNIT_TEST_ENV, UnitTest::ScopedAllocatorBenchmarkEnvironment)
#else
Code/Framework/AzCore/Tests/Memory/AllocatorBenchmarks.cpp
+8 -2
View File
@@ -291,7 +291,7 @@ namespace Benchmark
for (auto _ : state)
{
state.PauseTiming();
AZStd::vector<void*>& 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();
}
}
};
Code/Framework/AzTest/AzTest/AzTest.h
+28 -14
View File
@@ -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<BenchmarkEnvironmentBase>& envElement)
{
return envElement.get() == env;
};
m_envs.erase(std::remove_if(m_envs.begin(), m_envs.end(), std::move(RemoveBenchmarkFunc)));
}
std::vector<std::unique_ptr<BenchmarkEnvironmentBase>>& GetBenchmarkEnvironments()
{
return m_envs;
@@ -194,21 +204,26 @@ namespace AZ
return *benchmarkEnv;
}
template<typename... Ts>
std::array<BenchmarkEnvironmentBase*, sizeof...(Ts)> 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)
template<typename T>
ScopedRegisterBenchmarkEnvironment(T& benchmarkEnv)
: m_benchmarkEnv(benchmarkEnv)
{}
~ScopedRegisterBenchmarkEnvironment()
{
std::array<BenchmarkEnvironmentBase*, EnvironmentCount> benchmarkEnvs{ { &RegisterBenchmarkEnvironment<Ts>()... } };
return benchmarkEnvs;
if (auto benchmarkRegistry = AZ::Environment::FindVariable<BenchmarkEnvironmentRegistry>(s_benchmarkEnvironmentName);
benchmarkRegistry != nullptr)
{
benchmarkRegistry->RemoveBenchmarkEnvironment(&m_benchmarkEnv);
}
}
else
{
std::array<BenchmarkEnvironmentBase*, EnvironmentCount> benchmarkEnvs{};
return benchmarkEnvs;
}
}
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<TEST_ENV>(); \
AZ::Test::ScopedRegisterBenchmarkEnvironment scopedBenchmarkEnv(AZ::Test::RegisterBenchmarkEnvironment<TEST_ENV>()); \
auto benchmarkEnvRegistry = AZ::Environment::FindVariable<AZ::Test::BenchmarkEnvironmentRegistry>(AZ::Test::s_benchmarkEnvironmentName); \
std::vector<std::unique_ptr<AZ::Test::BenchmarkEnvironmentBase>>* 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::BenchmarkEnvironmentRegistry>(AZ::Test::s_benchmarkEnvironmentName); \
std::vector<std::unique_ptr<AZ::Test::BenchmarkEnvironmentBase>>* benchmarkEnvs = benchmarkEnvRegistry ? &(benchmarkEnvRegistry->GetBenchmarkEnvironments()) : nullptr; \
if (benchmarkEnvs != nullptr) \