Added ITT traces to GStreamingExecutor #19917

AsyaPronina · 2021-04-16T01:33:16Z

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 other license that is incompatible with OpenCV
The PR is proposed to proper branch
There is reference to 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

force_builders=Custom,Custom Win,Custom Mac
build_gapi_standalone:Linux x64=ade-0.1.1f
build_gapi_standalone:Win64=ade-0.1.1f
build_gapi_standalone:Mac=ade-0.1.1f
build_gapi_standalone:Linux x64 Debug=ade-0.1.1f


Xbuild_image:Custom=centos:7
Xbuildworker:Custom=linux-1
build_gapi_standalone:Custom=ade-0.1.1f

build_image:Custom=ubuntu-openvino-2021.3.0:20.04
build_image:Custom Win=openvino-2021.2.0
build_image:Custom Mac=openvino-2021.2.0

test_modules:Custom=gapi,python2,python3,java
test_modules:Custom Win=gapi,python2,python3,java
test_modules:Custom Mac=gapi,python2,python3,java

buildworker:Custom=linux-1
# disabled due high memory usage: test_opencl:Custom=ON
test_opencl:Custom=OFF
test_bigdata:Custom=1
test_filter:Custom=*

modules/gapi/src/backends/cpu/gcpubackend.cpp

modules/gapi/src/executor/gstreamingexecutor.cpp

dmatveev

Looks cool!

modules/gapi/CMakeLists.txt

modules/gapi/src/executor/gstreamingexecutor.cpp

modules/gapi/src/utils/gapi_itt.cpp

modules/gapi/src/utils/gapi_itt.hpp

dmatveev

I believe SyncQueue instrumentation can be dropped for now as it only bloats the trace and is nearly equal to StreamingInput::get and so on. Overall well done!

modules/gapi/CMakeLists.txt

dmatveev · 2021-04-21T20:18:44Z

modules/gapi/src/compiler/gislandmodel.cpp

+            backend_name = matches[1].str();
+            island_name = island_name + "_" + backend_name;
+        }
+    }


If match fails, I'd use the full backend type name instead of nothing.

Full backend name will contain some mangled symbhols and numbers which are maps to "::" namespace resolution operator, is it applicable?

Yes, please use the full name in that case.

Thanks, fixed!

modules/gapi/src/compiler/gislandmodel.cpp

modules/gapi/src/executor/gstreamingexecutor.cpp

AsyaPronina · 2021-04-26T10:06:25Z

Hi Anton(@anton-potapov)!
Could you please review changes in gtbbexecutor.cpp?
I had to do them because Linux OpenCL build wasn't green without.

dmatveev

I think TBB changes are ok and you can proceed with merge after fixing the non-regular island name handling.

dmatveev · 2021-04-28T11:55:11Z

modules/gapi/src/compiler/gislandmodel.cpp

+    if (std::regex_search(backend_impl_type_name, matches, rgx)) {
+        if (2ul == matches.size()) {
+            backend_name = matches[1].str();
+            island_name = island_name + "_" + backend_name;


This one is reworked!

dmatveev · 2021-04-28T11:55:31Z

modules/gapi/src/compiler/gislandmodel.cpp

+            backend_name = matches[1].str();
+            island_name = island_name + "_" + backend_name;
+        }
+    }


Yes, please use the full name in that case.

dmatveev · 2021-04-28T11:56:31Z

modules/gapi/src/executor/gstreamingexecutor.cpp

@@ -1443,6 +1491,13 @@ void cv::gimpl::GStreamingExecutor::setSource(GRunArgs &&ins)
            out_queues.push_back(reader_queues(*m_island_graph, out_eh));
        }

+        // Create just empty island meta information
+        std::string island_meta_info { };
+#if defined(OPENCV_WITH_ITT) && !defined(GAPI_STANDALONE)


btw whats wrong with STANDALONE here?

Fixed!
Thanks a lot!
I have realized that OPENCV_WITH_ITT will be defined only if ITT dependency will be successfully resolved, what can be done only if we building with OpenCV. So, there will be no such define in G-API standalone mode and check for !GAPI_STANDALONE is extra.
I have added a comment about this in itt.hpp file and TODO to support ITT traces in GAPI standalone mode in CMakeLists.txt.

anton-potapov · 2021-05-04T07:48:32Z

modules/gapi/src/compiler/gislandmodel.hpp

@@ -290,7 +292,8 @@ namespace GIslandModel
    //     from the original model (! don't mix with DataSlot)
    // FIXME: GAPI_EXPORTS because of tests only!
    ade::NodeHandle GAPI_EXPORTS producerOf(const ConstGraph &g, ade::NodeHandle &data_nh);
-
+    // traceName - returns pretty island name for passed island node


I believe in should be traceIslandName

Thanks, Anton!!

AsyaPronina · 2021-05-04T13:34:06Z

Hi Maxim(@mshabunin), Alexander(@alalek)!
Assigning this PR to Ruslan(@rgarnov) as I didn't have time to finish it before DmitryM(@dmatveev)'s vacation.

mshabunin · 2021-05-05T13:31:10Z

modules/gapi/src/compiler/gislandmodel.cpp

+    std::string backend_name = "";
+    auto& backend_impl = island_ptr->backend().priv();
+    std::string backend_impl_type_name = typeid(backend_impl).name();
+    std::regex rgx("G([a-zA-Z0-9]+)BackendImpl");


std::regex is not working in GCC<4.9 (https://stackoverflow.com/questions/12530406/is-gcc-4-8-or-earlier-buggy-about-regular-expressions), but we still kinda support CentOS 7 which uses GCC 4.8.5. Is it possible to rewrite this part without regex usage?

AsyaPronina requested review from dmatveev, anton-potapov and dbudniko April 16, 2021 01:33

AsyaPronina commented Apr 16, 2021

View reviewed changes

modules/gapi/src/backends/cpu/gcpubackend.cpp Show resolved Hide resolved

AsyaPronina commented Apr 16, 2021

View reviewed changes

modules/gapi/src/executor/gstreamingexecutor.cpp Outdated Show resolved Hide resolved

AsyaPronina commented Apr 16, 2021

View reviewed changes

modules/gapi/src/executor/gstreamingexecutor.cpp Show resolved Hide resolved

AsyaPronina commented Apr 16, 2021

View reviewed changes

modules/gapi/src/executor/gstreamingexecutor.cpp Show resolved Hide resolved

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch from ff8bb51 to 3757ce2 Compare April 16, 2021 01:54

AsyaPronina commented Apr 19, 2021

View reviewed changes

modules/gapi/src/executor/gstreamingexecutor.cpp Outdated Show resolved Hide resolved

dmatveev self-assigned this Apr 19, 2021

dmatveev added the category: g-api / gapi label Apr 19, 2021

dmatveev added this to the 4.5.3 milestone Apr 19, 2021

dmatveev reviewed Apr 19, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch 7 times, most recently from 66a90b7 to 09a5fa9 Compare April 21, 2021 17:36

dmatveev reviewed Apr 21, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch 9 times, most recently from 9ad402b to a2b654a Compare April 23, 2021 20:27

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch 3 times, most recently from 7caf350 to d8ece4a Compare April 23, 2021 21:31

dmatveev reviewed Apr 28, 2021

View reviewed changes

anton-potapov reviewed May 4, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch 3 times, most recently from a6faabb to 96e2e5a Compare May 4, 2021 13:17

AsyaPronina assigned rgarnov May 4, 2021

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch from 96e2e5a to 12170ab Compare May 4, 2021 13:30

rgarnov approved these changes May 4, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch from 12170ab to 6d630ab Compare May 4, 2021 20:33

mshabunin reviewed May 5, 2021

View reviewed changes

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch 3 times, most recently from f82892a to 4568709 Compare May 7, 2021 18:20

mshabunin approved these changes May 7, 2021

View reviewed changes

Added ITT traces to GStreamingExecutor

3a49ff9

AsyaPronina force-pushed the asyadev/itt_traces_in_gstreamingexecutor branch from 4568709 to 3a49ff9 Compare May 11, 2021 09:55

alalek unassigned dmatveev May 11, 2021

alalek merged commit df05bc6 into opencv:master May 11, 2021

alalek mentioned this pull request Jun 4, 2021

(5.x) Merge 4.x #20216

Merged

dmatveev mentioned this pull request Sep 14, 2023

Fail to build 4.5.2 with cuda, TBB and mkl #19954

Closed

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ITT traces to GStreamingExecutor #19917

Added ITT traces to GStreamingExecutor #19917

AsyaPronina commented Apr 16, 2021

dmatveev left a comment

dmatveev left a comment

dmatveev Apr 21, 2021

AsyaPronina Apr 21, 2021 •

edited

dmatveev Apr 28, 2021

AsyaPronina May 4, 2021

AsyaPronina commented Apr 26, 2021

dmatveev left a comment

dmatveev Apr 28, 2021

AsyaPronina May 4, 2021

dmatveev Apr 28, 2021

dmatveev Apr 28, 2021

AsyaPronina May 4, 2021

anton-potapov May 4, 2021

AsyaPronina May 4, 2021

AsyaPronina commented May 4, 2021

mshabunin May 5, 2021

Added ITT traces to GStreamingExecutor #19917

Added ITT traces to GStreamingExecutor #19917

Conversation

AsyaPronina commented Apr 16, 2021

Pull Request Readiness Checklist

dmatveev left a comment

Choose a reason for hiding this comment

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsyaPronina Apr 21, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsyaPronina commented Apr 26, 2021

dmatveev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AsyaPronina commented May 4, 2021

Choose a reason for hiding this comment

AsyaPronina Apr 21, 2021 •

edited