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

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 

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 
#include 
#include 
#include 

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 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
#include
#include
#include "TimerManager.h"

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

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 t

StackExchange Code Review Q#8611, answer score: 8