Fix itt resource memory leak issue#34096
Fix itt resource memory leak issue#34096sgbihu wants to merge 1 commit intoopenvinotoolkit:releases/2026/0from
Conversation
Change the public to private and try to fix linux configure issue Try to fix CI build issue Try to fix the CI Cmake issue Try another way to fix the CI Try to add a new machnism to release the DLL resource Add lost file Fix CI build errors try to fix MacOS link issue Fix the compile issue Update the configure to control build static/shared library Try to only enable shutdown on dynamic library(May need delete) Refactor some code for itt fix Try to fix the build issue Add install target Try to fix the issue try to fix the build warning Try to fix the sample build cmake error Try to add install target Try to fix the NVplugin cmake issue Try to install object target Try to fix NVplugin build issue Refactor the code to avoid release resource at DLLMain, instead in the static object destruct Fix lost file Fix review inputs Fix review inputs Update year
| $<BUILD_INTERFACE:$<TARGET_PROPERTY:protobuf::libprotobuf,INTERFACE_INCLUDE_DIRECTORIES>>) | ||
| set_target_properties(${TARGET_NAME} PROPERTIES INTERPROCEDURAL_OPTIMIZATION_RELEASE ${ENABLE_LTO}) | ||
| target_compile_features(${TARGET_NAME} PRIVATE $<TARGET_PROPERTY:protobuf::libprotobuf,INTERFACE_COMPILE_FEATURES>) | ||
| target_link_libraries(${TARGET_NAME} PRIVATE openvino::shutdown) |
There was a problem hiding this comment.
why do we need to link additional library into frontends? Not sure that for fixing memory leaks we need to create the library?
There was a problem hiding this comment.
We want to unify the resource deallocation logic. In this way, all new modules with resource deallocation requirements can use this interface. I found this case is similar to the ITT case, so change it.
There was a problem hiding this comment.
In general, we need to talk about this deallocation logic. May be we need to have another approach.
We need to discuss it on arch meeting with @praasz.
There was a problem hiding this comment.
This is small library allow clean resources and is linked with ITT library (plugins use so no additional linking required).
Is used in FE to use same way to release resources, only small library is used for protobuf release instead some bigger lib (if there will be no dedicated shutdown lib)
@sgbihu
Could explain how changes has been tested
There was a problem hiding this comment.
You can find the duplicate steps in the https://jira.devtools.intel.com/browse/CVS-180657, I used the gflags/appverifier to monitor the memory leak issue. I can list the brief steps here:
- setup the gflags/appverifier.
- execute the onnxruntime_shared_lib_dlopen_test.exe with windbg tool (if the program break with memory leak info, it means program has memory leak issue. It's fine if it can exit)
There was a problem hiding this comment.
The old way attempts to call DLLMain/library_unload to release resources when the DLL is unloaded.
This change wants to use a static object's destructor to release the resource. The static object will be released when DLL exit. In this way, it gets those benefits:
- The shutdown_protobuf doesn't need to be an object target (I forgot to change this here, but this didn't impact the function, change the target from static to object is another memory leak fix in Fix the onnxfrontend didn't call DLLMain when exit #33641). This change can dismiss this platform dependency issue.
- Reuse the OV core's code and make all releases aligned.
- This also make the code more clean and reusable.
There was a problem hiding this comment.
why do we need to link additional library into frontends? Not sure that for fixing memory leaks we need to create the library?
Linkage to the new library allows to ensure a modular approach. We can put the code directly to the frontend/backends/core as alternative solution but it is a bad practice.
If this shutdown static library is linked to any other shared lib, nothing changes until you implement OV_REGISTER_SHUTDOWN_CALLBACK(...) somewhere inside the shared lib.
So from frontend library perspective nothing is changed if PROTOBUF_REQUIRED=OFF.
If PROTOBUF_REQUIRED=ON (onnx, paddle, tensorflow), only order of sequence when google::protobuf::ShutdownProtobufLibrary(); is called could be changed
|
Thank you for that advice, we are working on it for some time already. |
There was a problem hiding this comment.
Pull request overview
This PR introduces a centralized shutdown-callback registry to run cleanup code when OpenVINO shared libraries/modules are unloaded, addressing a reported ITT-related memory leak (and reworking protobuf shutdown to use the same mechanism).
Changes:
- Added a new
openvino::shutdownstatic library that providesov::register_shutdown_callbackand aOV_REGISTER_SHUTDOWN_CALLBACKmacro. - Replaced platform-specific protobuf shutdown (DllMain /
__attribute__((destructor))) with a shutdown callback registration. - Registered an ITT shutdown callback that calls
__itt_release_resources()on unload, and wiredopenvino::shutdowninto core/runtime, plugins, and frontends build targets as needed.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/frontends/common/shutdown_protobuf/shutdown_protobuf.cpp | Switch protobuf shutdown to the shutdown-callback mechanism. |
| src/frontends/common/shutdown_protobuf/CMakeLists.txt | Link protobuf shutdown object target with openvino::shutdown. |
| src/common/shutdown/shutdown.cpp | Implements shutdown callback registry and invokes callbacks on unload. |
| src/common/shutdown/include/openvino/shutdown.hpp | Declares shutdown API + registration macro. |
| src/common/shutdown/CMakeLists.txt | Adds/export/installs new openvino_shutdown static library. |
| src/common/itt/src/itt.cpp | Registers ITT resource-release callback on unload. |
| src/common/itt/include/openvino/itt.hpp | Adds internal::shutdown() declaration. |
| src/common/itt/CMakeLists.txt | Links openvino::itt with openvino::shutdown. |
| src/common/CMakeLists.txt | Adds shutdown subdirectory. |
| src/cmake/openvino.cmake | Links openvino::shutdown into openvino shared runtime build. |
| cmake/developer_package/plugins/plugins.cmake | Links plugins with openvino::shutdown when building shared libs. |
| cmake/developer_package/frontends/frontends.cmake | Links protobuf-required frontends with openvino::shutdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #define OV_REGISTER_SHUTDOWN_CALLBACK(func) \ | ||
| namespace { \ | ||
| static bool ov_shutdown_register_##func = ov::register_shutdown_callback(func); \ |
There was a problem hiding this comment.
OV_REGISTER_SHUTDOWN_CALLBACK expands to a namespace-scope static bool that is never referenced, which will trigger -Wunused-variable under -Wall (and can become a build break with warnings-as-errors). Also, using ov_shutdown_register_##func makes the macro fail for non-identifier arguments (e.g., &func, qualified names) and can cause redefinition errors if the macro is invoked twice with the same callback name in one TU. Consider generating a unique variable name (e.g., with __COUNTER__/__LINE__), and marking it [[maybe_unused]] (and/or const) so it is warning-free.
| #define OV_REGISTER_SHUTDOWN_CALLBACK(func) \ | |
| namespace { \ | |
| static bool ov_shutdown_register_##func = ov::register_shutdown_callback(func); \ | |
| #define OV_CONCAT_IMPL(a, b) a##b | |
| #define OV_CONCAT(a, b) OV_CONCAT_IMPL(a, b) | |
| #define OV_REGISTER_SHUTDOWN_CALLBACK(func) \ | |
| namespace { \ | |
| [[maybe_unused]] const bool \ | |
| OV_CONCAT(ov_shutdown_register_, __COUNTER__) = \ | |
| ov::register_shutdown_callback(func); \ |
There was a problem hiding this comment.
we referenced src\frontends\onnx\frontend\src\core\operator_set.hpp, #define ONNX_OP(name, range, ...) static bool onnx_op_reg = ONNX_OP_M(name, range, __VA_ARGS__)
| bool register_callback(const std::function<void()>& func) { | ||
| if (!func) { | ||
| return false; | ||
| } | ||
| m_callbacks.emplace_back(func); | ||
| return true; |
There was a problem hiding this comment.
ShutdownRegistry::register_callback mutates m_callbacks without any synchronization. Since register_shutdown_callback is a public function (and may be used from multiple threads), this introduces a data race and potential heap corruption. Please add thread-safety (e.g., guard m_callbacks with a mutex, and consider taking the std::function by value and moving it into the vector).
There was a problem hiding this comment.
This was called at dll load stage, didn't have thread issue.
| ~ShutdownRegistry() { | ||
| for (auto& func : m_callbacks) { | ||
| func(); | ||
| } | ||
| m_callbacks.clear(); | ||
| } |
There was a problem hiding this comment.
ShutdownRegistry's destructor calls registered callbacks directly. If any callback throws, the process will std::terminate() during DLL unload / static deinitialization. Also, iterating m_callbacks directly can misbehave if callbacks register additional callbacks (vector reallocation/invalidated references). A safer pattern is to move/swap the callback list into a local container under a lock, then invoke callbacks (often in reverse registration order) inside a noexcept destructor with exceptions swallowed/logged.
There was a problem hiding this comment.
One DLL only has one instance, so it doesn't need the mutex.
Details:
Need to call
__itt_release_resourceswhen unloadopenvino.dll.The solutions
Here is the solution: we create a class used to store all resource deallocation methods, then create a static object. The release method will register to the static object; this object will be released when the dll unload, all release functions will be called in the destructor. In this way, we didn't need to change any code in DLLMain/unload_library. Just use a MACRO to define the function pointer, like the code below.
Port #33887 to 2026.0
Tickets: