Feature/[SE] scheduler #992

iceseer · 2021-04-26T12:32:59Z

Description of the Change

Added scheduler with execution thread loop. Can be used to manage your own process loop in a current thread.

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

lebdron · 2021-04-26T16:15:17Z

test/module/irohad/subscription/subscription_test.cpp

+  auto tid = manager->dispatcher()->bind(scheduler);
+  ASSERT_TRUE(tid);
+
+  utils::WaitForSingleObject complete[2];


Suggested change

utils::WaitForSingleObject complete[2];

Can be removed because the test is single-threaded.

lebdron · 2021-04-26T16:15:25Z

test/module/irohad/subscription/subscription_test.cpp

+ * @and they subscribe to different events in a single current scheduler
+ * @and both events are notified followed by dispose and process
+ * @then the handlers of each subscriber must be called once and process finish
+ * it's loop


Suggested change

* it's loop

* its loop

lebdron · 2021-04-26T16:33:04Z

irohad/subscription/async_dispatcher_impl.hpp

+    }
+
+    bool unbind(Tid tid) override {
+      return binded_.exclusiveAccess([tid](BoundContexts &bound) {


Suggested change

return binded_.exclusiveAccess([tid](BoundContexts &bound) {

return bound_.exclusiveAccess([tid](BoundContexts &bound) {

kuvaldini · 2021-04-26T16:46:45Z

irohad/subscription/async_dispatcher_impl.hpp

   private:
-    using Parent = IDispatcher<kCount, kPoolSize>;
+    using Parent = IDispatcher;

    struct ThreadHandlerContext {
      /// Worker thread to execute tasks


This is not a thread from now

kuvaldini · 2021-04-26T16:51:42Z

irohad/subscription/scheduler_impl.hpp

+    }
+
+    void addDelayed(std::chrono::microseconds timeout, Task &&t) override {
+#ifdef SE_SYNC_CALL_IF_SAME_THREAD


Please use templated constant or constexpr contruction-time constant or anything but not define.

Template parameter allows you to execute both kind of code. But you can't.

kuvaldini · 2021-04-26T16:51:46Z

irohad/subscription/scheduler_impl.hpp

+ * If you need to execute task, that was made in this thread and want to be
+ * executed in the same thread without delay - you need to uncomment this define
+ */
+//#define SE_SYNC_CALL_IF_SAME_THREAD


no-no-no! Looks like a dead code in future.

kuvaldini · 2021-04-26T16:53:11Z

irohad/subscription/subscription_engine.hpp

@@ -6,6 +6,7 @@
 #ifndef IROHA_SUBSCRIPTION_SUBSCRIPTION_ENGINE_HPP
 #define IROHA_SUBSCRIPTION_SUBSCRIPTION_ENGINE_HPP

+#include <assert.h>


Suggested change

#include <assert.h>

#include <cassert>

static_assert is not defined in cassert

kuvaldini · 2021-04-26T16:54:22Z

irohad/subscription/subscription_engine.hpp

@@ -91,18 +92,17 @@ namespace iroha::subscription {
     * be kept valid in case the other subscriber will be deleted from this
     * container)
     */
-    template <typename Dispatcher::Tid kTid>
-    IteratorType subscribe(SubscriptionSetId set_id,
+    IteratorType subscribe(typename Dispatcher::Tid tid,


Pls also fix doc @tparam

kuvaldini · 2021-04-26T17:15:23Z

test/module/irohad/subscription/subscription_test.cpp

+  subscriber1->subscribe(1ull, event_id);
+  subscriber2->subscribe(1ull, event_id);


Suggested change

subscriber1->subscribe(1ull, event_id);

subscriber2->subscribe(1ull, event_id);

subscriber1->subscribe(1, event_id);

subscriber2->subscribe(1, event_id);

No. If so, we need 1u. 1 is a signed int type.

kuvaldini · 2021-04-26T17:50:37Z

irohad/subscription/async_dispatcher_impl.hpp

+      if (!scheduler)
+        return std::nullopt;
+
+      return bound_.exclusiveAccess([scheduler(std::move(scheduler))](


Just for readability less parenthes:

Suggested change

return bound_.exclusiveAccess([scheduler(std::move(scheduler))](

return bound_.exclusiveAccess([scheduler=std::move(scheduler)](

The meaning here is the construction of an object and you need to call it as a constructor.

kuvaldini · 2021-04-26T17:54:22Z

irohad/subscription/async_dispatcher_impl.hpp

+                                        BoundContexts &bound) {
+        auto const execution_tid = kHandlersCount + bound.next_tid_offset;
+        assert(bound.contexts.find(execution_tid) == bound.contexts.end());
+        bound.contexts[execution_tid] = ThreadHandlerContext{scheduler, false};


You have replaced terminology ThreadHandler with Sceduler - pls rename class.

Suggested change

bound.contexts[execution_tid] = ThreadHandlerContext{scheduler, false};

bound.contexts[execution_tid] = ShedulerContext{scheduler, false};

kuvaldini · 2021-04-26T18:42:35Z

irohad/subscription/scheduler_impl.hpp

+          try {
+            if (task)
+              task();
+          } catch (...) {


sure? let's log error at least!

this is the last barrier

kuvaldini · 2021-04-26T18:44:48Z

irohad/subscription/scheduler_impl.hpp

+
+  class SchedulerBase : public IScheduler, utils::NoCopy, utils::NoMove {
+   private:
+    using Time = std::chrono::high_resolution_clock;


I'd prefer same terminology as std:: and boost::

Suggested change

using Time = std::chrono::high_resolution_clock;

using Clock = std::chrono::high_resolution_clock;

I like time and timestamp much more than clock and time-point.

bind/unbind additional schedulers to dispatcher Signed-off-by: iceseer <iceseer@gmail.com>

iceseer force-pushed the feature/se_scheduler branch 2 times, most recently from 5bee6ad to b43e11a Compare April 26, 2021 16:09

iceseer requested review from lebdron and kuvaldini April 26, 2021 16:09

iceseer force-pushed the feature/se_scheduler branch 2 times, most recently from d9b877d to 3ee170e Compare April 26, 2021 16:31

lebdron approved these changes Apr 26, 2021

View reviewed changes

lebdron reviewed Apr 26, 2021

View reviewed changes

iceseer force-pushed the feature/se_scheduler branch from 3ee170e to eb8e856 Compare April 26, 2021 16:36

kuvaldini requested changes Apr 26, 2021

View reviewed changes

iceseer requested a review from kuvaldini April 27, 2021 08:26

iceseer force-pushed the feature/se_scheduler branch from 0313a4d to 32ce6b4 Compare April 27, 2021 08:55

kuvaldini approved these changes Apr 27, 2021

View reviewed changes

iceseer force-pushed the feature/se_scheduler branch from 32ce6b4 to 0a7cd27 Compare April 27, 2021 11:14

added schedulers

2330cd7

bind/unbind additional schedulers to dispatcher Signed-off-by: iceseer <iceseer@gmail.com>

iceseer force-pushed the feature/se_scheduler branch from 0a7cd27 to 2330cd7 Compare April 27, 2021 12:19

iceseer merged commit 4569564 into hyperledger:support/1.2.x Apr 27, 2021

iceseer deleted the feature/se_scheduler branch April 27, 2021 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/[SE] scheduler #992

Feature/[SE] scheduler #992

iceseer commented Apr 26, 2021

lebdron Apr 26, 2021

iceseer Apr 26, 2021

lebdron Apr 26, 2021

iceseer Apr 26, 2021

lebdron Apr 26, 2021

iceseer Apr 26, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

kuvaldini Apr 26, 2021

iceseer Apr 27, 2021

	return binded_.exclusiveAccess([tid](BoundContexts &bound) {
	return bound_.exclusiveAccess([tid](BoundContexts &bound) {

		subscriber1->subscribe(1ull, event_id);
		subscriber2->subscribe(1ull, event_id);

	return bound_.exclusiveAccess([scheduler(std::move(scheduler))](
	return bound_.exclusiveAccess([scheduler=std::move(scheduler)](

	bound.contexts[execution_tid] = ThreadHandlerContext{scheduler, false};
	bound.contexts[execution_tid] = ShedulerContext{scheduler, false};

	using Time = std::chrono::high_resolution_clock;
	using Clock = std::chrono::high_resolution_clock;

Feature/[SE] scheduler #992

Feature/[SE] scheduler #992

Conversation

iceseer commented Apr 26, 2021

Description of the Change

Benefits

Possible Drawbacks

Usage Examples or Tests [optional]

Alternate Designs [optional]

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment