From 5be21da7c29c5fd4e9455f9123d696ef11604ca9 Mon Sep 17 00:00:00 2001 From: Alejandro Saucedo Date: Sun, 30 Aug 2020 14:15:09 +0100 Subject: [PATCH] Fixed #15 memory leak by introducing virtual function into all operation base classes to ensure the dependent class destructors are called --- CMakeLists.txt | 6 +- single_include/kompute/Kompute.hpp | 17 +++-- src/Algorithm.cpp | 63 ++++++++++++++++++- src/include/kompute/operations/OpAlgoBase.hpp | 9 ++- .../kompute/operations/OpAlgoLhsRhsOut.hpp | 2 +- src/include/kompute/operations/OpBase.hpp | 2 +- .../kompute/operations/OpCreateTensor.hpp | 2 +- src/include/kompute/operations/OpMult.hpp | 2 +- test/TestManager.cpp | 46 +++++++------- 9 files changed, 114 insertions(+), 35 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index de32931..dd45165 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -5,13 +5,15 @@ set(CMAKE_CXX_STANDARD 17) set(CMAKE_EXPORT_COMPILE_COMMANDS ON) -set(CMAKE_CXX_FLAGS_DEBUG "-DDEBUG=1") -set(CMAKE_CXX_FLAGS_RELEASE "-DRELEASE=1") +set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -DDEBUG=1") +set(CMAKE_CXX_FLAGS_RELEASE "{CMAKE_CXX_FLAGS_RELEASE} -DRELEASE=1") + set(CMAKE_VERBOSE_MAKEFILE on) option(KOMPUTE_OPT_BUILD_TESTS "Enable if you want to build tests" ON) option(KOMPUTE_OPT_BUILD_DOCS "Enable if you want to build documentation" ON) +option(KOMPUTE_OPT_DEBUG_SYMBOLS "Enable if you want to build debug with symbols" 0) # Allow scripts to call main kompute Makefile function(kompute_make KOMPUTE_MAKE_TARGET) diff --git a/single_include/kompute/Kompute.hpp b/single_include/kompute/Kompute.hpp index e9aa488..00e2565 100755 --- a/single_include/kompute/Kompute.hpp +++ b/single_include/kompute/Kompute.hpp @@ -392,7 +392,7 @@ class OpBase * intended to destroy the resources in the parent class. This can be done * by passing the mFreeTensors=false. */ - ~OpBase() + virtual ~OpBase() { SPDLOG_DEBUG("Kompute OpBase destructor started"); @@ -849,7 +849,7 @@ class OpAlgoBase : public OpBase * Default destructor, which is in charge of destroying the algorithm * components but does not destroy the underlying tensors */ - ~OpAlgoBase(); + virtual ~OpAlgoBase() override; /** * The init function is responsible for the initialisation of the algorithm @@ -1057,12 +1057,19 @@ std::vector OpAlgoBase::fetchSpirvBinaryData() std::ifstream fileStream(this->mOptSpirvBinPath, std::ios::binary | std::ios::in | std::ios::ate); + if (!fileStream.good()) { + throw std::runtime_error("Error reading file: " + this->mOptSpirvBinPath); + } + size_t shaderFileSize = fileStream.tellg(); fileStream.seekg(0, std::ios::beg); char* shaderDataRaw = new char[shaderFileSize]; fileStream.read(shaderDataRaw, shaderFileSize); fileStream.close(); + SPDLOG_WARN( + "Kompute OpAlgoBase fetched {} bytes", shaderFileSize); + return std::vector(shaderDataRaw, shaderDataRaw + shaderFileSize); } @@ -1112,7 +1119,7 @@ class OpAlgoLhsRhsOut : public OpAlgoBase * Default destructor, which is in charge of destroying the algorithm * components but does not destroy the underlying tensors */ - ~OpAlgoLhsRhsOut(); + virtual ~OpAlgoLhsRhsOut() override; /** * The init function is responsible for ensuring that all of the tensors @@ -1357,7 +1364,7 @@ class OpMult : public OpAlgoBase * Default destructor, which is in charge of destroying the algorithm * components but does not destroy the underlying tensors */ - ~OpMult() { + ~OpMult() override { SPDLOG_DEBUG("Kompute OpMult destructor started"); } @@ -1396,7 +1403,7 @@ class OpCreateTensor : public OpBase * Default destructor which in this case expects the parent class to free * the tensors */ - ~OpCreateTensor(); + ~OpCreateTensor() override; /** * In charge of initialising the primary Tensor as well as the staging diff --git a/src/Algorithm.cpp b/src/Algorithm.cpp index bce53c6..41d6729 100644 --- a/src/Algorithm.cpp +++ b/src/Algorithm.cpp @@ -27,6 +27,62 @@ Algorithm::~Algorithm() "Kompute Algorithm destructor reached with null Device pointer"); return; } + + if (this->mFreePipeline) { + SPDLOG_DEBUG("Kompute Algorithm Destroying pipeline"); + if (!this->mPipeline) { + SPDLOG_ERROR("Kompute Algorithm Error requested to destroy pipeline but it is null"); + } + this->mDevice->destroy(*this->mPipeline); + } + + if (this->mFreePipelineCache) { + SPDLOG_DEBUG("Kompute Algorithm Destroying pipeline cache"); + if (!this->mPipelineCache) { + SPDLOG_ERROR("Kompute Algorithm Error requested to destroy pipeline cache but it is null"); + } + this->mDevice->destroy(*this->mPipelineCache); + } + + if (this->mFreePipelineLayout) { + SPDLOG_DEBUG("Kompute Algorithm Destroying pipeline layout"); + if (!this->mPipelineLayout) { + SPDLOG_ERROR("Kompute Algorithm Error requested to destroy pipeline layout but it is null"); + } + this->mDevice->destroy(*this->mPipelineLayout); + } + + if (this->mFreeShaderModule) { + SPDLOG_DEBUG("Kompute Algorithm Destroying shader module"); + if (!this->mShaderModule) { + SPDLOG_ERROR("Kompute Algorithm Error requested to destroy shader module but it is null"); + } + this->mDevice->destroy(*this->mShaderModule); + } + + if (this->mFreeDescriptorSet) { + SPDLOG_DEBUG("Kompute Algorithm Freeing Descriptor Set"); + if (!this->mDescriptorSet) { + SPDLOG_ERROR("Kompute Algorithm Error requested to free descriptor set"); + } + this->mDevice->freeDescriptorSets(*this->mDescriptorPool, 1, this->mDescriptorSet.get()); + } + + if (this->mFreeDescriptorSetLayout) { + SPDLOG_DEBUG("Kompute Algorithm Destroying Descriptor Set Layout"); + if (!this->mDescriptorSetLayout) { + SPDLOG_ERROR("Kompute Algorithm Error requested to destroy descriptor set layout but it is null"); + } + this->mDevice->destroy(*this->mDescriptorSetLayout); + } + + if (this->mFreeDescriptorPool) { + SPDLOG_DEBUG("Kompute Algorithm Destroying Descriptor Pool"); + if (!this->mDescriptorPool) { + SPDLOG_ERROR("Kompute Algorithm Error requested to destroy descriptor pool but it is null"); + } + this->mDevice->destroy(*this->mDescriptorPool); + } } void @@ -99,6 +155,7 @@ Algorithm::createParameters(std::vector>& tensorParams) this->mDescriptorSetLayout = std::make_shared(); this->mDevice->createDescriptorSetLayout( &descriptorSetLayoutInfo, nullptr, this->mDescriptorSetLayout.get()); + this->mFreeDescriptorSetLayout = true; vk::DescriptorSetAllocateInfo descriptorSetAllocateInfo( *this->mDescriptorPool, @@ -109,6 +166,7 @@ Algorithm::createParameters(std::vector>& tensorParams) this->mDescriptorSet = std::make_shared(); this->mDevice->allocateDescriptorSets(&descriptorSetAllocateInfo, this->mDescriptorSet.get()); + this->mFreeDescriptorSet = true; // TODO: Explore design exposing the destination array element for (size_t i = 0; i < tensorParams.size(); i++) { @@ -153,6 +211,7 @@ Algorithm::createShaderModule(const std::vector& shaderFileData) this->mShaderModule = std::make_shared(); this->mDevice->createShaderModule( &shaderModuleInfo, nullptr, this->mShaderModule.get()); + this->mFreeShaderModule = true; SPDLOG_DEBUG("Kompute Algorithm create shader module success"); } @@ -171,6 +230,7 @@ Algorithm::createPipeline(std::vector specializationData) this->mPipelineLayout = std::make_shared(); this->mDevice->createPipelineLayout( &pipelineLayoutInfo, nullptr, this->mPipelineLayout.get()); + this->mFreePipelineLayout = true; std::vector specializationEntries; @@ -203,15 +263,16 @@ Algorithm::createPipeline(std::vector specializationData) 0); // TODO: Confirm what the best structure is with pipeline cache - this->mFreePipelineCache = true; vk::PipelineCacheCreateInfo pipelineCacheInfo = vk::PipelineCacheCreateInfo(); this->mPipelineCache = std::make_shared(); this->mDevice->createPipelineCache( &pipelineCacheInfo, nullptr, this->mPipelineCache.get()); + this->mFreePipelineCache = true; vk::ResultValue pipelineResult = this->mDevice->createComputePipeline(*this->mPipelineCache, pipelineInfo); + this->mFreePipeline = true; if (pipelineResult.result != vk::Result::eSuccess) { throw std::runtime_error("Failed to create pipeline result: " + diff --git a/src/include/kompute/operations/OpAlgoBase.hpp b/src/include/kompute/operations/OpAlgoBase.hpp index 72bb999..c63d053 100644 --- a/src/include/kompute/operations/OpAlgoBase.hpp +++ b/src/include/kompute/operations/OpAlgoBase.hpp @@ -61,7 +61,7 @@ class OpAlgoBase : public OpBase * Default destructor, which is in charge of destroying the algorithm * components but does not destroy the underlying tensors */ - ~OpAlgoBase(); + virtual ~OpAlgoBase() override; /** * The init function is responsible for the initialisation of the algorithm @@ -269,12 +269,19 @@ std::vector OpAlgoBase::fetchSpirvBinaryData() std::ifstream fileStream(this->mOptSpirvBinPath, std::ios::binary | std::ios::in | std::ios::ate); + if (!fileStream.good()) { + throw std::runtime_error("Error reading file: " + this->mOptSpirvBinPath); + } + size_t shaderFileSize = fileStream.tellg(); fileStream.seekg(0, std::ios::beg); char* shaderDataRaw = new char[shaderFileSize]; fileStream.read(shaderDataRaw, shaderFileSize); fileStream.close(); + SPDLOG_WARN( + "Kompute OpAlgoBase fetched {} bytes", shaderFileSize); + return std::vector(shaderDataRaw, shaderDataRaw + shaderFileSize); } diff --git a/src/include/kompute/operations/OpAlgoLhsRhsOut.hpp b/src/include/kompute/operations/OpAlgoLhsRhsOut.hpp index 2480ea6..ad89687 100644 --- a/src/include/kompute/operations/OpAlgoLhsRhsOut.hpp +++ b/src/include/kompute/operations/OpAlgoLhsRhsOut.hpp @@ -48,7 +48,7 @@ class OpAlgoLhsRhsOut : public OpAlgoBase * Default destructor, which is in charge of destroying the algorithm * components but does not destroy the underlying tensors */ - ~OpAlgoLhsRhsOut(); + virtual ~OpAlgoLhsRhsOut() override; /** * The init function is responsible for ensuring that all of the tensors diff --git a/src/include/kompute/operations/OpBase.hpp b/src/include/kompute/operations/OpBase.hpp index c8c1af4..1cadbf1 100644 --- a/src/include/kompute/operations/OpBase.hpp +++ b/src/include/kompute/operations/OpBase.hpp @@ -54,7 +54,7 @@ class OpBase * intended to destroy the resources in the parent class. This can be done * by passing the mFreeTensors=false. */ - ~OpBase() + virtual ~OpBase() { SPDLOG_DEBUG("Kompute OpBase destructor started"); diff --git a/src/include/kompute/operations/OpCreateTensor.hpp b/src/include/kompute/operations/OpCreateTensor.hpp index f08bef1..025bf28 100644 --- a/src/include/kompute/operations/OpCreateTensor.hpp +++ b/src/include/kompute/operations/OpCreateTensor.hpp @@ -37,7 +37,7 @@ class OpCreateTensor : public OpBase * Default destructor which in this case expects the parent class to free * the tensors */ - ~OpCreateTensor(); + ~OpCreateTensor() override; /** * In charge of initialising the primary Tensor as well as the staging diff --git a/src/include/kompute/operations/OpMult.hpp b/src/include/kompute/operations/OpMult.hpp index 45a63f5..35cef76 100644 --- a/src/include/kompute/operations/OpMult.hpp +++ b/src/include/kompute/operations/OpMult.hpp @@ -84,7 +84,7 @@ class OpMult : public OpAlgoBase * Default destructor, which is in charge of destroying the algorithm * components but does not destroy the underlying tensors */ - ~OpMult() { + ~OpMult() override { SPDLOG_DEBUG("Kompute OpMult destructor started"); } diff --git a/test/TestManager.cpp b/test/TestManager.cpp index 9674cc9..ff06342 100755 --- a/test/TestManager.cpp +++ b/test/TestManager.cpp @@ -9,35 +9,37 @@ TEST_CASE("End to end OpMult Flow should execute correctly from manager") { spdlog::info("TEST CASE STARTING"); spdlog::info("Creating manager"); - kp::Manager mgr; + { + kp::Manager mgr; - spdlog::info("Creating first tensor"); - std::shared_ptr tensorLHS{ new kp::Tensor( { 0, 1, 2 }) }; - mgr.evalOp({ tensorLHS }); + spdlog::info("Creating first tensor"); + std::shared_ptr tensorLHS{ new kp::Tensor({ 0, 1, 2 }) }; + mgr.evalOp({ tensorLHS }); - spdlog::info("Creating second tensor"); - std::shared_ptr tensorRHS{ new kp::Tensor( - { 2, 4, 6 }) }; - mgr.evalOp({ tensorRHS }); + spdlog::info("Creating second tensor"); + std::shared_ptr tensorRHS{ new kp::Tensor( + { 2, 4, 6 }) }; + mgr.evalOp({ tensorRHS }); - // TODO: Add capabilities for just output tensor types - spdlog::info("Creating output tensor"); - std::shared_ptr tensorOutput{ new kp::Tensor( - { 0, 0, 0 }) }; - mgr.evalOp({ tensorOutput }); + // TODO: Add capabilities for just output tensor types + spdlog::info("Creating output tensor"); + std::shared_ptr tensorOutput{ new kp::Tensor( + { 0, 0, 0 }) }; + mgr.evalOp({ tensorOutput }); - spdlog::info("OpCreateTensor success for tensors"); - spdlog::info("Tensor one: {}", tensorLHS->data()); - spdlog::info("Tensor two: {}", tensorRHS->data()); - spdlog::info("Tensor output: {}", tensorOutput->data()); + spdlog::info("OpCreateTensor success for tensors"); + spdlog::info("Tensor one: {}", tensorLHS->data()); + spdlog::info("Tensor two: {}", tensorRHS->data()); + spdlog::info("Tensor output: {}", tensorOutput->data()); - spdlog::info("Calling op mult"); - mgr.evalOp>({ tensorLHS, tensorRHS, tensorOutput }); + spdlog::info("Calling op mult"); + mgr.evalOp>({ tensorLHS, tensorRHS, tensorOutput }); - spdlog::info("OpMult call success"); - spdlog::info("Tensor output: {}", tensorOutput->data()); + spdlog::info("OpMult call success"); + spdlog::info("Tensor output: {}", tensorOutput->data()); - REQUIRE(tensorOutput->data() == std::vector{0, 4, 12}); + REQUIRE(tensorOutput->data() == std::vector{0, 4, 12}); + } spdlog::info("Called manager eval success END PROGRAM"); }