[OVEP] Fixed coverity issues #687
Conversation
| try { | ||
| ov::Tensor* tensor = nullptr; | ||
| { | ||
| std::unique_lock lock(mutex_); |
There was a problem hiding this comment.
For automatic mutex unlocking you can use lock guards like below, this approach is much safer -
| std::unique_lock lock(mutex_); | |
| std::lock_guard<std::mutex> lock(mutex_); |
The ov::Tensor* tensor = nullptr; can still be inside the same scope here.
There was a problem hiding this comment.
Placing the delete operation within the scoped critical section results in a Coverity medium-severity. Additionally, since delete is a relatively time-consuming operation, which can potentially block other threads, so I placed the delete operation outside, which will decrease the time for my critical section.
There was a problem hiding this comment.
Sure. can you use the lock_guards instead for both Alloc() & Free() calls like mentioned above as you are removing lock.unlock() in your change ( this might lead to deadlocks, perf degradation, logical errors), this will ensure auto unlocking mechanism. This is much simpler to manage & execute the flow without having to unlock the mutes
There was a problem hiding this comment.
ig, this ca n still go in, we can change this later, this needs to go in today due to some release issues, can you please approve this for now and i can make the change later, may be next week
jatinwadhwa921
force-pushed
the
jatin_cov_1_22
branch
from
1d066a4 to
13d9cc3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR addresses Coverity-reported issues by refining lock scope management and optimizing parameter passing across OpenVINO execution provider components.
- Simplified lock/unlock logic in OVRTAllocator::Free and clarified scope
- Standardized loop iteration to use
const auto&for efficiency - Applied
std::moveto reduce unnecessary shared_ptr copies in provider factory and execution provider
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| onnxruntime/core/providers/openvino/ov_allocator.cc | Simplified locking scope and ensured proper tensor deletion |
| onnxruntime/core/providers/openvino/openvino_provider_factory.cc | Updated loops to use const auto& and moved parameters for efficiency |
| onnxruntime/core/providers/openvino/openvino_execution_provider.cc | Applied std::move to shared_context in constructor init list |
| { | ||
| std::unique_lock lock(mutex_); | ||
| auto it = allocated_.find(p); | ||
| if (it != allocated_.end()) { | ||
| ov::Tensor* tensor = it->second; | ||
| tensor = it->second; | ||
| allocated_.erase(it); | ||
| lock.unlock(); | ||
| } | ||
| } // lock will be automatically released when it goes out of scope |
There was a problem hiding this comment.
[nitpick] The extra inner scope around the lock is unnecessary and the comment on the closing brace is misleading. Consider removing the extra {} block and scoping the unique_lock directly to simplify control flow and improve readability.
| // (potential) SharedContext creation. | ||
| auto ov_core = OVCore::Get(); | ||
| pi.device_type = ParseDeviceType(ov_core, provider_options); | ||
| pi.device_type = ParseDeviceType(std::move(ov_core), provider_options); |
There was a problem hiding this comment.
[nitpick] Moving ov_core into ParseDeviceType isn’t necessary since it isn’t reused afterward. Consider passing ov_core by const reference or by value without std::move to preserve clarity of ownership semantics.
| pi.device_type = ParseDeviceType(std::move(ov_core), provider_options); | |
| pi.device_type = ParseDeviceType(ov_core, provider_options); |
Fixed coverity issues