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

G-API: Add synchronous execution for IE backend #22588

Merged
merged 8 commits into from
Oct 4, 2022

Conversation

TolyaTalamanov
Copy link
Contributor

@TolyaTalamanov TolyaTalamanov commented Sep 29, 2022

Pull Request Readiness Checklist

See details at https://github.com/opencv/opencv/wiki/How_to_contribute#making-a-good-pull-request

  • I agree to contribute to the project under Apache 2 License.
  • To the best of my knowledge, the proposed patch is not based on a code under GPL or another license that is incompatible with OpenCV
  • The PR is proposed to the proper branch
  • There is a reference to the original bug report and related work
  • There is accuracy test, performance test and test data in opencv_extra repository, if applicable
    Patch to opencv_extra has the same branch name.
  • The feature is well documented and sample code can be built with the project CMake

Overview

The implementation is straight forward: replace IE::InferRequest to IInferExecutor which encapsulates how execution should be started (Infer()/StartAsync). Every kind of IInferExecutor must notify RequestPool when execution finishes by using by using callback that was passed while instantiation (see m_notify()) it helps RequestPool to understand which requests are working and which are idle.

cv::gimpl::ie::RequestPool::RequestPool(std::vector<InferenceEngine::InferRequest>&& requests) {
for (size_t i = 0; i < requests.size(); ++i) {
m_requests.emplace_back(
std::make_shared<AsyncInferExecutor>(std::move(requests[0]),
Copy link
Contributor Author

@TolyaTalamanov TolyaTalamanov Sep 29, 2022

Choose a reason for hiding this comment

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

Here the question how to switch between AsyncInferExecutor and SyncInferExecutor. I see two options here:

  1. Provide to user handle: params.cfgInferenceAPI(ParamDesc::API api) // ParamDesc::API::ASYNC by default
  2. Calculate it based on number of infer requests. if nireq > 1 then it must be AsyncInferRequest since there is no sense to infer synchronously with multiple infer requests.

I prefer the first option since it's more flexible and sometimes there is a difference between sync/async mode even with nireq == 1.

@dmatveev do you mind 1) option?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we have different number of nireq each time - is it possible? Voting for the first option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, discussed locally

@TolyaTalamanov
Copy link
Contributor Author

@smirnov-alexey Could you have a look, please?

@smirnov-alexey
Copy link
Contributor

@TolyaTalamanov please, add some tests

Copy link
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

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

What is the reason for these changes? I see the synchronous path isn't used yet?

Do you plan to make it user-controllable? What are the benefits?

// RunF - function which is set blobs and run async inference.
// CallbackF - function which is obtain output blobs and post it to output.
// SetInputDataF - function which set input data.
// ReadOutputDataF - function which read output data.
struct Task {
Copy link
Contributor

Choose a reason for hiding this comment

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

If there's no body (the execution callback) in Task, is it still a Task?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually the body is set_input_data and read_output_data. I guess it still be called Task

@TolyaTalamanov
Copy link
Contributor Author

What is the reason for these changes? I see the synchronous path isn't used yet?

Do you plan to make it user-controllable? What are the benefits?

I'd glad to have opportunity to control sync/async behaviour:

  1. benchmark_app exposes this as -api=async/sync flag.
  2. In case nireq=1 sync mode works faster. In most cases gain is small, but I observed around 20-30% if model has multiple inputs (up to 30+ for noise suppression models). It's definitely OpenVINO & plugins issues, but having this simple switcher might save us a headache.
  3. Debugging nireq=1 in sync mode is easier whether it's gdb or vtune

@TolyaTalamanov
Copy link
Contributor Author

@TolyaTalamanov please, add some tests

Added

@TolyaTalamanov
Copy link
Contributor Author

@dmatveev @smirnov-alexey Addressed comments, have a look, please :)

Copy link
Contributor

@dmatveev dmatveev left a comment

Choose a reason for hiding this comment

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

👍

@asmorkalov
Copy link
Contributor

@TolyaTalamanov Please rebase the PR and fix conflicts.

@TolyaTalamanov
Copy link
Contributor Author

@TolyaTalamanov Please rebase the PR and fix conflicts.

Done

@TolyaTalamanov
Copy link
Contributor Author

@asmorkalov It's ready to be merged

@asmorkalov asmorkalov merged commit bf5d7c0 into opencv:4.x Oct 4, 2022
@alalek alalek mentioned this pull request Jan 8, 2023
@asmorkalov asmorkalov added this to the 4.7.0 milestone Jan 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants