Is this timer efficient?

Question

I found myself writing code to run at a specified frequency more than once, so I decided to make this simple timer. My main concern is that this is not the most efficient way of doing this since with SDL I would just ask for the time in ms and sleep in ms.

I'm storing the current time in timer_update() to avoid the possibility of it becoming greater than the final time and generating a huge pause since it's unsigned.

Usage is pretty simple, timer_init() is called once before the loop and timer_update() is called once at the end of every iteration.

timer.h

#ifndef TIMER_H
#define TIMER_H

typedef struct {
    double step;
    double tfinal;
} Timer;

void timer_init(Timer *timer, double hertz);
void timer_update(Timer *timer);

#endif

timer.c

#include "timer.h"

#define _POSIX_C_SOURCE 199309L
#include <time.h>
#include <errno.h>
#include <stdio.h>

static unsigned long get_ms(void)
{
    struct timespec buffer;
    unsigned long temp;

    if(clock_gettime(CLOCK_MONOTONIC, &buffer) != 0){
        perror("clock_gettime");
        return 0;
    }

    temp = buffer.tv_sec * 1000;
    temp += buffer.tv_nsec / 1000000;
    return temp;
}

static struct timespec ms_to_timespec(unsigned long ms)
{
    struct timespec temp = {ms / 1000, (ms % 1000) * 1000000};
    return temp;
}

void timer_init(Timer *timer, double hertz)
{
    timer->step = 1000 / hertz;
    timer->tfinal = get_ms() + timer->step;
}

void timer_update(Timer *timer)
{
    unsigned long now = get_ms();
    struct timespec required_sleep, rem;

    if(now < timer->tfinal){
        required_sleep = ms_to_timespec(timer->tfinal - now);
        timer->tfinal += timer->step;

        try_to_sleep:
        if(nanosleep(&required_sleep, &rem) == -1 && errno == EINTR)
            if(nanosleep(&rem, &required_sleep) == -1 && errno == EINTR)
                goto try_to_sleep;
    }

    else
        timer->tfinal += timer->step;
}

Answer 1

Is this timer efficient?

• Yes, but there are two optimizations that I can see. Get rid of the goto and use a do-while loop. I have noticed a slight performance increase when it has been written this way in other programs.

  This increase will become non-existent however if you have compiler optimization settings enabled. If they are enabled, they compile into the exact same assembly code. However, I also find a loop more readable than a goto, so that is another reason to switch over.

• There other optimization I could see is by cleaning up your if-else a tiny bit (untested). This should speed up your code a tiny bit (again, if optimizations on your compiler are enabled, your results will vary).

  if(now < timer->tfinal)
  {
      required_sleep = ms_to_timespec(timer->tfinal - now);
  
      // loop implementation
  }
  timer->tfinal += timer->step;
  

  You may wonder why this will speed up your code. The more branches you have, the more susceptible you are to a branch prediction failure and those failures can be expensive when it comes to efficiency.

---

A few other notes:

• Use a designated initializer to initialize a structure ^(C99).

  struct timespec temp = 
  { 
      .step = ms / 1000, 
      .tfinal = (ms % 1000) * 1000000 
  };
  

• Group your #includes and #defines together in a more organized fashion.

  #include <time.h>  // group library headers
  #include <errno.h>
  #include <stdio.h>
  #include "timer.h" // group user defined headers
  
  #define _POSIX_C_SOURCE 199309L  // group defines
  

• Your != 0 comparision check in your if conditional could be removed since it is redundant (the if will always execute if the conditional isn't equal to 0). However, this may increase readability to you, so it would be okay to keep it.

Answer 2

this Method is kind of ugly

void timer_update(Timer *timer)
{
    unsigned long now = get_ms();
    struct timespec required_sleep, rem;

    if(now < timer->tfinal){
        required_sleep = ms_to_timespec(timer->tfinal - now);
        timer->tfinal += timer->step;

        try_to_sleep:
        if(nanosleep(&required_sleep, &rem) == -1 && errno == EINTR)
            if(nanosleep(&rem, &required_sleep) == -1 && errno == EINTR)
                goto try_to_sleep;
    }

    else
        timer->tfinal += timer->step;
}

You have nested if statements that have no braces and you only need one if statement with a slightly longer conditional. This should stop one thing from being tested twice.

try_to_sleep:
if((nanosleep(&required_sleep, &rem) == -1 && errno == EINTR) && nanosleep(&rem, &required_sleep)) {
    goto try_to_sleep;
}

The else statement bracing style should match the if statement that it is attached to, even if it is a one liner or it should truly be one line.

Using braces:

else {
    timer->tfinal += timer->step;
}

One Lined

else timer->tfinal += timer->step;

Answer 3

Rather than looping around goto, why not calculate how much time is remaining and convert it into ticks and sleep (ticks)? It would also be better to find some way to generate an event rather than lopping continuously, if timing is not very strict.