-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Changes from 20 commits
8692146
606d76a
4de6dfc
0ad5b8d
0627bac
1b85916
cf7703b
0c59d6e
10e386e
32fbc1e
1413484
cf6526a
31e5178
3e96cff
452ca91
47513f0
15030b6
e20b5e5
0c128a2
9f04cc4
78a511f
9646bb1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need it? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It cannot work, we will encounter problem in some CI test, sometimes frontend manager will released before ov::Model, which cause segment fault. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I add such reference at ov::Model is to fix segment fault in CI test failure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can we investigate these issues in order to understand how frontend manager affects There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we have this definition in public place? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the same There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Anyhow I reproduce the same failure in local, such as test_NGraph.py
The failure test item is:
It's obvious that ov::Model has been released before ops, I dump the call trace as below: test_inference_engine/test_NGraph.py ..
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Could you please provide more examples of such issues where static variable holds another DLL? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
|
@@ -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, | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -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; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. CC @ilyachur |
||
|
||
string ov::op::v0::Constant::convert_value_to_string(size_t index) const { | ||
string rc; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add bindings to it here: Without it Python API will lack new property. Tests for it could be useful as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sure, will do it! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
|
||
/** | ||
* @brief Namespace with device properties | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ | |
#include <memory> | ||
#include <mutex> | ||
#include <string> | ||
#include <threading/ie_executor_manager.hpp> | ||
#include <vector> | ||
|
||
#include "any_copy.hpp" | ||
|
@@ -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"; | ||
|
||
|
@@ -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; | ||
|
@@ -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"); | ||
|
@@ -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); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suppose for such needs we need to introduce There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, It has been resolved in #12022 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that PR you use There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
|
@@ -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(); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After this change the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
||
|
@@ -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()) | ||
|
@@ -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 | ||
|
@@ -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; | ||
|
||
|
@@ -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()) | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)