From d3bc8b552c9bd5a0a88bdc40b71e09ec8c149cb7 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Niccol=C3=B2=20Betto?= <niccolo.betto@skywarder.eu>
Date: Wed, 28 Dec 2022 16:28:24 +0100
Subject: [PATCH] [TaskScheduler] Store tasks in a preallocated `std::vector`

The tasks vector is resized to MAX_TASKS (256 at the time of writing
this) immediately after construction to avoid reallocations.
---
 src/shared/scheduler/TaskScheduler.cpp | 58 ++++++++++----------------
 src/shared/scheduler/TaskScheduler.h   | 19 ++++-----
 2 files changed, 30 insertions(+), 47 deletions(-)

diff --git a/src/shared/scheduler/TaskScheduler.cpp b/src/shared/scheduler/TaskScheduler.cpp
index 5b41ac35c..44f5f363d 100644
--- a/src/shared/scheduler/TaskScheduler.cpp
+++ b/src/shared/scheduler/TaskScheduler.cpp
@@ -40,47 +40,33 @@ static constexpr unsigned int MS_PER_TICK =
 }  // namespace Constants
 
 TaskScheduler::TaskScheduler(miosix::Priority priority)
-    : ActiveObject(STACK_MIN_FOR_SKYWARD, priority)
+    : ActiveObject(STACK_MIN_FOR_SKYWARD, priority), tasks()
 {
-    // Create dynamically the tasks vector because of too much space
-    tasks = new std::array<Task, TASKS_SIZE>();
+    // Preallocate the vector to avoid dynamic allocation later on
+    tasks.reserve(MAX_TASKS);
 
-    // Initialize the vector elements
-    for (size_t i = 1; i < TASKS_SIZE; i++)
-    {
-        (*tasks)[i] = Task();
-    }
+    // Reserve the zeroth element so that the task ID is never zero, for
+    // API compatibility: returned value from addTask() is zero on error.
+    tasks.emplace_back();
 }
 
-TaskScheduler::~TaskScheduler() { delete tasks; }
-
 size_t TaskScheduler::addTask(function_t function, uint32_t period,
                               Policy policy, int64_t startTick)
 {
     Lock<FastMutex> lock(mutex);
-    size_t id = 1;
-
-    // Find a suitable id for the new task
-    for (; id < TASKS_SIZE; id++)
-    {
-        if ((*tasks)[id].empty())
-        {
-            break;
-        }
-    }
 
-    // Check if in the corresponding id there's already a task
-    if ((*tasks)[id].enabled)
+    if (tasks.size() >= MAX_TASKS)
     {
-        // Unlock the mutex for expensive operation
+        // Unlock the mutex to release the scheduler resources before logging
         Unlock<FastMutex> unlock(mutex);
 
-        LOG_ERR(logger, "Full task scheduler, id = {:zu}", id);
+        LOG_ERR(logger, "Full task scheduler");
         return 0;
     }
 
-    // Create a new task with the given parameters
-    (*tasks)[id] = Task(function, period, policy);
+    // Insert a new task with the given parameters
+    tasks.emplace_back(function, period, policy);
+    size_t id = tasks.size() - 1;
 
     if (policy == Policy::ONE_SHOT)
     {
@@ -96,14 +82,14 @@ size_t TaskScheduler::addTask(function_t function, uint32_t period,
 
 void TaskScheduler::enableTask(size_t id)
 {
-    if (id > TASKS_SIZE)
+    if (id > tasks.size() - 1)
     {
         LOG_ERR(logger, "Tried to enable an out-of-range task, id = {}", id);
         return;
     }
 
     Lock<FastMutex> lock(mutex);
-    Task& task = (*tasks)[id];
+    Task& task = tasks[id];
 
     // Check that the task function is not empty
     // Attempting to run an empty function will throw a bad_function_call
@@ -122,14 +108,14 @@ void TaskScheduler::enableTask(size_t id)
 
 void TaskScheduler::disableTask(size_t id)
 {
-    if (id > TASKS_SIZE)
+    if (id > tasks.size() - 1)
     {
         LOG_ERR(logger, "Tried to disable an out-of-range task, id = {}", id);
         return;
     }
 
     Lock<FastMutex> lock(mutex);
-    (*tasks)[id].enabled = false;
+    tasks[id].enabled = false;
 }
 
 bool TaskScheduler::start()
@@ -161,9 +147,9 @@ vector<TaskStatsResult> TaskScheduler::getTaskStats()
 
     vector<TaskStatsResult> result;
 
-    for (size_t id = 1; id < TASKS_SIZE; id++)
+    for (size_t id = 1; id < tasks.size(); id++)
     {
-        const Task& task = (*tasks)[id];
+        const Task& task = tasks[id];
         if (task.enabled)
         {
             result.push_back(fromTaskIdPairToStatsResult(task, id));
@@ -185,7 +171,7 @@ void TaskScheduler::normalizeTasks()
 
         if (event.nextTick < currentTick)
         {
-            Task& task = (*tasks)[event.taskId];
+            Task& task = tasks[event.taskId];
             event.nextTick +=
                 ((currentTick - event.nextTick) / task.period + 1) *
                 task.period;
@@ -213,7 +199,7 @@ void TaskScheduler::run()
 
         int64_t startTick = getTick();
         Event nextEvent   = agenda.top();
-        Task& nextTask    = (*tasks)[nextEvent.taskId];
+        Task& nextTask    = tasks[nextEvent.taskId];
 
         // If the task has the SKIP policy and its execution was missed, we need
         // to move it forward to match the period
@@ -260,7 +246,7 @@ void TaskScheduler::run()
 void TaskScheduler::updateStats(const Event& event, int64_t startTick,
                                 int64_t endTick)
 {
-    Task& task = (*tasks)[event.taskId];
+    Task& task = tasks[event.taskId];
 
     // Activation stats
     float activationError = startTick - event.nextTick;
@@ -281,7 +267,7 @@ void TaskScheduler::updateStats(const Event& event, int64_t startTick,
 
 void TaskScheduler::enqueue(Event event, int64_t startTick)
 {
-    Task& task = (*tasks)[event.taskId];
+    Task& task = tasks[event.taskId];
     switch (task.policy)
     {
         case Policy::ONE_SHOT:
diff --git a/src/shared/scheduler/TaskScheduler.h b/src/shared/scheduler/TaskScheduler.h
index b4a275939..b7e83e496 100644
--- a/src/shared/scheduler/TaskScheduler.h
+++ b/src/shared/scheduler/TaskScheduler.h
@@ -63,9 +63,9 @@ public:
     using function_t = std::function<void()>;
 
     /**
-     * @brief It defines the tasks array maximum size
+     * @brief The maximum number of tasks the scheduler can handle.
      */
-    static constexpr size_t TASKS_SIZE = 256;
+    static constexpr size_t MAX_TASKS = 256;
 
     /**
      * @brief Task behavior policy.
@@ -98,14 +98,10 @@ public:
     explicit TaskScheduler(miosix::Priority priority = miosix::PRIORITY_MAX -
                                                        1);
 
-    ~TaskScheduler();
-
     /**
-     * @brief Add a task function to the scheduler with an auto generated id.
+     * @brief Add a task function to the scheduler with an auto generated ID.
      *
      * Note that each task has it's own unique ID, even one shot tasks!
-     * Therefore, if a task already exists with the same id, the function will
-     * fail and return false.
      *
      * For one shot tasks, the period is used as a delay. If 0 the task will be
      * executed immediately, otherwise after the given period.
@@ -114,7 +110,7 @@ public:
      * @param period Inter call period [ms].
      * @param policy Task policy, default is SKIP.
      * @param startTick First activation time, useful for synchronizing tasks.
-     * @return true if the task was added successfully.
+     * @return The ID of the task if it was added successfully, 0 otherwise.
      */
     size_t addTask(function_t function, uint32_t period,
                    Policy policy     = Policy::SKIP,
@@ -252,9 +248,10 @@ private:
     }
 
     miosix::FastMutex mutex;  ///< Mutex to protect tasks and agenda.
-    std::array<Task, TASKS_SIZE>* tasks;  ///< Holds all tasks to be scheduled.
-    miosix::ConditionVariable condvar;    ///< Used when agenda is empty.
-    EventQueue agenda;                    ///< Ordered list of functions.
+    std::vector<Task> tasks;  ///< Holds all tasks to be scheduled, preallocated
+                              ///< to MAX_TASKS avoid dynamic allocations.
+    miosix::ConditionVariable condvar;  ///< Used when agenda is empty.
+    EventQueue agenda;                  ///< Ordered list of functions.
 
     PrintLogger logger = Logging::getLogger("taskscheduler");
 };
-- 
GitLab