Remove executor not run option

Reviewers: teon.banek

Reviewed By: teon.banek

Subscribers: pullbot

Differential Revision: https://phabricator.memgraph.io/D1062

config/benchmarking.conf

@@ -10,7 +10,7 @@
 
 # no GC
 --gc-cycle-sec=-1
---skiplist_gc_interval=0
+--skiplist_gc_interval=-1
 
 # no query execution time limit
 --query_execution_time_sec=-1

src/data_structures/concurrent/skiplist_gc.cpp

@@ -1,6 +1,10 @@
 #include "skiplist_gc.hpp"
-#include "gflags/gflags.h"
 
-DEFINE_HIDDEN_int32(skiplist_gc_interval, 10,
-                    "Interval of how often does skiplist gc run in seconds. To "
-                    "disable set to 0.");
+#include <iostream>
+#include "utils/flag_validation.hpp"
+
+DEFINE_VALIDATED_HIDDEN_int32(
+    skiplist_gc_interval, 10,
+    "Interval of how often does skiplist gc run in seconds. To "
+    "disable set to -1.",
+    FLAG_IN_RANGE(-1, std::numeric_limits<int32_t>::max()));

src/data_structures/concurrent/skiplist_gc.hpp

@@ -2,6 +2,7 @@
 
 #include <malloc.h>
 
+#include <experimental/optional>
 #include <list>
 #include <mutex>
 #include <utility>
@@ -31,14 +32,16 @@ template <class TNode>
 class SkipListGC {
  public:
   explicit SkipListGC() {
-    executor_job_id_ =
-        GetExecutor().RegisterJob([this]() { GarbageCollect(); });
+    if (GetExecutor()) {
+      executor_job_id_ =
+          GetExecutor()->RegisterJob([this]() { GarbageCollect(); });
+    }
   }
 
   ~SkipListGC() {
     // We have to unregister the job because otherwise Executor might access
     // some member variables of this class after it has been destructed.
-    GetExecutor().UnRegisterJob(executor_job_id_);
+    if (GetExecutor()) GetExecutor()->UnRegisterJob(executor_job_id_);
     for (auto it = deleted_queue_.begin(); it != deleted_queue_.end(); ++it) {
       TNode::destroy(it->second);
     }
@@ -55,8 +58,7 @@ class SkipListGC {
     // because of their order of destruction. For example of one bug take a look
     // at documentation in database/dbms.hpp. Rethink ownership and lifetime of
     // executor.
-    static Executor executor(
-        (std::chrono::seconds(FLAGS_skiplist_gc_interval)));
+    static auto executor = ConstructExecutor();
 
     return executor;
   }
@@ -155,9 +157,19 @@ class SkipListGC {
   }
 
  private:
+  /// Constructs executor depending on flag - has to be done this way because of
+  /// C++14
+  auto ConstructExecutor() {
+    std::unique_ptr<Executor> executor;
+    if (FLAGS_skiplist_gc_interval != -1) {
+      executor = std::make_unique<Executor>(
+          std::chrono::seconds(FLAGS_skiplist_gc_interval));
+    }
+    return executor;
+  }
+
   int64_t executor_job_id_{-1};
   std::mutex mutex_;
-  std::mutex singleton_mutex_;
 
   // List of accesssors from begin to end by an increasing id.
   std::list<AccessorStatus> accessors_;

src/utils/executor.hpp

@@ -14,8 +14,9 @@ class Executor {
  public:
   template <typename TRep, typename TPeriod>
   explicit Executor(const std::chrono::duration<TRep, TPeriod> pause) {
-    if (pause != pause.zero())
-      scheduler_.Run(pause, std::bind(&Executor::Execute, this));
+    DCHECK(pause > std::chrono::seconds(0))
+        << "Duration between executions should be reasonable";
+    scheduler_.Run(pause, std::bind(&Executor::Execute, this));
   }
 
   ~Executor() {
@@ -25,6 +26,11 @@ class Executor {
     scheduler_.Stop();
   }
 
+  Executor(Executor &&e) = default;
+  Executor &operator=(Executor &&) = default;
+  Executor(const Executor &e) = delete;
+  Executor &operator=(const Executor &) = delete;
+
   /**
    * @brief - Add function to job queue.
    */

tests/unit/executor.cpp

@@ -5,21 +5,6 @@
 
 #include "utils/executor.hpp"
 
-TEST(Executor, DontRun) {
-  std::atomic<int> count{0};
-  {
-    Executor exec(std::chrono::seconds(0));
-    // Be sure executor is sleeping.
-    std::this_thread::sleep_for(std::chrono::milliseconds(100));
-
-    exec.RegisterJob([&count]() { ++count; });
-    // Try to wait to test if executor might somehow become awake and execute
-    // the job.
-    std::this_thread::sleep_for(std::chrono::milliseconds(500));
-  }
-  EXPECT_EQ(count, 0);
-}
-
 TEST(Executor, Run) {
   std::atomic<int> count{0};
   {

tests/unit/skiplist_gc.cpp

@@ -33,7 +33,7 @@ class FakeItem {
 DECLARE_int32(skiplist_gc_interval);
 
 TEST(SkipListGC, CreateNewAccessors) {
-  FLAGS_skiplist_gc_interval = 0;
+  FLAGS_skiplist_gc_interval = -1;
   SkipListGC<FakeItem> gc;
   auto &accessor1 = gc.CreateNewAccessor();
   auto &accessor2 = gc.CreateNewAccessor();
@@ -49,7 +49,7 @@ TEST(SkipListGC, CreateNewAccessors) {
 }
 
 TEST(SkipListGC, DeleteItem) {
-  FLAGS_skiplist_gc_interval = 0;
+  FLAGS_skiplist_gc_interval = -1;
   SkipListGC<FakeItem> gc;
   auto &accessor1 = gc.CreateNewAccessor();
   std::atomic<int> count{0};
@@ -63,7 +63,7 @@ TEST(SkipListGC, DeleteItem) {
 }
 
 TEST(SkipListGC, DontDeleteItem) {
-  FLAGS_skiplist_gc_interval = 0;
+  FLAGS_skiplist_gc_interval = -1;
   SkipListGC<FakeItem> gc;
   auto &accessor1 = gc.CreateNewAccessor();
   auto &accessor2 = gc.CreateNewAccessor();
@@ -85,7 +85,7 @@ TEST(SkipListGC, DontDeleteItem) {
 }
 
 TEST(SkipListGC, Destructor) {
-  FLAGS_skiplist_gc_interval = 0;
+  FLAGS_skiplist_gc_interval = -1;
   std::atomic<int> count{0};
   auto item1 = new FakeItem(count, 1);
   {
@@ -97,7 +97,7 @@ TEST(SkipListGC, Destructor) {
 }
 
 TEST(SkipListGC, MultipleDeletes) {
-  FLAGS_skiplist_gc_interval = 0;
+  FLAGS_skiplist_gc_interval = -1;
   SkipListGC<FakeItem> gc;
   std::atomic<int> count{0};
   auto &accessor1 = gc.CreateNewAccessor();