Linux C++ Timer Class: How can I improve the accuracy?

Question

I wrote this class today, but I am trying to figure out how to make it more accurate. I pass in seconds and multiply by 1000 to make it milliseconds, and the time does not line up. I need the ability to have multiple timers, so using timer_gettime(), which limits one clock per process, is not an option. Also note I borrowed my idea from free glut.

Please feel free to leave general comments on style, but please focus more on how to improve the accuracy of the timer.

#include "TimerManager.h"
#include <iostream>

void func1(int id)
{
  std::cout << "I was called with " << std::endl;
}

void func2(int id)
{
  std::cout << "I was called too ))))))))))))))))))))))))))))))))))))))))))))))))))))))))))" << std::endl;
}

int main(int argc, char *argv[])
{
  TimerManager t;
  t.addTimer(6, func1);
  t.addTimer(2, func2);
  while(true) {
    t.start();
  }
  return 0;
}

---

#ifndef TIMERMANAGER_H_
#define TIMERMANAGER_H_

#include <stdlib.h>
#include <iostream>
#include <pthread.h>
#include <list>

extern "C" {
  void *create_pthread(void *data);
}

class TimerManager {
public:
  TimerManager();
  ~TimerManager();
  void start();
  void stop();
  void addTimer(long usec, void (*callback)(int id));
private:
  class Timer  
  {
  public:
    Timer(long usec, void (*callback)(int)) :
      duration(usec),
      callback(callback),
      start(0)
    {    
    }   
    bool operator ==(Timer other)
    {   
      if ((this->callback == other.callback) && (this->duration == other.duration)) {
        return true;
      }   
      return false;
    }   
    long duration;
    void (*callback)(int);
    long start;
  };  
  std::list<Timer> m_timers;
  pthread_t m_timer_thread;
  Timer setUpTimer(long usec, void (*callback)(int id));
  friend void *create_pthread(void *data);
  void run();
  bool go; 
};

#endif

---

#include <algorithm>
#include <iterator>
#include <sys/time.h>
#include "TimerManager.h"

extern "C" void *create_pthread(void *data)
{
  TimerManager *thread_timer_manager = static_cast<TimerManager *>(data);
  thread_timer_manager->run();
  return data;
}

TimerManager::TimerManager() :
    go(false)
{
  int thread_creation = pthread_create(&m_timer_thread, NULL, create_pthread, this);
  if(thread_creation != 0) {
    std::cerr << "Failed to create thread" << std::endl;
    return;
  } else {
    std::cout << "The status flag is " << thread_creation << std::endl;
  }
}

TimerManager::~TimerManager() 
{
}

void TimerManager::run() 
{
  struct timeval l_tv;
  while(true) {
    if (go) {
      typedef std::list<Timer>::iterator li; 
      li begin = m_timers.begin();
      li end = m_timers.end();
      int item = 0;
      for(;begin != end; ++begin) {
        gettimeofday(&l_tv, NULL);
        long elapsed_time = (l_tv.tv_usec - begin->start);
        sleep(1);
        if (elapsed_time >= begin->duration) {
          //std::cout << "Status of timer " << item << " is " << elapsed_time << " > " << begin->duration << std::endl;
          begin->callback(item);
          gettimeofday(&l_tv, NULL);
          begin->start = l_tv.tv_usec;
        } else {
          //std::cout << "Status of timer " << item << " is " << elapsed_time << " < " << begin->duration << std::endl;
        }   
        item++;
      }   
    }   
  }
}

void TimerManager::start()
{
  go = true;
}

oid TimerManager::stop() 
{
  go = false;
}

TimerManager::Timer TimerManager::setUpTimer(long usec, void (*callback)(int id))
{
  struct timeval l_tv;
  gettimeofday(&l_tv, NULL);
  Timer l_t(usec * 100, callback);
  l_t.start = l_tv.tv_usec;
  std::cout << "Timer created with a values of START: " << l_t.start << " CALLBACK: " << &l_t.callback << " DURATION: " << l_t.duration << std::endl;
  return l_t;
}

void TimerManager::addTimer(long usec, void (*callback)(int id))
{
  Timer insert = setUpTimer(usec, callback);
  typedef std::list<Timer>::iterator li;
  li begin = m_timers.begin();
  li end = m_timers.end();
  for(;begin != end; ++begin) {
    if (*begin == *end) {
      return;
    }
  }
  m_timers.push_back(insert);
}

---

Update: Changes after reading answers.

Here is the updated code,

#include "TimerManager.h"
#include <iostream>
#include <fstream>
#include <sys/time.h>

extern "C"
void func1(int id)
{
  struct timeval l_tv;
  gettimeofday(&l_tv, NULL);
  std::cout << "I was called (1) @ " << l_tv.tv_usec << std::endl;
}

extern "C"
void func2(int id)
{
  struct timeval l_tv;
  gettimeofday(&l_tv, NULL);
  std::cout << "I was called (2) @ " << l_tv.tv_usec << std::endl;
}

int main(int, char *[])
{
  TimerManager t;
  t.addTimer(1000000 / 2, func1);
  t.addTimer(1000000 * 4, func2);
  t.start();
  while(true) {
    sleep(1);
  }
  return 0;
}

---

#ifndef TIMERMANAGER_H_
#define TIMERMANAGER_H_

#include <stdlib.h>
#include <iostream>
#include <pthread.h>
#include <list>

extern "C" {
  void *create_pthread(void *data);
}

class TimerManager {
public:
  TimerManager();
  ~TimerManager();
  void start();
  void stop();
  void addTimer(long usec, void (*callback)(int id));
private:
  class Timer  
  {
  public:
    Timer(long usec, void (*callback)(int)) :
      duration(usec),
      callback(callback),
      start(0)
    {      
    }
    bool operator ==(Timer other)
    {
      if ((this->callback == other.callback) && (this->duration == other.duration)) {
        return true;
      }
      return false;
    }
    void operator =(Timer other)
    {
      duration = other.duration;
      callback = other.callback;
      start = other.start;
    }
    suseconds_t duration;
    void (*callback)(int);
    suseconds_t start;
  };
  Timer setUpTimer(long micro_duration, void (*callback)(int id));
  friend void *create_pthread(void *data);
  void run();
  bool m_bRunning;
  bool m_bGo;
  long m_lMinSleep;
  std::list<Timer> m_cTimers;
  pthread_t m_tTimerThread;
  pthread_cond_t m_tGoLockCondition;
  pthread_mutex_t m_tGoLock;
};

#endif

---

#include <algorithm>
#include <iterator>
#include <sys/time.h>
#include "TimerManager.h"

extern "C" void *create_pthread(void *data)
{
  TimerManager *thread_timer_manager = static_cast<TimerManager *>(data);
  thread_timer_manager->run();
  return data;
}

TimerManager::TimerManager() :
  m_bRunning(false),
  m_bGo(false),
  m_lMinSleep(0)
{
  int mutex_creation = pthread_mutex_init(&m_tGoLock, NULL);
  if(mutex_creation != 0) {
    std::cerr << "Failed to create mutex" << std::endl;                                   // Use RAII if resource acquisition fails
    return;
  }

  int mutex_cond_creation = pthread_cond_init(&m_tGoLockCondition, NULL);
  if(mutex_cond_creation != 0) {
    std::cerr << "Failed to create condition mutex" << std::endl;                         // Use RAII if resource acquisition fails
    return;
  }

  int thread_creation = pthread_create(&m_tTimerThread, NULL, create_pthread, this);      // On success call create_pthread to start tiemr loop
  if(thread_creation != 0) {
    std::cerr << "Failed to create thread" << std::endl;                                  // Use RAII if resource acquisition fails
    return;
  }
  m_bRunning = true;
}

TimerManager::~TimerManager() 
{
    pthread_mutex_lock(&m_tGoLock);                                                       // m_bRunning access on other thread
    m_bRunning = false;
    pthread_mutex_unlock(&m_tGoLock);
    void *result;
    pthread_join(m_tTimerThread, &result);                                                // Do not let calling thread exit before deleting
    pthread_mutex_destroy(&m_tGoLock);                                                    // Now destroy the mutex (release resources)
    pthread_cond_destroy(&m_tGoLockCondition);
}

void TimerManager::run() 
{
  pthread_mutex_lock(&m_tGoLock);                                                         // Timers run on seperate thread
  while(m_bRunning) {                                                                     // While timer manager exists
    while (!m_bGo) {                                                                      // While timer manager told to run
      pthread_cond_wait(&m_tGoLockCondition, &m_tGoLock);                                 // Set in the start() member function
    }
    pthread_mutex_unlock(&m_tGoLock);                                                     // Once timer unlocked and mutex released
    if (!m_bRunning) {                                                                    // Make sure timer manager not out of scope
      break;
    }

    struct timeval l_tv;
    usleep(std::max(0l, m_lMinSleep));
    gettimeofday(&l_tv, NULL);                                                            // System call to get time of day
    m_lMinSleep = 0;                                                                      // Used to sleep if no timer to go off soon
    long l_lMin = 0;                                                                      // Used if timer goes off to get actual Min sleep
    for(std::list<Timer>::iterator it = m_cTimers.begin(); it != m_cTimers.end(); ++it) { // Iterate over timers to see which one is going off
      TimerManager::Timer l_oTimer = *it;                                                 // Obtain a copy of the timer
      long elapsed_time = ((l_tv.tv_sec * 1000000 + l_tv.tv_usec) - (l_oTimer.start));    // Calcuate the elapsed time from the start of timer X
      l_lMin = elapsed_time - l_oTimer.duration;                                          // Minimum time you can sleep in loop
      if (elapsed_time >= l_oTimer.duration) {                                            // If time passed is greater than or equal to duration: THEN
        l_lMin = l_oTimer.duration;                                                       // The minimum you can wait is possibly the entire duration of that timer
        l_oTimer.callback(0);                                                             // Call the call back
        gettimeofday(&l_tv, NULL);                                                        // After callback called...
        it->start = (l_tv.tv_sec * 1000000) + l_tv.tv_usec;                               // Start the timer over again
      }
      m_lMinSleep = std::min(m_lMinSleep, l_lMin);                                        // Find the actual minumum time you can sleep to not lock
    }
  }
}

void TimerManager::start()
{
  pthread_mutex_lock(&m_tGoLock);                                                         // Go flag accessed from another thread
  m_bGo = true;
  pthread_cond_signal(&m_tGoLockCondition);
  pthread_mutex_unlock(&m_tGoLock);
}

void TimerManager::stop() 
{
  pthread_mutex_lock(&m_tGoLock);                                                         // Go flag accessed from another thread
  m_bGo = false;
  pthread_mutex_unlock(&m_tGoLock);
}

TimerManager::Timer TimerManager::setUpTimer(long micro_duration, void (*callback)(int id))
{
  struct timeval l_tv;
  gettimeofday(&l_tv, NULL);                                                              // System call to get the ms and sec since Epoch0
  Timer l_oTimer(micro_duration, callback);                                               // Create a timer
  l_oTimer.start = (l_tv.tv_sec * 1000000) + l_tv.tv_usec;                                // Tell the timer when to start
  return l_oTimer;                                                                        // Return a copy to addTimer
}

void TimerManager::addTimer(long usec, void (*callback)(int id)) 
{
  pthread_mutex_lock(&m_tGoLock);                                                         // Tell object to wait till timer inserted
  Timer insert = setUpTimer(usec, callback);
  for (std::list<Timer>::iterator it = m_cTimers.begin(); it != m_cTimers.end(); ++it) {
    if (*it == insert) {                                                                  // Return if timer callback and duration the same
        return;
    }
  }
  m_cTimers.push_back(insert);                                                            // If no duplicate timers found then insert into list
  pthread_mutex_unlock(&m_tGoLock);                                                       // Tell object it is okay to proceed
}

Answer 1

Timer Accuracy:

I would say you currently have a bug in your code as you sleep 1 sec between checking each Timer object. After a sleep you should check all the Timer objects once this is complete then go back to sleep.

Timer Granularity

sleep() is very course grained (1 second resolution).

MS has a higher resolution timer (but it is not portable).

I have seen people use select() as a cross platform solution for a higher resolution timer (personally not tried it but it looks like it works). Technically it is to detect activity on streams (ie your web server uses it to wait for input from 1000s of browsers). But if you specify no streams (I think you will need to test) and a timeout of appropriate seconds and micro seconds then you should get your delay.

timeval   timeout = {10 /* seconds */, 23 /* and microseconds */};
if (select(0, NULL, NULL, NULL, &timeout) != 0)
{
      // Need to deal with errors like EINTR
}

Comments on code:

Variable Names

There are several variables that have really short names that are no descriptive to somebody reading your code. I would try and make these names more descriptive to their intent.

Bad use of start()

start() does not look like it needs to be called more than once.
Otherwise why have stop()?

while(true) {
  t.start();
}

thread_destruction

In the destructor I would wait for the thread to terminate.

Otherwise the run() method may potentially be acting on invalid data.

TimerManager::~TimerManager() 
{
    running    = false;   // New member that the thread checks periodically to see
                          // if it should continue running.
                          // Alternative you can send a signal to the thread to kill it.
                          // But I would avoid that as it results in objects not being
                          // destroyed correctly.

    // Wait for the thread to check the flag and terminate.
    void*   result;
    pthread_join(m_timer_thread, &result);
}

Small Change to run() to use running:

void TimerManager::run() 
{
  struct timeval l_tv;
  while(running) /*previously this was true */{
    if (go) {

Thread start-up and avoiding busy loops

In your thread run() phase you basically have a busy wait.
if go is false then the while loop repeats very quickly without releasing the processor for other threads. I bet this shoots your CPU utilization to 100% very quickly if you do anything before calling start() on the TimerManager.

There are two solutions to this

• Put a sleep(1) in the else part
  • This forces the thread to give up its time slice to another thread.
• Use a conditional variable.
  • This will suspend the thread until you signal it to go (in start())

Simple Sleep Solution:

void TimerManager::run() 
{
  struct timeval l_tv;
  while(running)
  {
    if (!go)
    {
        sleep(1);
    }
    else { // Code as before.

Condition Variable

void TimerManager::run() 
{
  struct timeval l_tv;
  while(running)
  {
     // Note: mutex and condition variable should be initialized in constructor
     // Must lock before testing go
     pthread_lock(goLock);
     while (!go)
     {
         // We use a while as the user
         // May potentially call start() stop() very quickly
         // So when the thread exits the condition variable we re-check the state.
         pthread_cond_wait(goCond, goLock);

         // The thread release the mutex when it is suspends and must re-acquire it
         // (internally) before it is release from the wait() call.

     }
     // Unlock now we have established go is true.
     // NOTE: This looks like a good place for RAII
     pthread_unlock(goLock);

     // Check to see if the running flag has been messed with.
     // It we are no longer running then break out of the running loop now.
     if (!running)
     {    break;
          // Note: this mean put a signal in the destructor
          //       just after setting running to false.
     }

We have to alter start and stop appropriately to go with the condition variable.

void TimerManager::start()
{
  pthread_lock(goLock);
  go = true;
  pthread_cond_signal(goCond);
  pthread_unlock(goLock);        // Another opportunity for RAII
}

void TimerManager::stop() 
{
  pthread_lock(goLock);
  go = false;              // Modify go should really have a lock
                           // to be consistent. 
  pthread_unlock(goLock);
}

Bad Variables names:

  li begin = m_timers.begin();
  li end = m_timers.end();

begin is a bad name for the looping variable as you increment it and then it will not be the beginning anymore. There is no need to call end outside the loop. Most standard implementations this is so fast you will not notice it (if internally it is a problem then the implementers have already optimized the call the end() to cash the iterator so you are not saving anything by calling it outside the loop.

  // In C++11 you can use auto for the type
  for(auto it = m_timers.begin(); it != m_timers.end(); ++it)

 // In C++03 you still need to use the correct type
 for(std::list<Timer>::iterator it = m_timers.begin(); it != m_timers.end(); ++it)

Incorrect usage of microseconds

    long elapsed_time = (l_tv.tv_usec - begin->start);

This is not the elapsed time.
The tv_usec loops every second so you need to this to tv_sec to get the actual time value.

    long elapsed_time_in_MS = ((l_tv.tv_sec * 1000000 + l_tv.tv_usec) - (begin->start * 1000000);

Bug in sleep calculations

You should probably move the calls to gettimeofday() and sleep() out of the loop body.

  for(;begin != end; ++begin)
  {
    gettimeofday(&l_tv, NULL); 
    long elapsed_time = (l_tv.tv_usec - begin->start);
    sleep(1);

I would get the time of day then Loop over all the timers to see if they had expired and call the associated function. Once all this has been done you can sleep() for the minimum time required until the next timeout (assuming that non of the functions take too long).

  minSleep = 0;                 // First sleep is zero

  sleep(std::max(0, minSleep));
  gettimeofday(&l_tv, NULL);
  minSleep = 0;                 // Reset sleep (will be calculated in loop.
  for(;begin != end; ++begin)
  {

    long elapsed_time = getElapsedTime(l_tv, egin->start);

    // If this timer is not going to fire this time then we have to sleep a tin bit
    // tiny bit for this Timer.
    long myMin = elapsed_time - begin->duration;

    if (elapsed_time >= begin->duration)
    {
        // If the timer has expired then we need to sleep the whole duration.
        myMin = begin->duration;

        // Current Code
    }
    // The actual amount of time to sleep is the minimum of all myMin.
    minSleep  = std::min(minSleep, myMin);
  }
  gettimeofday(&endOfCallbacks, NULL);
  timeSpentCallingCB = endOfCallbacks.

Dead Code

This seems like dead code:

  li begin = m_timers.begin();
  li end = m_timers.end();
  for(;begin != end; ++begin) {
    if (*begin == *end) {
      return;
    }
  }

Centralize typedef

You have several of the same typdef:

 typedef std::list<Timer>::iterator li;

If you put it inside the class definition then it will be in the scope of all methods of that class. If fact I would go one step further and typedef the container type.

class TimerManager
{
    typedef std::list<Timer>      Container;
    typedef Container::iterator   iterator;

Answer 2

Review Based on Updated Answer:

In the constructor of TimerManager

TimerManager::TimerManager() :

There are several places where a failed call to pthread_X results in you abandoning the initialization of the object and returning. The problem with this is that you now have an object that is not in a consistent state (people can still call start() stop() and addTimer()).

If something fails during construction it is best to throw an exception (If an object is bad then you should not allow it to be used).

Your problem here is that the objects you are initializing in the constructor are C type objects that have a separate destroy call that must be called (but an exception from the constructor will prevent the destructor being called). As a result you may find your self wrapping each of the C objects in their own class to make sure they are automatically destroyed correctly.

The assignment operator for Timer:

void operator =(Timer other)
{
  duration = other.duration;
  callback = other.callback;
  start = other.start;
}

If you don't define it then the compiler will generate a version that does exactly the same (with the caveat that it will return a reference that allows assignment chaining). I have nothing against using void to stop the chaining but I though I should just point it out.

Always try and make sue you symmetrically lock and unlock things like mutexes.

  pthread_mutex_lock(&m_tGoLock); 
  while(m_bRunning)
  { 
      while (!m_bGo)
      { 
          pthread_cond_wait(&m_tGoLockCondition, &m_tGoLock); 
      }
      pthread_mutex_unlock(&m_tGoLock);   
      // STUFF
 }

Notice that if m_bRunning is false at start-up then you may not unlock m_tGoLock. Also the second time around the loop you are call un-lock without calling lock. This case can be fixed by moving the first lock inside the first while loop. In general though this is a good case for the usage of RAII where things need to be done symmetrically even when exceptions are in play (In parallel to the advice about the constructor, it may be worth wrapping the pthread_X objects in some C++ classes).

Though I comment you for commenting.
I prefer comments at a minimum and try not to write comments that tell you what the code is doing. We should be able to see that from reading the code. Comments should be there to explain the overall technique of what we are doing (ie what not how).

Answer 3

gettimeofday should not be used to measure time. Use clock_gettime(CLOCK_MONOTONIC) instead - this clock is not affected by discontinuous jumps in the system time (e.g. if the system administrator manually changes the clock).

Answer 4

2 bugs:

1. This value should be the other way around; thread never sleeps (always sleep a negative number):

   Original:

   l_lMin = elapsed_time - l_oTimer.duration;
   

   Fix:

   l_lMin = l_oTimer.duration - elapsed_time;
   

2. m_lMinSleep = 0; is wrong. In the minimum calculation, 0 or any other positive number, 0 is always the minimum. That is why you need to set it to max long value, or change the type to unsigned with -1 value.