Skip to content

Fix itt resource memory leak issue #34096

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sgbihu wants to merge 1 commit into from
Closed

Fix itt resource memory leak issue #34096

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmake/developer_package/frontends/frontends.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ macro(ov_add_frontend)

# Shutdown protobuf when unloading the frontend dynamic library
if(OV_FRONTEND_PROTOBUF_REQUIRED AND BUILD_SHARED_LIBS)
target_link_libraries(${TARGET_NAME} PRIVATE openvino::protobuf_shutdown)
target_link_libraries(${TARGET_NAME} PRIVATE openvino::protobuf_shutdown openvino::shutdown)
endif()

if(NOT BUILD_SHARED_LIBS)
Expand Down
4 changes: 3 additions & 1 deletion cmake/developer_package/plugins/plugins.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ function(ov_add_plugin)
endif()

target_compile_definitions(${OV_PLUGIN_NAME} PRIVATE IMPLEMENT_OPENVINO_RUNTIME_PLUGIN)
if(NOT BUILD_SHARED_LIBS)
if(BUILD_SHARED_LIBS)
target_link_libraries(${OV_PLUGIN_NAME} PRIVATE openvino::shutdown)
else()
# to distinguish functions creating plugin objects
target_compile_definitions(${OV_PLUGIN_NAME} PRIVATE
OV_CREATE_PLUGIN=create_plugin_engine_${OV_PLUGIN_DEVICE_NAME})
Expand Down
4 changes: 4 additions & 0 deletions src/cmake/openvino.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,10 @@ target_link_libraries(${TARGET_NAME}
PUBLIC $<$<AND:$<CXX_COMPILER_ID:GNU>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,9.1>>:stdc++fs>
$<$<AND:$<CXX_COMPILER_ID:Clang>,$<VERSION_LESS:$<CXX_COMPILER_VERSION>,9.0>>:c++fs>)

if(BUILD_SHARED_LIBS)
target_link_libraries(${TARGET_NAME} PRIVATE openvino::shutdown)
endif()

if (TBBBIND_2_5_FOUND)
target_link_libraries(${TARGET_NAME} PRIVATE ${TBBBIND_2_5_IMPORTED_TARGETS})
endif()
Expand Down
2 changes: 2 additions & 0 deletions src/common/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
# SPDX-License-Identifier: Apache-2.0
#

add_subdirectory(shutdown)

add_subdirectory(itt)
add_subdirectory(conditional_compilation)
add_subdirectory(util)
Expand Down
2 changes: 1 addition & 1 deletion src/common/itt/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ add_library(${TARGET_NAME} STATIC ${SOURCES})
add_library(openvino::itt ALIAS ${TARGET_NAME})
set_target_properties(${TARGET_NAME} PROPERTIES EXPORT_NAME itt)

target_link_libraries(${TARGET_NAME} PUBLIC openvino::util)
target_link_libraries(${TARGET_NAME} PUBLIC openvino::util openvino::shutdown)

if(NOT ENABLE_PROFILING_ITT STREQUAL "OFF")
if(TARGET ittapi::ittnotify)
Expand Down
1 change: 1 addition & 0 deletions src/common/itt/include/openvino/itt.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ void taskEnd(domain_t d);
void threadName(const char* name);
void regionBegin(domain_t d, handle_t t);
void regionEnd(domain_t d);
void shutdown();
} // namespace internal
/**
* @endcond
Expand Down
14 changes: 14 additions & 0 deletions src/common/itt/src/itt.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include <mutex>
#include <vector>

#include "openvino/shutdown.hpp"

#ifdef ENABLE_PROFILING_ITT
# include <ittnotify.h>
#endif
Expand Down Expand Up @@ -108,6 +110,10 @@ void regionEnd(domain_t d) {
current_region_handle = nullptr;
}

void shutdown() {
__itt_release_resources();
}

#else

domain_t domain(const char*) {
Expand All @@ -128,8 +134,16 @@ void regionBegin(domain_t, handle_t) {}

void regionEnd(domain_t) {}

void shutdown() {}

#endif // ENABLE_PROFILING_ITT

} // namespace internal
} // namespace itt
} // namespace openvino

static void shutdown_itt_resources() {
openvino::itt::internal::shutdown();
}

OV_REGISTER_SHUTDOWN_CALLBACK(shutdown_itt_resources)
18 changes: 18 additions & 0 deletions src/common/shutdown/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Copyright (C) 2018-2026 Intel Corporation
# SPDX-License-Identifier: Apache-2.0
#

set(TARGET_NAME openvino_shutdown)

add_library(${TARGET_NAME} STATIC shutdown.cpp)

add_library(openvino::shutdown ALIAS ${TARGET_NAME})
target_include_directories(${TARGET_NAME} PUBLIC
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>)
set_target_properties(${TARGET_NAME} PROPERTIES EXPORT_NAME shutdown)
ov_add_clang_format_target(${TARGET_NAME}_clang FOR_TARGETS ${TARGET_NAME})

# install & export
ov_install_static_lib(${TARGET_NAME} ${OV_CPACK_COMP_CORE})
ov_developer_package_export_targets(TARGET openvino::shutdown
INSTALL_INCLUDE_DIRECTORIES "${CMAKE_CURRENT_SOURCE_DIR}/include/")
16 changes: 16 additions & 0 deletions src/common/shutdown/include/openvino/shutdown.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
// Copyright (C) 2018-2026 Intel Corporation
// SPDX-License-Identifier: Apache-2.0
//
#pragma once
#include <functional>
#include <vector>

namespace ov {
// Registers a shutdown callback
bool register_shutdown_callback(const std::function<void()>& func);
}

#define OV_REGISTER_SHUTDOWN_CALLBACK(func) \
namespace { \
static bool ov_shutdown_register_##func = ov::register_shutdown_callback(func); \
Comment on lines +13 to +15
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
#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); \

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@sgbihu sgbihu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__)

}
42 changes: 42 additions & 0 deletions src/common/shutdown/shutdown.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright (C) 2018-2026 Intel Corporation
// SPDX-License-Identifier: Apache-2.0
//
#include "openvino/shutdown.hpp"

#include <functional>
#include <vector>

namespace ov {

class ShutdownRegistry {
private:
std::vector<std::function<void()>> m_callbacks;
ShutdownRegistry() = default;
public:

static ShutdownRegistry& get() {
static ShutdownRegistry instance;
return instance;
}

bool register_callback(const std::function<void()>& func) {
if (!func) {
return false;
}
m_callbacks.emplace_back(func);
return true;
Comment on lines +22 to +27
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@sgbihu sgbihu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was called at dll load stage, didn't have thread issue.

}

~ShutdownRegistry() {
for (auto& func : m_callbacks) {
func();
}
m_callbacks.clear();
}
Comment on lines +30 to +35
Copy link

Copilot AI Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@sgbihu sgbihu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One DLL only has one instance, so it doesn't need the mutex.

};

bool register_shutdown_callback(const std::function<void()>& func) {
return ShutdownRegistry::get().register_callback(func);
}

} // namespace ov
1 change: 1 addition & 0 deletions src/frontends/common/shutdown_protobuf/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,4 @@ target_include_directories(${TARGET_NAME} SYSTEM PRIVATE
$<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)
Copy link
Collaborator

@rkazants rkazants Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to link additional library into frontends? Not sure that for fixing memory leaks we need to create the library?

Copy link
Contributor Author

@sgbihu sgbihu Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@rkazants rkazants Feb 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@praasz praasz Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@sgbihu sgbihu Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@sgbihu sgbihu Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@olpipi olpipi Feb 13, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

26 changes: 4 additions & 22 deletions src/frontends/common/shutdown_protobuf/shutdown_protobuf.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,28 +4,10 @@

#include <google/protobuf/stubs/common.h>

#if defined(_WIN32) && !defined(__MINGW32__) && !defined(__MINGW64__)
# include <windows.h>
BOOL WINAPI DllMain(HINSTANCE hinstDLL, // handle to DLL module
DWORD fdwReason, // reason for calling function
LPVOID lpReserved) // reserved
{
// Perform actions based on the reason for calling.
switch (fdwReason) {
case DLL_PROCESS_ATTACH:
case DLL_THREAD_ATTACH:
case DLL_THREAD_DETACH:
break;
#include "openvino/shutdown.hpp"

case DLL_PROCESS_DETACH:
google::protobuf::ShutdownProtobufLibrary();
break;
}
return TRUE; // Successful DLL_PROCESS_ATTACH.
}
#elif defined(__linux__) || defined(__APPLE__)
extern "C" __attribute__((destructor)) void library_unload();
void library_unload() {
static void shutdown_frontend_resources() {
google::protobuf::ShutdownProtobufLibrary();
}
#endif

OV_REGISTER_SHUTDOWN_CALLBACK(shutdown_frontend_resources)
Loading