From: Inki Dae Date: Thu, 11 Jul 2024 02:25:36 +0000 (+0900) Subject: task_manager: fix memory leak issue X-Git-Tag: accepted/tizen/unified/20240903.110722~2 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=fbab3eab8c3e2f1f306652384a71a2db6462b570;p=platform%2Fcore%2Fapi%2Fsingleo.git task_manager: fix memory leak issue Fix memory leak issue to SharedBuffer objects when task manager is performed. The memory leak happended in two cases, 1. ref_count of SharedBuffer class wasn't discounted correctly because 'std::atomic ref_count' isn't initialized.[1] So clear it correctly. 2. missed to release input buffer. As for this, it moved the release call from each node class into threadCb of TaskManager class so that making sure to miss releasing the input buffer. With this, each node class is free to care about the buffer release. Regarding the memory leak, this patch introduces buffer tracker which manages to check if each buffer is released or not. In addition, use call by reference as a parameter of setOutputBuffer function. [1] https://saco-evaluator.org.za/docs/cppreference/en/cpp/atomic/atomic/atomic.html Change-Id: I3b20a7b9962e086a6d95bc04d996f527efc545eb Signed-off-by: Inki Dae --- diff --git a/common/CMakeLists.txt b/common/CMakeLists.txt new file mode 100644 index 0000000..b075544 --- /dev/null +++ b/common/CMakeLists.txt @@ -0,0 +1,3 @@ +SET(COMMON_DIRECTORY ${ROOT_DIRECTORY}/common) + +LIST(APPEND COMMON_HEADER_LIST ${COMMON_HEADER_LIST} ${COMMON_DIRECTORY}/include) \ No newline at end of file diff --git a/common/include/SharedBuffer.h b/common/include/SharedBuffer.h index aa6e9ac..2617541 100644 --- a/common/include/SharedBuffer.h +++ b/common/include/SharedBuffer.h @@ -18,21 +18,41 @@ #define __SHARED_BUFFER_H__ #include -#include +#include #include #include "SingleoCommonTypes.h" +#include "SingleoException.h" namespace singleo { +static std::map _bufferTracker; +static std::mutex _bufferTrackerMutex; + class SharedBuffer { private: std::vector > _data; - std::atomic ref_count; + std::atomic ref_count { 0 }; + static void pushSharedBuffer(SharedBuffer *sharedBuffer) + { + std::unique_lock lock(_bufferTrackerMutex); + + _bufferTracker[reinterpret_cast(sharedBuffer)] = true; + } + + static void popSharedBuffer(SharedBuffer *sharedBuffer) + { + std::unique_lock lock(_bufferTrackerMutex); + + _bufferTracker[reinterpret_cast(sharedBuffer)] = false; + } public: - SharedBuffer() = default; + SharedBuffer() + { + SharedBuffer::pushSharedBuffer(this); + } ~SharedBuffer() = default; void addInput(std::shared_ptr data) @@ -52,9 +72,27 @@ public: void release() { - if (ref_count.fetch_sub(1, std::memory_order_acq_rel) == 1) + if (ref_count.fetch_sub(1, std::memory_order_acq_rel) == 1) { for (auto &data : _data) data->release(); + + SharedBuffer::popSharedBuffer(this); + } + } + + static void checkMemoryLeak() + { + std::unique_lock lock(_bufferTrackerMutex); + + unsigned int bufferLeakCnt = 0; + + for (auto &bufferTracker : _bufferTracker) + if (bufferTracker.second == true) + bufferLeakCnt++; + + if (bufferLeakCnt > 0) + throw exception::InvalidOperation("SharedBuffer memory leak detected. " + std::to_string(bufferLeakCnt) + + " buffer(s) are not released."); } }; diff --git a/services/CMakeLists.txt b/services/CMakeLists.txt index 771fe92..6df9aa9 100644 --- a/services/CMakeLists.txt +++ b/services/CMakeLists.txt @@ -5,6 +5,7 @@ FILE(GLOB SINGLEO_SERVICE_SOURCE_FILES "${PROJECT_SOURCE_DIR}/*.cpp" "${PROJECT_SOURCE_DIR}/src/*.cpp" "${PROJECT_SOURCE_DIR}/common/src/*.cpp") +INCLUDE(${ROOT_DIRECTORY}/common/CMakeLists.txt) INCLUDE(${ROOT_DIRECTORY}/log/CMakeLists.txt) INCLUDE(${ROOT_DIRECTORY}/input/CMakeLists.txt) INCLUDE(${ROOT_DIRECTORY}/inference/CMakeLists.txt) @@ -17,7 +18,7 @@ ENDIF() ADD_LIBRARY(${PROJECT_NAME} SHARED ${SINGLEO_SERVICE_SOURCE_FILES}) -TARGET_INCLUDE_DIRECTORIES(${PROJECT_NAME} PRIVATE include common/include ${ROOT_DIRECTORY}/capi/ ${ROOT_DIRECTORY}/common/include ${INPUT_HEADER_LIST} ${INFERENCE_HEADER_LIST} ${SERVICE_HEADER_LIST} ${LOG_HEADER_LIST}) +TARGET_INCLUDE_DIRECTORIES(${PROJECT_NAME} PRIVATE include common/include ${ROOT_DIRECTORY}/capi/ ${COMMON_HEADER_LIST} ${INPUT_HEADER_LIST} ${INFERENCE_HEADER_LIST} ${SERVICE_HEADER_LIST} ${LOG_HEADER_LIST}) TARGET_LINK_LIBRARIES(${PROJECT_NAME} PRIVATE opencv_core opencv_imgcodecs opencv_highgui opencv_videoio ${INFERENCE_LIBRARY_LIST} ${SERVICE_LIBRARY_LIST} ${LOG_LIBRARY_LIST}) INSTALL(TARGETS ${PROJECT_NAME} DESTINATION ${LIB_INSTALL_DIR}) \ No newline at end of file diff --git a/services/task_manager/include/BranchNode.h b/services/task_manager/include/BranchNode.h index ba7dc0e..831adb2 100644 --- a/services/task_manager/include/BranchNode.h +++ b/services/task_manager/include/BranchNode.h @@ -64,8 +64,6 @@ public: n->setStatus(NodeStatus::INVALID); _cb(this); - - _inputBuffer->release(); } }; diff --git a/services/task_manager/include/BridgeNode.h b/services/task_manager/include/BridgeNode.h index cdc545e..f877f30 100644 --- a/services/task_manager/include/BridgeNode.h +++ b/services/task_manager/include/BridgeNode.h @@ -62,8 +62,6 @@ public: _cb(this); - _inputBuffer->release(); - _status = NodeStatus::INVALID; // Bridge node got the result from previous task node so enable this bridge node. if (_outputBuffer) diff --git a/services/task_manager/include/CallbackNode.h b/services/task_manager/include/CallbackNode.h index 079cc26..c4bac80 100644 --- a/services/task_manager/include/CallbackNode.h +++ b/services/task_manager/include/CallbackNode.h @@ -63,7 +63,7 @@ public: void addDependency(INode *node) override; std::vector &getDependencies() override; std::vector &getNexts() override; - void setOutputBuffer(std::shared_ptr outputBuffer) override; + void setOutputBuffer(std::shared_ptr &outputBuffer) override; std::shared_ptr &getOutputBuffer() override; void wait() override; void wakeup() override; diff --git a/services/task_manager/include/EndpointNode.h b/services/task_manager/include/EndpointNode.h index af13aff..956ae94 100644 --- a/services/task_manager/include/EndpointNode.h +++ b/services/task_manager/include/EndpointNode.h @@ -42,6 +42,7 @@ public: { _results.clear(); _status = NodeStatus::INVALID; + // If at least one dependency is valid, endpoint node is valid. for (auto &dep : this->getDependencies()) { if (dynamic_cast(dep)->getStatus() == NodeStatus::VALID) { @@ -49,6 +50,7 @@ public: break; } } + if (_status == NodeStatus::INVALID) return; @@ -57,8 +59,6 @@ public: if (_cb) _cb(this); - - _inputBuffer->release(); } }; diff --git a/services/task_manager/include/INode.h b/services/task_manager/include/INode.h index 46f9471..bdcc2a8 100644 --- a/services/task_manager/include/INode.h +++ b/services/task_manager/include/INode.h @@ -47,7 +47,7 @@ public: virtual void addNext(INode *node) = 0; virtual std::vector &getNexts() = 0; virtual std::vector &getDependencies() = 0; - virtual void setOutputBuffer(std::shared_ptr outputBuffer) = 0; + virtual void setOutputBuffer(std::shared_ptr &outputBuffer) = 0; virtual std::shared_ptr &getOutputBuffer() = 0; virtual void configure() = 0; virtual void invoke() = 0; diff --git a/services/task_manager/include/TaskNode.h b/services/task_manager/include/TaskNode.h index f50a4f8..8a915d8 100644 --- a/services/task_manager/include/TaskNode.h +++ b/services/task_manager/include/TaskNode.h @@ -61,7 +61,7 @@ public: void addDependency(INode *node) override; std::vector &getDependencies() override; std::vector &getNexts() override; - void setOutputBuffer(std::shared_ptr outputBuffer) override; + void setOutputBuffer(std::shared_ptr &outputBuffer) override; std::shared_ptr &getOutputBuffer() override; void wait() override; void wakeup() override; diff --git a/services/task_manager/src/CallbackNode.cpp b/services/task_manager/src/CallbackNode.cpp index 0c7814b..f1ce185 100644 --- a/services/task_manager/src/CallbackNode.cpp +++ b/services/task_manager/src/CallbackNode.cpp @@ -95,7 +95,7 @@ std::vector &CallbackNode::getNexts() return _nexts; } -void CallbackNode::setOutputBuffer(std::shared_ptr outputBuffer) +void CallbackNode::setOutputBuffer(std::shared_ptr &outputBuffer) { outputBuffer->addRef(); _outputBuffer = outputBuffer; diff --git a/services/task_manager/src/InferenceNode.cpp b/services/task_manager/src/InferenceNode.cpp index 31aae47..3a911b4 100644 --- a/services/task_manager/src/InferenceNode.cpp +++ b/services/task_manager/src/InferenceNode.cpp @@ -52,14 +52,12 @@ void InferenceNode::invoke() return; } } + auto &inputs = _inputBuffer->getInputs(); // TODO. consider for multiple inputs later. _task->invoke(*inputs[0]); - // Inference request has been completed so release input data. - _inputBuffer->release(); - _status = NodeStatus::INVALID; if (!_task->result()._is_empty) { _resultMutex.lock(); diff --git a/services/task_manager/src/TaskManager.cpp b/services/task_manager/src/TaskManager.cpp index cbf7f3b..3f5ccd5 100644 --- a/services/task_manager/src/TaskManager.cpp +++ b/services/task_manager/src/TaskManager.cpp @@ -110,6 +110,8 @@ void TaskManager::threadCb(INode *node) } node->invoke(); + if (node->getInputBuffer()) + node->getInputBuffer()->release(); node->wakeup(); // Spawn threads for next nodes @@ -338,6 +340,8 @@ void TaskManager::clear() _nodes.clear(); _results.clear(); _is_thread_created.clear(); + + SharedBuffer::checkMemoryLeak(); } } diff --git a/services/task_manager/src/TaskNode.cpp b/services/task_manager/src/TaskNode.cpp index db6342e..7d5ae1d 100644 --- a/services/task_manager/src/TaskNode.cpp +++ b/services/task_manager/src/TaskNode.cpp @@ -89,7 +89,7 @@ std::vector &TaskNode::getNexts() return _nexts; } -void TaskNode::setOutputBuffer(std::shared_ptr outputBuffer) +void TaskNode::setOutputBuffer(std::shared_ptr &outputBuffer) { outputBuffer->addRef(); _outputBuffer = outputBuffer;