From 9c7950cd220d94255cb2ddca3bb09140d382b7da Mon Sep 17 00:00:00 2001 From: Compute-Runtime-Validation Date: Fri, 7 Jul 2023 04:11:40 +0200 Subject: [PATCH] Revert "feature: add optional onChunkFree callback to AbstractBuffersPool" This reverts commit b7ecf99abb441fd022d799049fda5cf64ebe6bd6. Signed-off-by: Compute-Runtime-Validation --- opencl/source/context/context.cpp | 2 +- .../source/utilities/buffer_pool_allocator.h | 10 +-- .../utilities/buffer_pool_allocator.inl | 32 ++------ .../utilities/buffer_pool_allocator_tests.cpp | 74 +++---------------- 4 files changed, 19 insertions(+), 99 deletions(-) diff --git a/opencl/source/context/context.cpp b/opencl/source/context/context.cpp index 32123e84b4..1c2062c647 100644 --- a/opencl/source/context/context.cpp +++ b/opencl/source/context/context.cpp @@ -504,7 +504,7 @@ bool Context::BufferPoolAllocator::isAggregatedSmallBuffersEnabled(Context *cont (isSupportedForSingleDeviceContexts && context->isSingleDeviceContext()); } -Context::BufferPool::BufferPool(Context *context) : BaseType(context->memoryManager, nullptr) { +Context::BufferPool::BufferPool(Context *context) : BaseType(context->memoryManager) { static constexpr cl_mem_flags flags{}; [[maybe_unused]] cl_int errcodeRet{}; Buffer::AdditionalBufferCreateArgs bufferCreateArgs{}; diff --git a/shared/source/utilities/buffer_pool_allocator.h b/shared/source/utilities/buffer_pool_allocator.h index f9061031ce..4291cedfd5 100644 --- a/shared/source/utilities/buffer_pool_allocator.h +++ b/shared/source/utilities/buffer_pool_allocator.h @@ -7,7 +7,6 @@ #pragma once #include "shared/source/helpers/constants.h" -#include "shared/source/helpers/non_copyable_or_moveable.h" #include "shared/source/utilities/stackvec.h" #include @@ -32,7 +31,7 @@ struct SmallBuffersParams { }; template -struct AbstractBuffersPool : public SmallBuffersParams, public NonCopyableClass { +struct AbstractBuffersPool : public SmallBuffersParams { // The prototype of a function allocating the `mainStorage` is not specified. // That would be an unnecessary limitation here - it is completely up to derived class implementation. // Perhaps the allocating function needs to leverage `HeapAllocator::allocate()` and also @@ -44,9 +43,8 @@ struct AbstractBuffersPool : public SmallBuffersParams, public NonCopyabl using Params::smallBufferThreshold; using Params::startingOffset; using AllocsVecCRef = const StackVec &; - using OnChunkFreeCallback = void (PoolT::*)(uint64_t offset, size_t size); - AbstractBuffersPool(MemoryManager *memoryManager, OnChunkFreeCallback onChunkFreeCallback); + AbstractBuffersPool(MemoryManager *memoryManager); AbstractBuffersPool(AbstractBuffersPool &&bufferPool); void tryFreeFromPoolBuffer(BufferParentType *possiblePoolBuffer, size_t offset, size_t size); bool isPoolBuffer(const BufferParentType *buffer) const; @@ -62,7 +60,6 @@ struct AbstractBuffersPool : public SmallBuffersParams, public NonCopyabl std::unique_ptr mainStorage; std::unique_ptr chunkAllocator; std::vector> chunksToFree; - OnChunkFreeCallback onChunkFreeCallback = nullptr; }; template @@ -84,11 +81,8 @@ class AbstractBuffersAllocator : public SmallBuffersParams { protected: inline bool isSizeWithinThreshold(size_t size) const { return smallBufferThreshold >= size; } - void tryFreeFromPoolBuffer(BufferParentType *possiblePoolBuffer, size_t offset, size_t size, std::vector &bufferPoolsVec); void drain(); - void drain(std::vector &bufferPoolsVec); void addNewBufferPool(BuffersPoolType &&bufferPool); - void addNewBufferPool(BuffersPoolType &&bufferPool, std::vector &bufferPoolsVec); std::mutex mutex; std::vector bufferPools; diff --git a/shared/source/utilities/buffer_pool_allocator.inl b/shared/source/utilities/buffer_pool_allocator.inl index 3355e65f7c..bc19971530 100644 --- a/shared/source/utilities/buffer_pool_allocator.inl +++ b/shared/source/utilities/buffer_pool_allocator.inl @@ -14,8 +14,7 @@ namespace NEO { template -AbstractBuffersPool::AbstractBuffersPool(MemoryManager *memoryManager, OnChunkFreeCallback onChunkFreeCb) - : memoryManager{memoryManager}, onChunkFreeCallback{onChunkFreeCb} { +AbstractBuffersPool::AbstractBuffersPool(MemoryManager *mm) : memoryManager{mm} { static_assert(std::is_base_of_v); } @@ -23,13 +22,12 @@ template AbstractBuffersPool::AbstractBuffersPool(AbstractBuffersPool &&bufferPool) : memoryManager{bufferPool.memoryManager}, mainStorage{std::move(bufferPool.mainStorage)}, - chunkAllocator{std::move(bufferPool.chunkAllocator)}, - onChunkFreeCallback{bufferPool.onChunkFreeCallback} {} + chunkAllocator{std::move(bufferPool.chunkAllocator)} {} template void AbstractBuffersPool::tryFreeFromPoolBuffer(BufferParentType *possiblePoolBuffer, size_t offset, size_t size) { if (this->isPoolBuffer(possiblePoolBuffer)) { - this->chunksToFree.push_back({offset, size}); + this->chunksToFree.push_back({offset + startingOffset, size}); } } @@ -50,9 +48,6 @@ void AbstractBuffersPool::drain() { } for (auto &chunk : this->chunksToFree) { this->chunkAllocator->free(chunk.first, chunk.second); - if (static_cast(this)->onChunkFreeCallback) { - (static_cast(this)->*onChunkFreeCallback)(chunk.first, chunk.second); - } } this->chunksToFree.clear(); } @@ -71,38 +66,23 @@ bool AbstractBuffersAllocator::is template void AbstractBuffersAllocator::tryFreeFromPoolBuffer(BufferParentType *possiblePoolBuffer, size_t offset, size_t size) { - this->tryFreeFromPoolBuffer(possiblePoolBuffer, offset, size, this->bufferPools); -} - -template -void AbstractBuffersAllocator::tryFreeFromPoolBuffer(BufferParentType *possiblePoolBuffer, size_t offset, size_t size, std::vector &bufferPoolsVec) { auto lock = std::unique_lock(this->mutex); - for (auto &bufferPool : bufferPoolsVec) { + for (auto &bufferPool : this->bufferPools) { bufferPool.tryFreeFromPoolBuffer(possiblePoolBuffer, offset, size); // NOLINT(clang-analyzer-cplusplus.NewDelete) } } template void AbstractBuffersAllocator::drain() { - this->drain(this->bufferPools); -} - -template -void AbstractBuffersAllocator::drain(std::vector &bufferPoolsVec) { - for (auto &bufferPool : bufferPoolsVec) { + for (auto &bufferPool : this->bufferPools) { bufferPool.drain(); } } template void AbstractBuffersAllocator::addNewBufferPool(BuffersPoolType &&bufferPool) { - this->addNewBufferPool(std::move(bufferPool), this->bufferPools); -} - -template -void AbstractBuffersAllocator::addNewBufferPool(BuffersPoolType &&bufferPool, std::vector &bufferPoolsVec) { if (bufferPool.mainStorage) { - bufferPoolsVec.push_back(std::move(bufferPool)); + this->bufferPools.push_back(std::move(bufferPool)); } } } // namespace NEO \ No newline at end of file diff --git a/shared/test/unit_test/utilities/buffer_pool_allocator_tests.cpp b/shared/test/unit_test/utilities/buffer_pool_allocator_tests.cpp index c78d099c7d..4220d67216 100644 --- a/shared/test/unit_test/utilities/buffer_pool_allocator_tests.cpp +++ b/shared/test/unit_test/utilities/buffer_pool_allocator_tests.cpp @@ -34,38 +34,27 @@ struct DummyBuffersPool : public NEO::AbstractBuffersPool; static constexpr auto dummyPtr = 0xdeadbeef0000; - DummyBuffersPool(NEO::MemoryManager *memoryManager, uint32_t poolOffset, BaseType::OnChunkFreeCallback onChunkFreeCallback) - : BaseType{memoryManager, onChunkFreeCallback} { + DummyBuffersPool(NEO::MemoryManager *memoryManager, uint32_t poolOffset) : BaseType(memoryManager) { dummyAllocations.resize(2); dummyAllocations[0] = reinterpret_cast(poolOffset + dummyPtr); dummyAllocations[1] = nullptr; // makes sure nullptrs don't cause SEGFAULTs } - DummyBuffersPool(NEO::MemoryManager *memoryManager) : DummyBuffersPool(memoryManager, 0x0, &DummyBuffersPool::onChunkFree) {} + DummyBuffersPool(NEO::MemoryManager *memoryManager) : DummyBuffersPool(memoryManager, 0x0) {} BaseType::AllocsVecCRef getAllocationsVector() { return dummyAllocations; } - void onChunkFree(uint64_t offset, size_t size) { - this->freedChunks.push_back({offset, size}); - this->onChunkFreeCalled = true; - } StackVec dummyAllocations; - std::vector> freedChunks{}; - bool onChunkFreeCalled = false; }; struct DummyBuffersAllocator : public NEO::AbstractBuffersAllocator { using BaseType = NEO::AbstractBuffersAllocator; using BaseType::addNewBufferPool; using BaseType::bufferPools; + using BaseType::drain; using BaseType::isSizeWithinThreshold; - - void drainUnderLock() { - auto lock = std::unique_lock(this->mutex); - this->BaseType::drain(); - } }; using NEO::MockExecutionEnvironment; @@ -169,13 +158,13 @@ TEST_F(AbstractSmallBuffersTest, givenBuffersAllocatorWhenChunkOfMainStorageTrie auto &chunksToFree2 = buffersAllocator.bufferPools[1].chunksToFree; EXPECT_EQ(chunksToFree1.size(), 0u); EXPECT_EQ(chunksToFree2.size(), 0u); - auto chunkSize = DummyBuffersPool::chunkAlignment * 4; - auto chunkOffset = DummyBuffersPool::chunkAlignment; + auto chunkSize = sizeof(DummyBuffer) / 8; + auto chunkOffset = sizeof(DummyBuffer) / 2; buffersAllocator.tryFreeFromPoolBuffer(poolStorage2, chunkOffset, chunkSize); EXPECT_EQ(chunksToFree1.size(), 0u); EXPECT_EQ(chunksToFree2.size(), 1u); auto [effectiveChunkOffset, size] = chunksToFree2[0]; - EXPECT_EQ(effectiveChunkOffset, chunkOffset); + EXPECT_EQ(effectiveChunkOffset, chunkOffset + DummyBuffersPool::startingOffset); EXPECT_EQ(size, chunkSize); buffersAllocator.releaseSmallBufferPool(); @@ -204,8 +193,8 @@ TEST_F(AbstractSmallBuffersTest, givenBuffersAllocatorWhenDrainingPoolsThenOnlyA buffersAllocator.addNewBufferPool(std::move(pool1)); buffersAllocator.addNewBufferPool(std::move(pool2)); - auto chunkSize = DummyBuffersPool::chunkAlignment * 4; - auto chunkOffset = DummyBuffersPool::chunkAlignment; + auto chunkSize = sizeof(DummyBuffer) / 16; + auto chunkOffset = sizeof(DummyBuffer) / 2; for (size_t i = 0; i < 3; i++) { auto exampleOffset = chunkOffset + i * chunkSize * 2; buffersAllocator.tryFreeFromPoolBuffer(buffer1, exampleOffset, chunkSize); @@ -214,54 +203,11 @@ TEST_F(AbstractSmallBuffersTest, givenBuffersAllocatorWhenDrainingPoolsThenOnlyA auto &chunksToFree1 = buffersAllocator.bufferPools[0].chunksToFree; auto &chunksToFree2 = buffersAllocator.bufferPools[1].chunksToFree; - auto &freedChunks1 = buffersAllocator.bufferPools[0].freedChunks; - auto &freedChunks2 = buffersAllocator.bufferPools[1].freedChunks; EXPECT_EQ(chunksToFree1.size(), 3u); EXPECT_EQ(chunksToFree2.size(), 3u); - EXPECT_EQ(freedChunks1.size(), 0u); - EXPECT_EQ(freedChunks2.size(), 0u); otherMemoryManager->deferAllocInUse = true; - buffersAllocator.drainUnderLock(); + buffersAllocator.drain(); EXPECT_EQ(chunksToFree1.size(), 0u); EXPECT_EQ(chunksToFree2.size(), 3u); - ASSERT_EQ(freedChunks1.size(), 3u); - EXPECT_EQ(freedChunks2.size(), 0u); - EXPECT_TRUE(buffersAllocator.bufferPools[0].onChunkFreeCalled); - EXPECT_FALSE(buffersAllocator.bufferPools[1].onChunkFreeCalled); - for (size_t i = 0; i < 3; i++) { - auto expectedOffset = chunkOffset + i * chunkSize * 2; - auto [freedOffset, freedSize] = freedChunks1[i]; - EXPECT_EQ(expectedOffset, freedOffset); - EXPECT_EQ(chunkSize, freedSize); - } -} - -TEST_F(AbstractSmallBuffersTest, givenBuffersAllocatorWhenDrainingPoolsThenOnChunkFreeIgnoredIfNotDefined) { - auto pool1 = DummyBuffersPool{this->memoryManager.get(), 0x0, nullptr}; - pool1.mainStorage.reset(new DummyBuffer(testVal)); - auto buffer1 = pool1.mainStorage.get(); - pool1.chunkAllocator.reset(new NEO::HeapAllocator{DummyBuffersPool::startingOffset, - DummyBuffersPool::aggregatedSmallBuffersPoolSize, - DummyBuffersPool::chunkAlignment, - DummyBuffersPool::smallBufferThreshold}); - auto buffersAllocator = DummyBuffersAllocator{}; - buffersAllocator.addNewBufferPool(std::move(pool1)); - - auto chunkSize = DummyBuffersPool::chunkAlignment * 4; - auto chunkOffset = DummyBuffersPool::chunkAlignment; - for (size_t i = 0; i < 3; i++) { - auto exampleOffset = chunkOffset + i * chunkSize * 2; - buffersAllocator.tryFreeFromPoolBuffer(buffer1, exampleOffset, chunkSize); - } - - auto &chunksToFree1 = buffersAllocator.bufferPools[0].chunksToFree; - auto &freedChunks1 = buffersAllocator.bufferPools[0].freedChunks; - EXPECT_EQ(chunksToFree1.size(), 3u); - EXPECT_EQ(freedChunks1.size(), 0u); - - buffersAllocator.drainUnderLock(); - EXPECT_EQ(chunksToFree1.size(), 0u); - EXPECT_EQ(freedChunks1.size(), 0u); - EXPECT_FALSE(buffersAllocator.bufferPools[0].onChunkFreeCalled); -} +} \ No newline at end of file