From 0eb30db8dba3e1892a19ee0f0a10c870004d6a56 Mon Sep 17 00:00:00 2001 From: Dominik Gleich Date: Mon, 18 Dec 2017 10:20:35 +0100 Subject: [PATCH] Remove executor not run option Reviewers: teon.banek Reviewed By: teon.banek Subscribers: pullbot Differential Revision: https://phabricator.memgraph.io/D1062 --- config/benchmarking.conf | 2 +- .../concurrent/skiplist_gc.cpp | 12 ++++++---- .../concurrent/skiplist_gc.hpp | 24 ++++++++++++++----- src/utils/executor.hpp | 10 ++++++-- tests/unit/executor.cpp | 15 ------------ tests/unit/skiplist_gc.cpp | 10 ++++---- 6 files changed, 40 insertions(+), 33 deletions(-) diff --git a/config/benchmarking.conf b/config/benchmarking.conf index ed38ee427..538cb243e 100644 --- a/config/benchmarking.conf +++ b/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 diff --git a/src/data_structures/concurrent/skiplist_gc.cpp b/src/data_structures/concurrent/skiplist_gc.cpp index 5a443983e..6af50d6ba 100644 --- a/src/data_structures/concurrent/skiplist_gc.cpp +++ b/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 +#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::max())); diff --git a/src/data_structures/concurrent/skiplist_gc.hpp b/src/data_structures/concurrent/skiplist_gc.hpp index 16cedde92..f6c547d21 100644 --- a/src/data_structures/concurrent/skiplist_gc.hpp +++ b/src/data_structures/concurrent/skiplist_gc.hpp @@ -2,6 +2,7 @@ #include +#include #include #include #include @@ -31,14 +32,16 @@ template 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; + if (FLAGS_skiplist_gc_interval != -1) { + executor = std::make_unique( + 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 accessors_; diff --git a/src/utils/executor.hpp b/src/utils/executor.hpp index 1f4318350..ee61edbce 100644 --- a/src/utils/executor.hpp +++ b/src/utils/executor.hpp @@ -14,8 +14,9 @@ class Executor { public: template explicit Executor(const std::chrono::duration 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. */ diff --git a/tests/unit/executor.cpp b/tests/unit/executor.cpp index 92e19b1c8..12fb1ff73 100644 --- a/tests/unit/executor.cpp +++ b/tests/unit/executor.cpp @@ -5,21 +5,6 @@ #include "utils/executor.hpp" -TEST(Executor, DontRun) { - std::atomic 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 count{0}; { diff --git a/tests/unit/skiplist_gc.cpp b/tests/unit/skiplist_gc.cpp index 5c7924160..fba78cbb6 100644 --- a/tests/unit/skiplist_gc.cpp +++ b/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 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 gc; auto &accessor1 = gc.CreateNewAccessor(); std::atomic count{0}; @@ -63,7 +63,7 @@ TEST(SkipListGC, DeleteItem) { } TEST(SkipListGC, DontDeleteItem) { - FLAGS_skiplist_gc_interval = 0; + FLAGS_skiplist_gc_interval = -1; SkipListGC 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 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 gc; std::atomic count{0}; auto &accessor1 = gc.CreateNewAccessor();