From f3fddf5f19fda64b4efb3ae8847379e61b16a0d1 Mon Sep 17 00:00:00 2001 From: Heinz-Peter Liechtenecker Date: Sat, 26 Sep 2020 13:32:54 +0200 Subject: [PATCH] Improving libcron performance (#9) Co-authored-by: Heinz-Peter Liechtenecker Co-authored-by: Per Malmberg --- README.md | 42 ++++++ libcron/CMakeLists.txt | 7 + libcron/include/libcron/Cron.h | 174 ++++++++++++------------ libcron/include/libcron/CronData.h | 2 + libcron/include/libcron/Task.h | 8 +- libcron/include/libcron/TaskQueue.h | 115 ++++++++++++++++ libcron/src/CronData.cpp | 13 +- test/CMakeLists.txt | 15 ++- test/CronTest.cpp | 197 ++++++++++++++++++++++++++++ 9 files changed, 481 insertions(+), 92 deletions(-) create mode 100644 libcron/include/libcron/TaskQueue.h diff --git a/README.md b/README.md index 7949218..7e05ba8 100644 --- a/README.md +++ b/README.md @@ -23,6 +23,8 @@ while(true) } ``` +In case there is a lot of time between you call `add_schedule` and `tick`, you can call `recalculate_schedule`. + The callback must have the following signature: ``` @@ -45,6 +47,46 @@ cron.add_schedule("Hello from Cron", "* * * * * ?", [=](auto& i) { }); ``` +- `libcron::TaskInformation::get_name` gives you the name of the current Task. This allows to add attach the same callback to multiple schedules: + +``` +libcron::Cron cron; + +auto f = [](auto& i) { + if (i.get_name() == "Task 1") + { + do_work_task_1(); + } + else if (i.get_name() == "Task 2") + { + do_work_task_2(); + } +}; + +cron.add_schedule("Task 1", "* * * * * ?", f); +cron.add_schedule("Task 2", "* * * * * ?", f); +``` + +## Adding multiple tasks with individual schedules at once + +libcron::cron::add_schedule needs to sort the underlying container each time you add a schedule. To improve performance when adding many tasks by only sorting once, there is a convinient way to pass either a `std::map`, a `std::vector>`, a `std::vector>` or a `std::unordered_map` to `add_schedule`, where the first element corresponds to the task name and the second element to the task schedule. Only if all schedules in the container are valid, they will be added to `libcron::Cron`. The return type is a `std::tuple`, where the boolean is `true` if the schedules have been added or false otherwise. If the schedules have not been added, the second element in the tuple corresponds to the task-name with the given invalid schedule. If there are multiple invalid schedules in the container, `add_schedule` will abort at the first invalid element: + +``` +std::map name_schedule_map; +for(int i = 1; i <= 1000; i++) +{ + name_schedule_map["Task-" + std::to_string(i)] = "* * * * * ?"; +} +name_schedule_map["Task-1000"] = "invalid"; +auto res = c1.add_schedule(name_schedule_map, [](auto&) { }); +if (std::get<0>(res) == false) +{ + std::cout << "Task " << std::get<1>(res) + << "has an invalid schedule: " + << std::get<2>(res) << std::endl; +} +``` + ## Removing schedules from `libcron::Cron` diff --git a/libcron/CMakeLists.txt b/libcron/CMakeLists.txt index 7a296b7..e026841 100644 --- a/libcron/CMakeLists.txt +++ b/libcron/CMakeLists.txt @@ -3,8 +3,15 @@ project(libcron) set(CMAKE_CXX_STANDARD 17) +# Deactivate Iterator-Debugging on Windows +option(LIBCRON_DEACTIVATE_ITERATOR_DEBUGGING "Build with iterator-debugging (MSVC only)." OFF) + if( MSVC ) set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4") + + if (LIBCRON_DEACTIVATE_ITERATOR_DEBUGGING) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_HAS_ITERATOR_DEBUGGING=0") + endif() else() set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic") endif() diff --git a/libcron/include/libcron/Cron.h b/libcron/include/libcron/Cron.h index bf6be70..7df8f5a 100644 --- a/libcron/include/libcron/Cron.h +++ b/libcron/include/libcron/Cron.h @@ -2,11 +2,14 @@ #include #include -#include #include #include +#include +#include +#include #include "Task.h" #include "CronClock.h" +#include "TaskQueue.h" namespace libcron { @@ -32,11 +35,16 @@ namespace libcron template std::ostream& operator<<(std::ostream& stream, const Cron& c); - template + template class Cron { public: bool add_schedule(std::string name, const std::string& schedule, Task::TaskFunction work); + + template> + std::tuple + add_schedule(const Schedules& name_schedule_map, Task::TaskFunction work); void clear_schedules(); void remove_schedule(const std::string& name); @@ -63,75 +71,23 @@ namespace libcron return clock; } + void recalculate_schedule() + { + for (auto& t : tasks.get_tasks()) + { + using namespace std::chrono_literals; + // Ensure that next schedule is in the future + t.calculate_next(clock.now() + 1s); + } + } + void get_time_until_expiry_for_tasks( std::vector>& status) const; friend std::ostream& operator<<<>(std::ostream& stream, const Cron& c); private: - class Queue - // Priority queue placing smallest (i.e. nearest in time) items on top. - : public std::priority_queue, std::greater<>> - { - public: - // Inherit to allow access to the container. - const std::vector& get_tasks() const - { - return c; - } - - std::vector& get_tasks() - { - return c; - } - - void clear() - { - lock.lock(); - - while (!empty()) - pop(); - - lock.unlock(); - } - - template - void remove(T to_remove) - { - lock.lock(); - - /* Copy current elements */ - std::vector temp{}; - std::swap(temp, c); - - /* Refill with elements ensuring correct order by calling push */ - for (const auto& task : temp) - { - if (task != to_remove) - push(task); - } - - lock.unlock(); - } - - void lock_queue() - { - /* Do not allow to manipulate the Queue */ - lock.lock(); - } - - void release_queue() - { - /* Allow Access to the Queue Manipulating-Functions */ - lock.unlock(); - } - - private: - LockType lock; - }; - - - Queue tasks{}; + TaskQueue tasks{}; ClockType clock{}; bool first_tick = true; std::chrono::system_clock::time_point last_tick{}; @@ -149,6 +105,7 @@ namespace libcron if (t.calculate_next(clock.now())) { tasks.push(t); + tasks.sort(); } tasks.release_queue(); } @@ -156,6 +113,50 @@ namespace libcron return res; } + template + template + std::tuple + Cron::add_schedule(const Schedules& name_schedule_map, Task::TaskFunction work) + { + bool is_valid = true; + std::tuple res{false, "", ""}; + + std::vector tasks_to_add; + tasks_to_add.reserve(name_schedule_map.size()); + + for (auto it = name_schedule_map.begin(); is_valid && it != name_schedule_map.end(); ++it) + { + const auto& [name, schedule] = *it; + auto cron = CronData::create(schedule); + is_valid = cron.is_valid(); + if (is_valid) + { + Task t{std::move(name), CronSchedule{cron}, work }; + if (t.calculate_next(clock.now())) + { + tasks_to_add.push_back(std::move(t)); + } + } + else + { + std::get<1>(res) = name; + std::get<2>(res) = schedule; + } + } + + // Only add tasks and sort once if all elements in the map where valid + if (is_valid && tasks_to_add.size() > 0) + { + tasks.lock_queue(); + tasks.push(tasks_to_add); + tasks.sort(); + tasks.release_queue(); + } + + std::get<0>(res) = is_valid; + return res; + } + template void Cron::clear_schedules() { @@ -241,30 +242,31 @@ namespace libcron last_tick = now; - std::vector executed{}; - - while (!tasks.empty() - && tasks.top().is_expired(now)) + if (!tasks.empty()) { - executed.push_back(tasks.top()); - tasks.pop(); - auto& t = executed[executed.size() - 1]; - t.execute(now); - } - - res = executed.size(); - - // Place executed tasks back onto the priority queue. - std::for_each(executed.begin(), executed.end(), [this, &now](Task& task) - { - // Must calculate new schedules using second after 'now', otherwise - // we'll run the same task over and over if it takes less than 1s to execute. - using namespace std::chrono_literals; - if (task.calculate_next(now + 1s)) + for (size_t i = 0; i < tasks.size(); i++) { - tasks.push(task); + if (tasks.at(i).is_expired(now)) + { + auto& t = tasks.at(i); + t.execute(now); + + using namespace std::chrono_literals; + if (!t.calculate_next(now + 1s)) + { + tasks.remove(t); + } + + res++; + } } - }); + + // Only sort if at least one task was executed + if (res > 0) + { + tasks.sort(); + } + } tasks.release_queue(); return res; diff --git a/libcron/include/libcron/CronData.h b/libcron/include/libcron/CronData.h index 6412988..1169456 100644 --- a/libcron/include/libcron/CronData.h +++ b/libcron/include/libcron/CronData.h @@ -4,6 +4,7 @@ #include #include #include +#include #include namespace libcron @@ -126,6 +127,7 @@ namespace libcron static const std::vector month_names; static const std::vector day_names; + static std::unordered_map cache; template void add_full_range(std::set& set); diff --git a/libcron/include/libcron/Task.h b/libcron/include/libcron/Task.h index a07d171..f30288c 100644 --- a/libcron/include/libcron/Task.h +++ b/libcron/include/libcron/Task.h @@ -13,6 +13,7 @@ namespace libcron public: virtual ~TaskInformation() = default; virtual std::chrono::system_clock::duration get_delay() const = 0; + virtual std::string get_name() const = 0; }; class Task : public TaskInformation @@ -50,12 +51,17 @@ namespace libcron return next_schedule > other.next_schedule; } + bool operator<(const Task& other) const + { + return next_schedule < other.next_schedule; + } + bool is_expired(std::chrono::system_clock::time_point now) const; std::chrono::system_clock::duration time_until_expiry(std::chrono::system_clock::time_point now) const; - std::string get_name() const + std::string get_name() const override { return name; } diff --git a/libcron/include/libcron/TaskQueue.h b/libcron/include/libcron/TaskQueue.h new file mode 100644 index 0000000..2650bc0 --- /dev/null +++ b/libcron/include/libcron/TaskQueue.h @@ -0,0 +1,115 @@ +#pragma once + +#include +#include +#include +#include +#include "Task.h" + +namespace libcron +{ + template + class TaskQueue + { + public: + const std::vector& get_tasks() const + { + return c; + } + + std::vector& get_tasks() + { + return c; + } + + size_t size() const noexcept + { + return c.size(); + } + + bool empty() const noexcept + { + return c.empty(); + } + + void push(Task& t) + { + c.push_back(std::move(t)); + } + + void push(Task&& t) + { + c.push_back(std::move(t)); + } + + void push(std::vector& tasks_to_insert) + { + c.reserve(c.size() + tasks_to_insert.size()); + c.insert(c.end(), std::make_move_iterator(tasks_to_insert.begin()), std::make_move_iterator(tasks_to_insert.end())); + } + + const Task& top() const + { + return c[0]; + } + + Task& at(const size_t i) + { + return c[i]; + } + + void sort() + { + std::sort(c.begin(), c.end(), std::less<>()); + } + + void clear() + { + lock.lock(); + c.clear(); + lock.unlock(); + } + + void remove(Task& to_remove) + { + auto it = std::find_if(c.begin(), c.end(), [&to_remove] (const Task& to_compare) { + return to_remove.get_name() == to_compare; + }); + + if (it != c.end()) + { + c.erase(it); + } + } + + void remove(std::string to_remove) + { + lock.lock(); + auto it = std::find_if(c.begin(), c.end(), [&to_remove] (const Task& to_compare) { + return to_remove == to_compare; + }); + if (it != c.end()) + { + c.erase(it); + } + + lock.unlock(); + } + + void lock_queue() + { + /* Do not allow to manipulate the Queue */ + lock.lock(); + } + + void release_queue() + { + /* Allow Access to the Queue Manipulating-Functions */ + lock.unlock(); + } + + private: + LockType lock; + std::vector c; + }; +} diff --git a/libcron/src/CronData.cpp b/libcron/src/CronData.cpp index e28896b..3122210 100644 --- a/libcron/src/CronData.cpp +++ b/libcron/src/CronData.cpp @@ -15,12 +15,23 @@ namespace libcron const std::vector CronData::month_names{ "JAN", "FEB", "MAR", "APR", "MAY", "JUN", "JUL", "AUG", "SEP", "OCT", "NOV", "DEC" }; const std::vector CronData::day_names{ "SUN", "MON", "TUE", "WED", "THU", "FRI", "SAT" }; + std::unordered_map CronData::cache{}; CronData CronData::create(const std::string& cron_expression) { CronData c; - c.parse(cron_expression); + auto found = cache.find(cron_expression); + if (found == cache.end()) + { + c.parse(cron_expression); + cache[cron_expression] = c; + } + else + { + c = found->second; + } + return c; } diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 25e7727..cca1396 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -3,8 +3,15 @@ project(cron_test) set(CMAKE_CXX_STANDARD 17) +# Deactivate Iterator-Debugging on Windows +option(LIBCRON_DEACTIVATE_ITERATOR_DEBUGGING "Build with iterator-debugging (MSVC only)." OFF) + if( MSVC ) - set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4") + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} /W4") + + if (LIBCRON_DEACTIVATE_ITERATOR_DEBUGGING) + set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -D_HAS_ITERATOR_DEBUGGING=0") + endif() else() set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic") endif() @@ -19,12 +26,12 @@ add_executable( ${PROJECT_NAME} CronDataTest.cpp CronRandomizationTest.cpp - CronScheduleTest.cpp - CronTest.cpp) + CronScheduleTest.cpp + CronTest.cpp) target_link_libraries(${PROJECT_NAME} libcron) set_target_properties(${PROJECT_NAME} PROPERTIES ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/out" LIBRARY_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/out" - RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/out") \ No newline at end of file + RUNTIME_OUTPUT_DIRECTORY "${CMAKE_CURRENT_LIST_DIR}/out") diff --git a/test/CronTest.cpp b/test/CronTest.cpp index 7de24e4..6fed1b8 100644 --- a/test/CronTest.cpp +++ b/test/CronTest.cpp @@ -1,5 +1,6 @@ #include #include +#include #include #include @@ -209,6 +210,7 @@ SCENARIO("Task priority") } AND_WHEN("Waiting based on the time given by the Cron instance") { + auto msec = std::chrono::duration_cast(c.time_until_next()); std::this_thread::sleep_for(c.time_until_next()); c.tick(); @@ -446,3 +448,198 @@ SCENARIO("Tasks can be added and removed from the scheduler") } } } + +SCENARIO("Testing CRON-Tick Performance") +{ + GIVEN("A Cron instance with no task") + { + using namespace std::chrono_literals; + using clock = std::chrono::high_resolution_clock; + using std::chrono::milliseconds; + using std::chrono::duration_cast; + + Cron c1{}; + auto& cron_clock1 = c1.get_clock(); + + Cron c2{}; + auto& cron_clock2 = c2.get_clock(); + + Cron c3{}; + auto& cron_clock3 = c3.get_clock(); + + int count1 = 0; + int count2 = 0; + int count3 = 0; + + WHEN("Creating 1000 CronData Objects") + { + std::string cron_job = "* * * * * ?"; + auto begin_cron_data = clock::now(); + for(int i = 1; i <= 1000; i++) + { + auto cron = CronData::create(cron_job); + } + auto end_cron_data = clock::now(); + auto msec_cron_data = duration_cast(end_cron_data - begin_cron_data); + + // Hopefully Creating a lot of Cron Objects does not take more than a second + REQUIRE(msec_cron_data <= 1000ms); + } + WHEN("Adding 1000 Tasks where with an invalid CRON-String with std::map") + { + std::map name_schedule_map; + for(int i = 1; i <= 1000; i++) + { + name_schedule_map["Task-" + std::to_string(i)] = "* * * * * ?"; + } + name_schedule_map["Task-1000"] = "invalid"; + + auto res = c1.add_schedule(name_schedule_map, + [](auto&) { }); + + REQUIRE_FALSE(std::get<0>(res)); + REQUIRE(std::get<1>(res) == "Task-1000"); + REQUIRE(std::get<2>(res) == "invalid"); + } + WHEN("Adding a std::vector>") + { + std::vector> name_schedule_map; + for(int i = 1; i <= 1000; i++) + { + name_schedule_map.push_back(std::make_tuple("Task-" + std::to_string(i), "* * * * * ?")); + } + + auto res = c1.add_schedule(name_schedule_map, + [](auto&) { }); + + REQUIRE(std::get<0>(res)); + } + WHEN("Adding a std::vector>") + { + std::vector> name_schedule_map; + for(int i = 1; i <= 1000; i++) + { + name_schedule_map.push_back(std::make_pair("Task-" + std::to_string(i), "* * * * * ?")); + } + + auto res = c1.add_schedule(name_schedule_map, + [](auto&) { }); + + REQUIRE(std::get<0>(res)); + } + WHEN("Adding a std::unordered_map") + { + std::unordered_map name_schedule_map; + for(int i = 1; i <= 1000; i++) + { + name_schedule_map["Task-" + std::to_string(i)] = "* * * * * ?"; + } + auto res = c1.add_schedule(name_schedule_map, + [](auto&) { }); + + REQUIRE(std::get<0>(res)); + } + WHEN("Adding 1000 Tasks to two Cron-Objects expiring after 1 second calling add_schedule") + { + auto begin_add_sequential = clock::now(); + for(int i = 1; i <= 1000; i++) + { + REQUIRE(c1.add_schedule("Task-" + std::to_string(i), "* * * * * ?", + [&count1](auto&) + { + count1++; + }) + ); + REQUIRE(c2.add_schedule("Task-" + std::to_string(i), "* * * * * ?", + [&count2](auto&) + { + count2++; + }) + ); + } + auto end_add_sequential = clock::now(); + + std::map name_schedule_map{}; + for(int i = 1; i <= 1000; i++) + { + name_schedule_map["Task-" + std::to_string(i)] = "* * * * * ?"; + } + + auto begin_add_batch = clock::now(); + REQUIRE(std::get<0>(c3.add_schedule(name_schedule_map, + [&count3](auto&) + { + count3++; + })) + ); + auto end_add_batch = clock::now(); + + + auto time_sequential = duration_cast(end_add_sequential - begin_add_sequential)/2; + auto time_batch = duration_cast(end_add_batch - begin_add_batch); + + // This should hopefully take only a few second? + REQUIRE(time_sequential < 10000ms); + REQUIRE(time_batch < 5000ms); + REQUIRE(time_batch < time_sequential); + REQUIRE(c1.count() == 1000); + REQUIRE(c2.count() == 1000); + REQUIRE(c3.count() == 1000); + + THEN("Execute all Tasks 10 Times") + { + milliseconds msec1; + auto t1 = std::thread([&cron_clock1, &c1, &msec1]() { + auto begin_tick = clock::now(); + for(auto i = 0; i < 10; ++i) + { + cron_clock1.add(std::chrono::seconds{1}); + c1.tick(); + } + + auto end_tick = clock::now(); + msec1 = duration_cast(end_tick - begin_tick)/10; + }); + + milliseconds msec2; + auto t2 = std::thread([&cron_clock2, &c2, &msec2]() { + auto begin_tick = clock::now(); + for(auto i = 0; i < 10; ++i) + { + cron_clock2.add(std::chrono::seconds{1}); + c2.tick(); + } + + auto end_tick = clock::now(); + msec2 = duration_cast(end_tick - begin_tick)/10; + }); + + milliseconds msec3; + auto t3 = std::thread([&cron_clock3, &c3, &msec3]() { + auto begin_tick = clock::now(); + for(auto i = 0; i < 10; ++i) + { + cron_clock3.add(std::chrono::seconds{1}); + c3.tick(); + } + + auto end_tick = clock::now(); + msec3 = duration_cast(end_tick - begin_tick)/10; + }); + + t1.join(); + t2.join(); + t3.join(); + + REQUIRE(count1 == 10000); + REQUIRE(count2 == 10000); + REQUIRE(count3 == 10000); + + // Hopefully executing a more or less empty task does only take some milliseconds + REQUIRE(msec1 <= 10ms); + REQUIRE(msec2 <= 10ms); + REQUIRE(msec3 <= 10ms); + } + } + } +}