diff --git a/emper/sleep_strategy/SemaphoreWorkerSleepStrategy.hpp b/emper/sleep_strategy/SemaphoreWorkerSleepStrategy.hpp index 0d2f07ac1cd5b2d3649dc0458ad9557d1adb76bd..40b4b11ca4ffb8822b3eeba55ae3353f3cfe6a4c 100644 --- a/emper/sleep_strategy/SemaphoreWorkerSleepStrategy.hpp +++ b/emper/sleep_strategy/SemaphoreWorkerSleepStrategy.hpp @@ -133,32 +133,37 @@ class AbstractSemaphoreWorkerSleepStrategy template <CallerEnvironment callerEnvironment> [[nodiscard]] inline auto mustNotify() -> bool { - typename Sem::CounterType skipWakeupThreshold; - if constexpr (callerEnvironment == CallerEnvironment::ANYWHERE) { - // On external work we always increment the semaphore unless we observe - // that its value is > workerCount. - // If we observe semValue > workerCount we are ensured that some worker will iterate - // its dispatchLoop again and must observe the new work. - // TODO: Could it be >= workerCount ? - skipWakeupThreshold = workerCount; - } else { - // For work from within emper we skip wakeup if we observe no one sleeping. - // This is sound because wakeupSleepingWorkers() is called from a active - // worker which will observe its own new work in its next dispatchLoop before - // going to sleep. - - // Note that sem_getvalue() is allowed to return 0 if there are - // waiting workers, hence we need to set the threshold also to - // 0. This has the disadvantage that we will perform one - // unnecessary sem_post. If we are sure the wakeupSem - // implementation does not return 0 with waiters, - // then the skipWakeupThreshold value should be - // reviewed and potentially changed to '-1'. - skipWakeupThreshold = 0; - } + /* On external work we always increment the semaphore unless we observe + * that its value is > workerCount. + * If we observe semValue > workerCount we are ensured that some worker will iterate + * its dispatchLoop again and must observe the new work. + + * For work from within emper we could skip wakeup if we observe no one sleeping. + * This is sound because wakeupSleepingWorkers() is called from a active + * worker which will observe its own new work in its next dispatchLoop before + * going to sleep. + * BUT this supposed not harmful race may not be harmful for guarantying + * progress but it is harmfull for latency. + * Inserting new work in the system races with workers about to sleep + * actually able to execute the work in a timely manner. + + * The overhead of increasing the semaphore "to" much are unnessesary atomic + * operations on global state. This overhead was not measured in contrast to + * the harm caused to latency by beeing greedy when using the semaphore. + + * Note that sem_getvalue() is allowed to return 0 if there are + * waiting workers, hence we need to set the threshold atleast to + * 0. This has the disadvantage that we will perform one + * unnecessary sem_post. If we are sure the wakeupSem + * implementation does not return 0 with waiters, + * then the skipWakeupThreshold value could be + * reviewed and potentially changed to '-1'. + * skipWakeupThreshold = 0; + */ + const typename Sem::CounterType skipWakeupThreshold = workerCount; auto semValue = wakeupSem.getValue(); - return !(semValue > skipWakeupThreshold); + return semValue <= skipWakeupThreshold; } public: