Async API for GAPI by anton-potapov · Pull Request #14346 · opencv/opencv

anton-potapov · 2019-04-16T09:20:32Z

naive implementation as a starting point

This pullrequest changes

Adds a naive implementation of Asynchronous API for GAPI module

modules/gapi/CMakeLists.txt

modules/gapi/include/opencv2/gapi/gcompiled_async.hpp

modules/gapi/include/opencv2/gapi/gcomputation_async.hpp

modules/gapi/src/compiler/gasync.cpp

dmatveev · 2019-04-16T13:48:44Z

Seems ok, fix minor stuff and add missing pieces (and make build green of course).

Thanks

modules/gapi/test/gapi_async_test.cpp

anton-potapov · 2019-04-16T14:15:39Z

@dmatveev @rgarnov @dbudniko please take a look

modules/gapi/src/compiler/gasync.cpp

alalek · 2019-04-17T10:49:54Z

modules/gapi/src/compiler/gasync.cpp

+                    mtx.lock();
+                    mtx.unlock();
+                    cv.notify_one();
+                    thrd.join();


Be careful with waiting for notifications from worker threads during "cleanup".
Win32 ExitProcess() terminates all non-main threads unconditionally - probably with broken synchronization state - holding mutexes (user space at least) and other synchronization primitives.
And after that, it calls atexit() handlers from shared libraries, like this destructor.

Hopefully thrd.joinable() can handle that properly.

alalek · 2019-04-17T10:50:03Z

modules/gapi/src/compiler/gasync.cpp

+            }
+        };
+
+        async_service the_ctx;


Probably static (or anonymous namespace)

What is about initialization "on-demand"?

static async_service& getAsyncService() { static async_service g_asyncService; return g_asyncService; } ... the_ctx. => getAsyncService().

+1 for on-demand

Thread It is already on created on - demand - during first call of add_task() async.cpp line 49

modules/gapi/test/gapi_async_test.cpp

dmatveev · 2019-04-24T10:42:53Z

modules/gapi/test/gapi_async_test.cpp

+
+    auto f = cv::gapi::wip::async_apply(self_mul,cv::gin(in_mat), cv::gout(out));
+    f.wait();
+}


Why it is DISABLED BTW?

enabled back

No comments at all. What is the general idea behind these tests?

dmatveev · 2019-04-24T10:44:14Z

modules/gapi/src/executor/gasync.cpp

+        };
+
+        async_service the_ctx;
+    }


This whole block of code still lacks comments. "What author has wanted to say?"

see below at line 128

Is it enough - or more comments are required ?

fixed the code and added comments

I still don't understand what is happening here. Queue, second queue (local instance). What is the purpose of this async_service? Why it's so complicated?

Is there any reply on @alalek's concerns regarding #14346 (comment) ?

- naive implementation as a starting point

anton-potapov · 2019-04-26T08:18:30Z

@dbudniko please review , it seems that @dmatveev is not reachable ...
@alalek could you please change the reviewer to @dbudniko or review it yourself ? (All design issues were resolved, only fixes for technical issues are left for review )

dbudniko · 2019-04-26T12:19:57Z

modules/gapi/include/opencv2/gapi/gcompiled_async.hpp

+#ifndef OPENCV_GAPI_GCOMPILED_ASYNC_HPP
+#define OPENCV_GAPI_GCOMPILED_ASYNC_HPP
+
+#include <future>


//for std::future
to align with next std headers comments.

dbudniko · 2019-04-26T12:21:44Z

modules/gapi/include/opencv2/gapi/gcompiled_async.hpp

+    class GCompiled;
+
+namespace gapi{
+namespace wip {


May be it worth to choose better name for namespace? Something containing "volatile".

No, wip was my proposal (communicated to Anton "verbally")

dbudniko · 2019-04-26T12:23:30Z

modules/gapi/include/opencv2/gapi/gcompiled_async.hpp

+
+namespace gapi{
+namespace wip {
+    GAPI_EXPORTS void                async(GCompiled& gcmpld, std::function<void(std::exception_ptr)>&& callback, GRunArgs &&ins, GRunArgsP &&outs);


Why do we need these overloads? Please provide comments for usage scenario for both.

modules/gapi/include/opencv2/gapi/gcompiled_async.hpp

dbudniko · 2019-04-26T12:26:19Z

modules/gapi/include/opencv2/gapi/gcomputation_async.hpp

+    class GComputation;
+namespace gapi {
+namespace wip  {
+    GAPI_EXPORTS void                async_apply(GComputation& gcomp, std::function<void(std::exception_ptr)>&& callback, GRunArgs &&ins, GRunArgsP &&outs, GCompileArgs &&args = {});


Again, please, provide comments regarding these overloads.

dbudniko · 2019-04-26T12:26:39Z

modules/gapi/src/executor/gasync.cpp

+
+#include <future>
+#include <condition_variable>
+//#include <chrono>


Please remove

dbudniko · 2019-04-26T12:43:02Z

modules/gapi/src/executor/gasync.cpp

+        };
+
+        async_service the_ctx;
+    }


I still don't understand what is happening here. Queue, second queue (local instance). What is the purpose of this async_service? Why it's so complicated?

dbudniko · 2019-04-26T12:44:51Z

modules/gapi/src/executor/gasync.cpp

+}//namespace
+
+//For now these async functions are simply wrapping serial version of apply/operator() into a functor.
+//These functors are then serialized into single queue, which is when processed by a devoted background thread.


"when" - "then" missprint?

thanks, removed

dbudniko · 2019-04-26T12:51:49Z

modules/gapi/test/gapi_async_test.cpp

+
+    auto f = cv::gapi::wip::async_apply(self_mul,cv::gin(in_mat), cv::gout(out));
+    f.wait();
+}


No comments at all. What is the general idea behind these tests?

dmatveev

I think we can go ahead with these changes, since critical issues were addressed and API is more or less stabilized for external team use.

I believe Anton will handle the rest with time.

dmatveev · 2019-04-29T14:36:37Z

modules/gapi/src/executor/gasync.cpp

+                        q.swap(second_q);
+                        lck.unlock();
+
+                        while (!second_q.empty())


should this while condition be extended with exiting check as well?

What is the policy on exiting? Should be wait for all issues request or just "forget" it?

dmatveev · 2019-04-29T14:40:53Z

modules/gapi/src/executor/gasync.cpp

+        };
+
+        async_service the_ctx;
+    }


Is there any reply on @alalek's concerns regarding #14346 (comment) ?

dmatveev · 2019-04-30T07:56:51Z

👍 @alalek can you please proceed with merge?

dmatveev

"Approved with ARs"

* Async API for GAPI - naive implementation as a starting point * Fix namespace comment in header

anton-potapov force-pushed the gapi_async branch from 0ea5786 to 5974b67 Compare April 16, 2019 12:21

dmatveev reviewed Apr 16, 2019

View reviewed changes

anton-potapov force-pushed the gapi_async branch from 5974b67 to 1f12b33 Compare April 16, 2019 13:53

SDxKeeper reviewed Apr 16, 2019

View reviewed changes

modules/gapi/test/gapi_async_test.cpp Outdated Show resolved Hide resolved

anton-potapov force-pushed the gapi_async branch from 1f12b33 to 7c4f219 Compare April 16, 2019 14:14

alalek reviewed Apr 17, 2019

View reviewed changes

anton-potapov force-pushed the gapi_async branch from 0b5592c to 36bdae0 Compare April 19, 2019 15:57

alalek reviewed Apr 22, 2019

View reviewed changes

modules/gapi/test/gapi_async_test.cpp Outdated Show resolved Hide resolved

dmatveev reviewed Apr 24, 2019

View reviewed changes

anton-potapov force-pushed the gapi_async branch from aea3a8e to 9321944 Compare April 24, 2019 15:50

Async API for GAPI

adbc6d7

- naive implementation as a starting point

anton-potapov force-pushed the gapi_async branch from 9321944 to adbc6d7 Compare April 25, 2019 07:38

dbudniko reviewed Apr 26, 2019

View reviewed changes

dmatveev reviewed Apr 29, 2019

View reviewed changes

Fix namespace comment in header

602526a

dmatveev approved these changes Apr 30, 2019

View reviewed changes

alalek assigned dmatveev Apr 30, 2019

alalek merged commit f5801ee into opencv:master Apr 30, 2019

anton-potapov mentioned this pull request May 6, 2019

Async API for GAPI #14492

Merged

anton-potapov deleted the gapi_async branch May 8, 2019 11:20

anton-potapov mentioned this pull request Oct 18, 2019

G-API: Doxygen documentatation for Async API #15735

Merged

alalek mentioned this pull request Jan 15, 2021

gapi(async): use lazy-initialization of async_service singleton #19326

Merged

a-sajjad72 pushed a commit to a-sajjad72/opencv that referenced this pull request Mar 30, 2023

Merge pull request opencv#14346 from anton-potapov:gapi_async

ea0d91a

* Async API for GAPI - naive implementation as a starting point * Fix namespace comment in header

Uh oh!

Conversation

anton-potapov commented Apr 16, 2019

This pullrequest changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dmatveev commented Apr 16, 2019

Uh oh!

Uh oh!

anton-potapov commented Apr 16, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalek Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anton-potapov commented Apr 26, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmatveev commented Apr 30, 2019

Uh oh!

dmatveev left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

alalek Apr 17, 2019 •

edited

Loading