Skip to content

[G-API] Pipeline modeling tool - expand performance report#22542

Merged
asmorkalov merged 1 commit intoopencv:4.xfrom
TolyaTalamanov:at/expand-performance-report-with-metrics
Oct 3, 2022
Merged

[G-API] Pipeline modeling tool - expand performance report#22542
asmorkalov merged 1 commit intoopencv:4.xfrom
TolyaTalamanov:at/expand-performance-report-with-metrics

Conversation

@TolyaTalamanov
Copy link
Copy Markdown
Contributor

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

@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

@smirnov-alexey @dmatveev Folks, could you have a look?

<< first_run_latency << " ms";

ss << name << ": "<< std::endl
<< std::string(2, ' ') << "Warm up time: " << warmup_time << " ms" << std::endl
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Compilation -> Warm up because it might be confused with OpenVINO model compilation.


ss << name << ": "<< std::endl
<< std::string(2, ' ') << "Warm up time: " << warmup_time << " ms" << std::endl
<< std::string(2, ' ') << "Elapsed time: " << elapsed << " ms" << std::endl
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: switch from std::endl to \n

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown
Contributor

@smirnov-alexey smirnov-alexey left a comment

Choose a reason for hiding this comment

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

Looks good

m_perf.max_latency = utils::max(m_perf.latencies);
m_perf.first_latency = m_perf.latencies[0];

// NB: Count how many executions won't fit to cameta latency iterval
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

iterval -> interval

const bool drop_frames);
bool pull(cv::gapi::wip::Data& data) override;
cv::GMetaArg descr_of() const override;
double getLatency() const { return m_latency; };
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

other methods don't have get in their name. Just for consistency

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

<< first_run_latency << " ms";

ss << name << ": \n"
<< std::string(2, ' ') << "Warm up time: " << warmup_time << " ms\n"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe there are iomanip ways to do padding instead of constructing empty strings... it is about streaming, after all

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Replaced to ' '

<< std::string(4, ' ') << "max: " << max_latency << " ms\n"
<< std::string(4, ' ') << "avg: " << std::fixed << std::setprecision(3)
<< avg_latency << " ms\n"
<< std::string(2, ' ') << "Throughput: " << std::fixed << std::setprecision(3)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also..
Why couldn't you just put those extra spaces into the string literals? You have them anyway, and that way padding would have been seen in the code, too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done

@asmorkalov
Copy link
Copy Markdown
Contributor

@TolyaTalamanov Please rebase and fix conflicts.

@TolyaTalamanov TolyaTalamanov force-pushed the at/expand-performance-report-with-metrics branch from a015f0f to b1d28d5 Compare October 3, 2022 09:06
@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

@TolyaTalamanov Please rebase and fix conflicts.

Done

@TolyaTalamanov
Copy link
Copy Markdown
Contributor Author

@asmorkalov Can it be merged?

@asmorkalov asmorkalov merged commit 468431c into opencv:4.x Oct 3, 2022
@alalek alalek mentioned this pull request Jan 8, 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.

4 participants