Skip to content
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.

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

Property to terminate tbb threads #11650

Merged
merged 22 commits into from
Jun 23, 2022
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
8692146
Property to force terminate tbb threads
riverlijunjie May 6, 2022
606d76a
Fix executorManager test case
riverlijunjie May 8, 2022
4de6dfc
Change frontendManger to be non-static instance
riverlijunjie May 10, 2022
0ad5b8d
Fix race condition between executor and executorManger
riverlijunjie May 15, 2022
0627bac
Merge remote-tracking branch 'final/master' into property_tbb_terminate
riverlijunjie May 15, 2022
1b85916
Add test case for tbb property
riverlijunjie May 17, 2022
cf7703b
Avoid terminate tbb in case of no tbb thread created
riverlijunjie May 18, 2022
0c59d6e
Fix Constant ops segmentfault issue
riverlijunjie May 19, 2022
10e386e
Make constant::m_data is released before frontendmanager
riverlijunjie May 21, 2022
32fbc1e
Merge branch 'master' into property_tbb_terminate
riverlijunjie May 25, 2022
1413484
Merge remote-tracking branch 'origin/master' into property_tbb_terminate
riverlijunjie May 26, 2022
cf6526a
Merge branch 'master' into property_tbb_terminate
riverlijunjie May 27, 2022
31e5178
tiny format change
riverlijunjie May 28, 2022
3e96cff
change tbb blocking_terminate to terminate
riverlijunjie Jun 16, 2022
452ca91
Merge branch 'master' into property_tbb_terminate
riverlijunjie Jun 16, 2022
47513f0
Merge branch 'master' into property_tbb_terminate
riverlijunjie Jun 16, 2022
15030b6
Remove unnecessary dependencies
riverlijunjie Jun 20, 2022
e20b5e5
Disable dynamic lib test case in static library compilation version
riverlijunjie Jun 21, 2022
0c128a2
Fix nested-namespace-definition issue
riverlijunjie Jun 21, 2022
9f04cc4
Address reviewer's comments
riverlijunjie Jun 21, 2022
78a511f
Merge branch 'master' into property_tbb_terminate
riverlijunjie Jun 22, 2022
9646bb1
Merge branch 'master' into property_tbb_terminate
riverlijunjie Jun 23, 2022
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
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ void regmodule_properties(py::module m) {
wrap_property_RW(m_properties, ov::inference_num_threads, "inference_num_threads");
wrap_property_RW(m_properties, ov::compilation_num_threads, "compilation_num_threads");
wrap_property_RW(m_properties, ov::affinity, "affinity");
wrap_property_RW(m_properties, ov::force_tbb_terminate, "force_tbb_terminate");

wrap_property_RO(m_properties, ov::supported_properties, "supported_properties");
wrap_property_RO(m_properties, ov::available_devices, "available_devices");
Expand Down
5 changes: 5 additions & 0 deletions src/cmake/ie_parallel.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -72,17 +72,21 @@ function(set_ie_threading_interface_for TARGET_NAME)

if(target_type STREQUAL "INTERFACE_LIBRARY")
set(LINK_TYPE "INTERFACE")
set(COMPILE_DEF_TYPE "INTERFACE")
elseif(target_type STREQUAL "EXECUTABLE" OR target_type STREQUAL "OBJECT_LIBRARY" OR
target_type STREQUAL "MODULE_LIBRARY")
set(LINK_TYPE "PRIVATE")
set(COMPILE_DEF_TYPE "PUBLIC")
elseif(target_type STREQUAL "STATIC_LIBRARY")
# Affected libraries: inference_engine_s, openvino_gapi_preproc_s
# they don't have TBB in public headers => PRIVATE
set(LINK_TYPE "PRIVATE")
set(COMPILE_DEF_TYPE "PUBLIC")
elseif(target_type STREQUAL "SHARED_LIBRARY")
# Affected libraries: inference_engine only
# TODO: why TBB propogates its headers to inference_engine?
set(LINK_TYPE "PRIVATE")
set(COMPILE_DEF_TYPE "PUBLIC")
else()
message(WARNING "Unknown target type")
endif()
Expand Down Expand Up @@ -113,6 +117,7 @@ function(set_ie_threading_interface_for TARGET_NAME)
if (TBB_FOUND)
set(IE_THREAD_DEFINE "IE_THREAD_TBB")
ie_target_link_libraries(${TARGET_NAME} ${LINK_TYPE} ${TBB_IMPORTED_TARGETS})
target_compile_definitions(${TARGET_NAME} ${COMPILE_DEF_TYPE} TBB_PREVIEW_WAITING_FOR_WORKERS=1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose we should duplicate this to ie_parallel.hpp since if the customer does not use cmake, such definition is lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, let me check how to solve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

just put definition to ie_parallel.hpp as well :)

else ()
set(THREADING "SEQ" PARENT_SCOPE)
message(WARNING "TBB was not found by the configured TBB_DIR path.\
Expand Down
2 changes: 1 addition & 1 deletion src/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ set_source_files_properties("${CMAKE_CURRENT_SOURCE_DIR}/src/pass/convert_precis

# Defines macro in C++ to load backend plugin
target_include_directories(ngraph_obj PUBLIC $<BUILD_INTERFACE:${OV_CORE_INCLUDE_PATH}>
PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src)
PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src)

add_library(ngraph INTERFACE)
target_link_libraries(ngraph INTERFACE openvino::runtime)
Expand Down
3 changes: 3 additions & 0 deletions src/core/include/openvino/core/model.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,9 @@ class OPENVINO_API Model : public std::enable_shared_from_this<Model> {
/// \brief Clones the original model
std::shared_ptr<ov::Model> clone() const;

/// \brief Reference to frontend maanager
std::shared_ptr<void> m_femgr;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to keep frontend manager instance is not released before ov::Model, due to frontend manager has been work as a dynamic instance rather than static instance.

Copy link
Contributor

@ilyachur ilyachur Jul 14, 2022

Choose a reason for hiding this comment

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

In general we don't need to have FR manager here. We need to keep only one frontend which created this particular model. And it already should be done. Here:

https://github.com/openvinotoolkit/openvino/blob/master/src/core/include/openvino/core/model.hpp#L46

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need to keep frontend manager here, ov::Model already keeps a reference to a particular frontend

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In general we don't need to have FR manager here. We need to keep only one frontend which created this particular model. And it already should be done. Here:

https://github.com/openvinotoolkit/openvino/blob/master/src/core/include/openvino/core/model.hpp#L46

It cannot work, we will encounter problem in some CI test, sometimes frontend manager will released before ov::Model, which cause segment fault.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't need to keep frontend manager here, ov::Model already keeps a reference to a particular frontend

I add such reference at ov::Model is to fix segment fault in CI test failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

In general we don't need to have FR manager here. We need to keep only one frontend which created this particular model. And it already should be done. Here:
https://github.com/openvinotoolkit/openvino/blob/master/src/core/include/openvino/core/model.hpp#L46

It cannot work, we will encounter problem in some CI test, sometimes frontend manager will released before ov::Model, which cause segment fault.

Can we investigate these issues in order to understand how frontend manager affects ov::Model? Frontend manager doesn't create ov::Model, it creates frontends and frontend creates ov::Model, it means that ov::Model depends on frontend but doesn't depend on FE manager.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we have this definition in public place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be move to protected or private place.


/// Model outputs
std::vector<ov::Output<ov::Node>> outputs();
ov::Output<ov::Node> output();
Expand Down
1 change: 1 addition & 0 deletions src/core/include/openvino/op/constant.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,7 @@ class OPENVINO_API Constant : public Op {
element::Type m_element_type;
Shape m_shape{};
std::shared_ptr<ngraph::runtime::AlignedBuffer> m_data;
std::shared_ptr<void> m_femgr;
Copy link
Contributor

Choose a reason for hiding this comment

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

the same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used to keep frontend manager instance is not released before ov::op::Constant which has shared memory.

Copy link
Contributor

Choose a reason for hiding this comment

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

ov::op::Constant should be hold by ov::Model I don't understand why we need to duplicate frontends here

Copy link
Contributor Author

@riverlijunjie riverlijunjie Jul 14, 2022

Choose a reason for hiding this comment

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

Maybe it is duplicated, because we first resolve the segment fault for ov::op::Constant caused by dynamic frontend manager, and then found ov::Model also exist such problem, so add reference to ov::Model. I will check/verify whether can remove reference in ov::op::Constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilya-lavrenov @ilyachur I have done some test to remove femgr reference from ov::op::constants, see test PR:#12166
Test result shows that we still need keep this reference in ov::op::constants, some test failed, such as below 2 cases:

  1. test_IENetwork.py
  2. test_NGraph.py
    You can reference to PR12166 CI result.

Anyhow I reproduce the same failure in local, such as test_NGraph.py

ov2022@ov2022-Z370-AORUS-Gaming-5:~/workspace/capi/openvino/src/bindings/python/tests_compatibility$ python3 -m pytest -s test_inference_engine/test_NGraph.py -v
========================================================================================= test session starts ==========================================================================================
platform linux -- Python 3.8.10, pytest-4.6.9, py-1.8.1, pluggy-0.13.0 -- /usr/bin/python3
cachedir: .pytest_cache
rootdir: /home/ov2022/workspace/capi/openvino/src/bindings/python
plugins: anyio-3.5.0
collected 6 items

test_inference_engine/test_NGraph.py::test_create_IENetwork_from_nGraph PASSED
test_inference_engine/test_NGraph.py::test_get_IENetwork_from_nGraph PASSED
test_inference_engine/test_NGraph.py::test_get_ops_from_IENetwork Segmentation fault (core dumped)

The failure test item is:

def test_get_ops_from_IENetwork():
ie = IECore()
net = ie.read_network(model=test_net_xml, weights=test_net_bin)
func = ng.function_from_cnn(net)
ops = func.get_ordered_ops()
ops_names = [op.friendly_name for op in ops]
assert len(ops_names) != 0
assert 'data' in ops_names

It's obvious that ov::Model has been released before ops, I dump the call trace as below:

test_inference_engine/test_NGraph.py ..

Thread 1 "python3" received signal SIGSEGV, Segmentation fault.
0x00007fff939b03ba in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x1266dc0) at /usr/include/c++/9/bits/shared_ptr_base.h:155
155 _M_dispose();
(gdb) bt
#0 0x00007fff939b03ba in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x1266dc0) at /usr/include/c++/9/bits/shared_ptr_base.h:155
#1 0x00007fff939a9b95 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::__shared_count (this=0x7fffffff7908, __in_chrg=) at /usr/include/c++/9/bits/shared_ptr_base.h:730
#2 0x00007fff92924f10 in std::__shared_ptr<ngraph::runtime::AlignedBuffer, (__gnu_cxx::_Lock_policy)2>::
__shared_ptr (this=0x7fffffff7900, __in_chrg=)
at /usr/include/c++/9/bits/shared_ptr_base.h:1169
#3 0x00007fff92940612 in std::__shared_ptr<ngraph::runtime::AlignedBuffer, (__gnu_cxx::_Lock_policy)2>::operator= (this=0x1267760, __r=...) at /usr/include/c++/9/bits/shared_ptr_base.h:1265
#4 0x00007fff929382fa in std::shared_ptrngraph::runtime::AlignedBuffer::operator= (this=0x1267760, __r=...) at /usr/include/c++/9/bits/shared_ptr.h:335
#5 0x00007fff9292f5e0 in ov::op::v0::Constant::Constant (this=0x1267570, __in_chrg=) at /home/ov2022/workspace/capi/openvino/src/core/src/op/constant.cpp:243
#6 0x00007fff923ba647 in __gnu_cxx::new_allocatorov::op::v0::Constant::destroyov::op::v0::Constant (this=0x1267570, __p=0x1267570) at /usr/include/c++/9/ext/new_allocator.h:152
#7 0x00007fff923b9c31 in std::allocator_traits<std::allocatorov::op::v0::Constant >::destroyov::op::v0::Constant (__a=..., __p=0x1267570) at /usr/include/c++/9/bits/alloc_traits.h:496
#8 0x00007fff923b916d in std::_Sp_counted_ptr_inplace<ov::op::v0::Constant, std::allocatorov::op::v0::Constant, (__gnu_cxx::_Lock_policy)2>::_M_dispose (this=0x1267560)
at /usr/include/c++/9/bits/shared_ptr_base.h:557
#9 0x00007fff939b03c6 in std::_Sp_counted_base<(__gnu_cxx::_Lock_policy)2>::_M_release (this=0x1267560) at /usr/include/c++/9/bits/shared_ptr_base.h:155
#10 0x00007fff939a9b95 in std::__shared_count<(__gnu_cxx::_Lock_policy)2>::
__shared_count (this=0x7fff90685880, __in_chrg=) at /usr/include/c++/9/bits/shared_ptr_base.h:730
#11 0x00007fff93a5563a in std::__shared_ptr<ov::op::v0::Constant, (__gnu_cxx::_Lock_policy)2>::~__shared_ptr (this=0x7fff90685878, __in_chrg=)
at /usr/include/c++/9/bits/shared_ptr_base.h:1169
#12 0x00007fff93a5565a in std::shared_ptrov::op::v0::Constant::~shared_ptr (this=0x7fff90685878, _in_chrg=) at /usr/include/c++/9/bits/shared_ptr.h:103
#13 0x00007fff93a5ca2c in pybind11::class
<ov::op::v0::Constant, std::shared_ptrov::op::v0::Constant, ov::Node>::dealloc (v_h=...)
at /home/ov2022/workspace/capi/openvino/src/bindings/python/thirdparty/pybind11/include/pybind11/pybind11.h:1612
#14 0x00007fff9398f7a1 in pybind11::detail::clear_instance (self=<_pyngraph.op.Constant at remote 0x7fff90685860>)
at /home/ov2022/workspace/capi/openvino/src/bindings/python/thirdparty/pybind11/include/pybind11/detail/class.h:399
#15 0x00007fff9398f8a3 in pybind11::detail::pybind11_object_dealloc (self=<_pyngraph.op.Constant at remote 0x7fff90685860>)
at /home/ov2022/workspace/capi/openvino/src/bindings/python/thirdparty/pybind11/include/pybind11/detail/class.h:419
#16 0x00000000005ee3fb in _Py_Dealloc (op=) at ../Objects/object.c:2215
#17 _Py_DECREF (filename=, lineno=541, op=) at ../Include/object.h:478
#18 _Py_XDECREF (op=) at ../Include/object.h:541
#19 list_dealloc (op=0x7fff9060eb40) at ../Objects/listobject.c:372
#20 0x00000000005ee8c0 in _Py_Dealloc (op=) at ../Objects/object.c:2215
#21 _Py_DECREF (filename=0x88c960 "../Objects/frameobject.c", lineno=430, op=) at ../Include/object.h:478

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is not an issue of constant op. You can have the same problem if frontend creates ov::Tensor. we need to fix the issue for common case, but it can require deeper refactoring of existed structures.

mutable std::atomic_bool m_all_elements_bitwise_identical{false};
mutable std::atomic_bool m_all_elements_bitwise_identical_checked{false};
bool m_alloc_buffer_on_visit_attributes = true;
Expand Down
8 changes: 8 additions & 0 deletions src/core/src/model.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,13 @@ BWDCMP_RTTI_DEFINITION(ov::AttributeAdapter<std::shared_ptr<ov::Model>>);

atomic<size_t> ov::Model::m_next_instance_id(0);

namespace ov {
namespace frontend {
class FrontEndManager;
std::shared_ptr<FrontEndManager> get_frontend_manager();
Copy link
Contributor

Choose a reason for hiding this comment

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

am I right that because of static frontend manager, we have issue with dynamically loaded OpenVINO.so:

  1. FE stores reference to OpenVINO.so
  2. OpenVINO stores references to frontend libraries
    And because of that, we have cyclic dependencies and libraries are not unloaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is the reason that why libopenvino_ir_frontend.so cannot be unloaded. But for libopenvino.so unload issue, there are more static variables and other dependencies/reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

But for libopenvino.so unload issue, there are more static variables and other dependencies/reference.

Could you please provide more examples of such issues where static variable holds another DLL?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are quit a lot such datas, not only static variable but also dependences data, I have ever setup a debug environment but not save the data, maybe I can re-setup and share the data to you later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ilya-lavrenov I sent some details info to you by email, please check it, thanks!

} // namespace frontend
} // namespace ov

namespace {

void check_all_variables_registered(const std::vector<shared_ptr<ov::Node>>& ordered_ops,
Expand Down Expand Up @@ -192,6 +199,7 @@ void ov::Model::prerequirements(bool detect_variables, bool detect_parameters) {
OV_ITT_SCOPED_TASK(ov::itt::domains::nGraph, "Model::prerequirements");

m_shared_rt_info = std::make_shared<SharedRTInfo>();
m_femgr = ov::frontend::get_frontend_manager();

const auto& ordered_ops = get_ordered_ops();
if (detect_parameters)
Expand Down
17 changes: 16 additions & 1 deletion src/core/src/op/constant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,13 @@

using namespace std;

namespace ov {
namespace frontend {
class FrontEndManager;
std::shared_ptr<FrontEndManager> get_frontend_manager();
} // namespace frontend
} // namespace ov

template <typename T>
static inline string to_cpp_string(T value) {
string rc;
Expand Down Expand Up @@ -50,6 +57,7 @@ ov::op::v0::Constant::Constant(const shared_ptr<ngraph::runtime::Tensor>& tensor
tensor->read(get_data_ptr_nc(), tensor->get_size_in_bytes());
}
constructor_validate_and_infer_types();
m_femgr = ov::frontend::get_frontend_manager();
}

ov::op::v0::Constant::Constant(const element::Type& type,
Expand Down Expand Up @@ -193,6 +201,7 @@ ov::op::v0::Constant::Constant(bool memset_allocation, const element::Type& type
m_shape(shape) {
allocate_buffer(memset_allocation);
constructor_validate_and_infer_types();
m_femgr = ov::frontend::get_frontend_manager();
}

void ov::op::v0::Constant::allocate_buffer(bool memset_allocation) {
Expand All @@ -214,6 +223,7 @@ ov::op::v0::Constant::Constant(const Constant& other) {
m_data = other.m_data;
update_identical_flags(other.m_all_elements_bitwise_identical_checked, other.m_all_elements_bitwise_identical);
constructor_validate_and_infer_types();
m_femgr = ov::frontend::get_frontend_manager();
}

ov::op::v0::Constant::Constant(const Constant& other, const ov::Shape& new_shape) {
Expand All @@ -225,9 +235,14 @@ ov::op::v0::Constant::Constant(const Constant& other, const ov::Shape& new_shape
m_data = other.m_data;
update_identical_flags(other.m_all_elements_bitwise_identical_checked, other.m_all_elements_bitwise_identical);
constructor_validate_and_infer_types();
m_femgr = ov::frontend::get_frontend_manager();
}

ov::op::v0::Constant::~Constant() = default;
ov::op::v0::Constant::~Constant() {
ilyachur marked this conversation as resolved.
Show resolved Hide resolved
// guarantee m_data is released before femgr
m_data = nullptr;
m_femgr = nullptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

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


string ov::op::v0::Constant::convert_value_to_string(size_t index) const {
string rc;
Expand Down
3 changes: 3 additions & 0 deletions src/frontends/common/include/openvino/frontend/manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ using FrontEndFactory = std::function<FrontEnd::Ptr()>;
/// frontends This is a main frontend entry point for client applications
class FRONTEND_API FrontEndManager final {
public:
using Ptr = std::shared_ptr<FrontEndManager>;

/// \brief Default constructor. Searches and loads of available frontends
FrontEndManager();

Expand Down Expand Up @@ -79,6 +81,7 @@ class FRONTEND_API FrontEndManager final {

template <>
FRONTEND_API FrontEnd::Ptr FrontEndManager::load_by_model(const std::vector<ov::Any>& variants);
FRONTEND_API FrontEndManager::Ptr get_frontend_manager();

// --------- Plugin exporting information --------------

Expand Down
37 changes: 37 additions & 0 deletions src/frontends/common/src/manager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -242,3 +242,40 @@ template <>
FrontEnd::Ptr FrontEndManager::load_by_model(const std::vector<ov::Any>& variants) {
return load_by_model_impl(variants);
}
namespace ov {
namespace frontend {

namespace {

class FrontEndManagerHolder {
std::mutex m_mutex;
std::weak_ptr<FrontEndManager> m_manager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it a weak pointer intentionally? Should the FrontEndManager objects created in this pattern be able to be destroyed and then replaced with other instances if needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is designed as a weak pointer so that it can be destroyed during openvino dll/lib unloaded, ov::core also take a reference of it so that it will not be released before ov::core destruction.


public:
FrontEndManagerHolder(const FrontEndManagerHolder&) = delete;
FrontEndManagerHolder& operator=(const FrontEndManagerHolder&) = delete;
t-jankowski marked this conversation as resolved.
Show resolved Hide resolved

FrontEndManagerHolder() = default;

FrontEndManager::Ptr get() {
std::lock_guard<std::mutex> lock(m_mutex);
auto manager = m_manager.lock();
if (!manager)
m_manager = manager = std::make_shared<FrontEndManager>();
return manager;
}
};

FrontEndManagerHolder& get_frontend_manager_holder() {
static FrontEndManagerHolder frontend_manager_holder;
return frontend_manager_holder;
}

} // namespace

FrontEndManager::Ptr get_frontend_manager() {
return get_frontend_manager_holder().get();
}

} // namespace frontend
ilya-lavrenov marked this conversation as resolved.
Show resolved Hide resolved
} // namespace ov
10 changes: 10 additions & 0 deletions src/inference/dev_api/threading/ie_executor_manager.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,16 @@ class INFERENCE_ENGINE_API_CLASS(ExecutorManager) {
*/
INFERENCE_ENGINE_DEPRECATED("Use IInferencePlugin::executorManager() instead")
static ExecutorManager* getInstance();

/**
* @brief Set TBB terminate flag
* @param flag A boolean value:
* True to terminate tbb during destruction
* False to not terminate tbb during destruction
* @return void
*/
virtual void setTbbFlag(bool flag) = 0;
virtual bool getTbbFlag() = 0;
};

INFERENCE_ENGINE_API_CPP(ExecutorManager::Ptr) executorManager();
Expand Down
9 changes: 9 additions & 0 deletions src/inference/include/openvino/runtime/properties.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -509,6 +509,15 @@ static constexpr Property<uint32_t, PropertyMutability::RW> auto_batch_timeout{"
static constexpr Property<std::tuple<unsigned int, unsigned int, unsigned int>, PropertyMutability::RO>
range_for_async_infer_requests{"RANGE_FOR_ASYNC_INFER_REQUESTS"};

/**
* @brief Read-write property to set whether force terminate tbb when ov core destruction
* value type: boolean
* - True explicitly terminate tbb when ov core destruction
* - False will not involve additional tbb operations when core destruction
* @ingroup ov_runtime_cpp_prop_api
*/
static constexpr Property<bool, PropertyMutability::RW> force_tbb_terminate{"FORCE_TBB_TERMINATE"};

Choose a reason for hiding this comment

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

Please add bindings to it here:
https://github.com/openvinotoolkit/openvino/blob/master/src/bindings/python/src/pyopenvino/core/properties/properties.cpp

Without it Python API will lack new property. Tests for it could be useful as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, will do it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


/**
* @brief Namespace with device properties
*/
Expand Down
26 changes: 26 additions & 0 deletions src/inference/src/ie_core.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include <memory>
#include <mutex>
#include <string>
#include <threading/ie_executor_manager.hpp>
#include <vector>

#include "any_copy.hpp"
Expand Down Expand Up @@ -54,6 +55,11 @@ using namespace std::placeholders;

namespace ov {

namespace frontend {
class FrontEndManager;
std::shared_ptr<FrontEndManager> get_frontend_manager();
} // namespace frontend

// Specify the default device when no device name is provided.
const std::string DEFAULT_DEVICE_NAME = "DEFAULT_DEVICE";

Expand Down Expand Up @@ -258,6 +264,8 @@ class CoreImpl : public ie::ICore, public std::enable_shared_from_this<ie::ICore
}
};

std::shared_ptr<void> frontEndManagerPtr;
ilya-lavrenov marked this conversation as resolved.
Show resolved Hide resolved
ExecutorManager::Ptr executorManagerPtr;
mutable std::unordered_set<std::string> opsetNames;
// TODO: make extensions to be optional with conditional compilation
mutable std::vector<ie::IExtensionPtr> extensions;
Expand Down Expand Up @@ -424,6 +432,8 @@ class CoreImpl : public ie::ICore, public std::enable_shared_from_this<ie::ICore

public:
CoreImpl(bool _newAPI) : newAPI(_newAPI) {
executorManagerPtr = executorManager();
frontEndManagerPtr = ov::frontend::get_frontend_manager();
opsetNames.insert("opset1");
opsetNames.insert("opset2");
opsetNames.insert("opset3");
Expand Down Expand Up @@ -860,6 +870,10 @@ class CoreImpl : public ie::ICore, public std::enable_shared_from_this<ie::ICore
"You can only get_config of the AUTO itself (without devices). "
"get_config is also possible for the individual devices before creating the AUTO on top.");

if (name == ov::force_tbb_terminate.name()) {
const auto flag = executorManager()->getTbbFlag();
return decltype(ov::force_tbb_terminate)::value_type(flag);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose for such needs we need to introduce get_property without device name, since such property is Core specific, not device-specific.
Can we do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, It has been resolved in #12022

Copy link
Contributor

Choose a reason for hiding this comment

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

In that PR you use set_property with no device name, but I think we can also add get_property with no device name to have mirrored API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can add such new interface.

auto parsed = parseDeviceNameIntoConfig(deviceName, arguments);
return GetCPPPluginByName(parsed._deviceName).get_property(name, parsed._config);
}
Expand Down Expand Up @@ -1113,6 +1127,18 @@ class CoreImpl : public ie::ICore, public std::enable_shared_from_this<ie::ICore
void SetConfigForPlugins(const std::map<std::string, std::string>& configMap, const std::string& deviceName) {
auto config = configMap;

for (auto& item : config) {
if (item.first == ov::force_tbb_terminate.name()) {
wangleis marked this conversation as resolved.
Show resolved Hide resolved
auto flag = item.second == CONFIG_VALUE(YES) ? true : false;
executorManager()->setTbbFlag(flag);
config.erase(item.first);
break;
}
}
if (config.empty()) {
return;
ilya-lavrenov marked this conversation as resolved.
Show resolved Hide resolved
}

InferenceEngine::DeviceIDParser parser(deviceName);
std::string clearDeviceName = parser.getDeviceName();

Expand Down
15 changes: 5 additions & 10 deletions src/inference/src/ie_network_reader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -440,11 +440,6 @@ CNNNetwork convert_to_cnnnetwork(std::shared_ptr<ngraph::Function>& function,
OPENVINO_SUPPRESS_DEPRECATED_END
}

ov::frontend::FrontEndManager& get_frontend_manager() {
static ov::frontend::FrontEndManager manager;
return manager;
}

std::vector<ov::Extension::Ptr> wrap_old_extensions(const std::vector<InferenceEngine::IExtensionPtr>& exts) {
std::vector<ov::Extension::Ptr> extensions;
for (const auto& ext : exts) {
Expand Down Expand Up @@ -488,7 +483,7 @@ CNNNetwork details::ReadNetwork(const std::string& modelPath,
#endif

// Try to load with FrontEndManager
auto& manager = get_frontend_manager();
auto manager = ov::frontend::get_frontend_manager();
Copy link
Contributor

Choose a reason for hiding this comment

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

After this change the get_frontend_manager method fro this class is not used any more(in this file at least). Can it be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it cannot, this method is located in namespace ov::frontend. If we didn't explicitly specify its namespace, it cannot be removed. We can still keep it as the same as the original code.

ov::frontend::FrontEnd::Ptr FE;
ov::frontend::InputModel::Ptr inputModel;

Expand All @@ -503,7 +498,7 @@ CNNNetwork details::ReadNetwork(const std::string& modelPath,
params.emplace_back(weights_path);
}

FE = manager.load_by_model(params);
FE = manager->load_by_model(params);
if (FE) {
FE->add_extension(ov_exts);
if (!exts.empty())
Expand All @@ -518,7 +513,7 @@ CNNNetwork details::ReadNetwork(const std::string& modelPath,

const auto fileExt = modelPath.substr(modelPath.find_last_of(".") + 1);
std::string FEs;
for (const auto& fe_name : manager.get_available_front_ends())
for (const auto& fe_name : manager->get_available_front_ends())
FEs += fe_name + " ";
IE_THROW(NetworkNotRead) << "Unable to read the model: " << modelPath
<< " Please check that model format: " << fileExt
Expand Down Expand Up @@ -555,7 +550,7 @@ CNNNetwork details::ReadNetwork(const std::string& model,
#endif // ENABLE_IR_V7_READER

// Try to load with FrontEndManager
auto& manager = get_frontend_manager();
auto manager = ov::frontend::get_frontend_manager();
ov::frontend::FrontEnd::Ptr FE;
ov::frontend::InputModel::Ptr inputModel;

Expand All @@ -567,7 +562,7 @@ CNNNetwork details::ReadNetwork(const std::string& model,
params.emplace_back(weights_buffer);
}

FE = manager.load_by_model(params);
FE = manager->load_by_model(params);
if (FE) {
FE->add_extension(ov_exts);
if (!exts.empty())
Expand Down
3 changes: 3 additions & 0 deletions src/inference/src/threading/ie_cpu_streams_executor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

#include "ie_parallel_custom_arena.hpp"
#include "ie_system_conf.h"
#include "threading/ie_executor_manager.hpp"
#include "threading/ie_thread_affinity.hpp"
#include "threading/ie_thread_local.hpp"

Expand Down Expand Up @@ -185,6 +186,7 @@ struct CPUStreamsExecutor::Impl {
_streams([this] {
return std::make_shared<Impl::Stream>(this);
}) {
_exectorMgr = executorManager();
auto numaNodes = getAvailableNUMANodes();
if (_config._streams != 0) {
std::copy_n(std::begin(numaNodes),
Expand Down Expand Up @@ -294,6 +296,7 @@ struct CPUStreamsExecutor::Impl {
using StreamIdToCoreTypes = std::vector<std::pair<custom::core_type_id, int>>;
StreamIdToCoreTypes total_streams_on_core_types;
#endif
ExecutorManager::Ptr _exectorMgr;
};

int CPUStreamsExecutor::GetStreamId() {
Expand Down
Loading