need your advice on clock()

[Pierre-Marie Baty]

I'm making a facility to get rid of gpGlobals->time everywhere I can.

This function is supposed to return the time in seconds since the process started, even after the clock() value has overflown.

Since I don't want to wait 24 days to check if my function works, could someone tell me if my implementation is correct ?

float ProcessTime (void)
{
   // this function returns the time in seconds elapsed since the executable process started.
   // The rollover check ensures the program will continue running after clock() will have
   // overflown its integer value (it does so every 24 days or so). With this rollover check
   // we have a lifetime of more than billion years, w00t!
   // thanks to botmeister for the rollover check idea.
 
   static long current_clock;
   static long prev_clock = 0;
   static long rollover_count = 0;
   float time_in_seconds;
   float rollover_difference;
 
   current_clock = clock (); // get system clock
 
   // has the clock overflown ?
   if (current_clock < prev_clock)
	  rollover_count++; // omg, it has, we're running for more than 24 days!
 
   // now convert the time to seconds and calculate the rollover difference
   time_in_seconds = (float) current_clock / CLOCKS_PER_SEC; // convert clock to seconds
   rollover_difference = (float) 286331153 / CLOCKS_PER_SEC * rollover_count;
 
   prev_clock = current_clock; // keep track of current time for future calls of this function
 
   // and return the time in seconds, adding the overflow differences if necessary.
   return (time_in_seconds + rollover_difference);
}

[botmeister]

Since your function is simply counting how many seconds have elapsed, try this instead

static float time_in_seconds = 0;
static long prev_clock = 0;
long current_clock; 
current_clock = clock();
if (current_clock < prev_clock)
time_in_seconds += ( (float) current_clock + 1 ) / CLOCKS_PER_SEC;
else
time_in_seconds = (float) current_clock / CLOCKS_PER_SEC;
return time_in_seconds;

You may want to use double instead of float because float does hot have enough precision from some time conversion operations.

btw, why won't my code format the way I want it to?

[Pierre-Marie Baty]

I am under the impression that your code assumes that the difference between current_clock and prev_clock is normally always 1. Is this correct ?

This is not the case, and can't be, because this function can be called anytime and it can elapse minutes (or hours) between two successive calls, and thus the current_clock and prev_clock variables could differ greatly. And time_in_seconds wouldn't have been updated since last call, and thus would be completely obsolete.

There may be something I don't understand, but to me it seems your code is heavily buggy

[botmeister]

oops, you are right, the code I posted won't work for at least 2 or 3 different reasons 8o

Let's have a look at this one instead

static float time_in_seconds = 0;
static long prev_clock = 0;
long current_clock;
float time_diff;

current_clock = clock();

if (current_clock < prev_clock)
{
// calculate ticks elapsed up to rollover event
time_diff = LONG_MAX - prev_clock;
// add on ticks elapsed after rollover event
time_diff = time_diff + current_clock + 1;
}
else
time_diff = current_clock - prev_clock;

prev_clock = current_clock;
time_in_seconds += time_diff / CLOCKS_PER_SEC;
return time_in_seconds
Note
The function MUST be called at least once between every rollover event!

DAMN! I still have no idea how to copy/paste code into this thing so it shows up the way I want it to. I need a tutorial or something.

[Pierre-Marie Baty]

Your code doesn't format right because you're using tabs as separators... and you can't write tabs in the "post reply" text boxes. I'm not sure there's anything you can do but to change your tabs to spaces...

Back on track, basically all I wanted to know was whether my function would work. Does it ? Assuming it does, what's the advantage of yours over mine ?

[botmeister]

Working with clock ticks is so much fun and so very confusing

I like what you've come up with, and you've got me revisting some of my existing "clock" code which probably can be made simpler and faster without losing any precision.

Anyhow, I looked over your code (and over) and found some minor things which are very subtle and may not matter much depending on the application, but there's one part which appears to be incorrect.

Here goes ...

static long current_clock;

The above line does not have to be static, however you may gain a performance edge by not allocating stack space for it on each call.

This line has a small error in it:

time_in_seconds = (float) current_clock / CLOCKS_PER_SEC;

After a rollover event, the elapsed time in seconds is actually:

time_in_seconds = (float) ( current_clock + 1 ) / CLOCKS_PER_SEC;

The extra clock tick is added because the rollover looks like this (assuming a MAX value of 2)

Clock ticks --->
0 1 2 0 1 2 0 1 2
	A B

The elapsed time from tick A to tick B is not 0 ticks, but 1 tick, however the exsisting calculation assumes 0 ticks elapsed, which is technically incorrect, but insignificant for timing the actions of a bot.

This line I cannot understand:

rollover_difference = (float) 286331153 / CLOCKS_PER_SEC * rollover_count;

What does 286331153 represent? If we are calculating the number of seconds elapsed during each rollover, then you should be using 2147483647 (2^31 - 1) as the constant (if I typed it in right). You should use the macro LONG_MAX to make the code machine independant.

(float) 286331153 / CLOCKS_PER_SEC is an inline division of two constants, you may as well precalculate this. The compiler may do this for you, but it would be cleaner to precalculate elsewhere.

You may want to replace the data type "float" with "double" because float does not have enough precision to get exact timing conversions in all cases.

The alternative function I posted reduces the number of divisions and multiplications which are computationally more expensive than addition and subtraction, it should reduce precision errors, and it should correct for all the errors I found (I hope anyway).

What I'd do next is replace float with double, and turn the function into a fully contained class object. There's probably still some performance improvements to be made in there as well.

[Pierre-Marie Baty]

Originally Posted by botmeister

This line has a small error in it:

time_in_seconds = (float) current_clock / CLOCKS_PER_SEC;

After a rollover event, the elapsed time in seconds is actually:

time_in_seconds = (float) ( current_clock + 1 ) / CLOCKS_PER_SEC;

...and one bug, one!
Thanks.
But instead of adding +1 to current_clock ONLY IF a rollover has happened, I'd rather add 1 to LONG_MAX (sounds more logical) - no worry, LONG_MAX won't overflow, since I'll cast it to (double) beforehand.

rollover_difference = ((double) LONG_MAX + 1) / CLOCKS_PER_SEC * rollover_count;

This line I cannot understand:

rollover_difference = (float) 286331153 / CLOCKS_PER_SEC * rollover_count;


What does 286331153 represent? If we are calculating the number of seconds elapsed during each rollover, then you should be using 2147483647 (2^31 - 1) as the constant (if I typed it in right). You should use the macro LONG_MAX to make the code machine independant.

Would you believe I don't understand myself how this number managed to land in the middle of my code ??? I remember having used Window's calculator to find out what the maximal value for a 32-bit integer would be, but looks like the Windows calculator is not trustable enough even for things like this o_O

(float) 286331153 / CLOCKS_PER_SEC is an inline division of two constants, you may as well precalculate this. The compiler may do this for you, but it would be cleaner to precalculate elsewhere.

good idea, thank you !

You may want to replace the data type "float" with "double" because float does not have enough precision to get exact timing conversions in all cases.

alright, you're right again, will do.

Thanks a lot for the review, it's much faster when you get your code reviewed by people who have not spent the day with the nose above it, pointing mistakes is so easy Tell me if I can give you a hand in return someday

[edit] Where is LONG_MAX defined ? I can't find this macro anywhere...

[Pierre-Marie Baty]

Nevermind, found it - in limits.h

Here's the function now.

float ProcessTime (void)
{
   // this function returns the time in seconds elapsed since the executable process started.
   // The rollover check ensures the program will continue running after clock() will have
   // overflown its integer value (it does so every 24 days or so). With this rollover check
   // we have a lifetime of more than billion years, w00t!
   // thanks to botmeister for the rollover check idea.
 
   static long current_clock;
   static long prev_clock = 0;
   static long rollover_count = 0;
   static double time_in_seconds;
   static double rollover = ((double) LONG_MAX + 1) / CLOCKS_PER_SEC; // fixed, won't move
   
   current_clock = clock (); // get system clock
 
   // has the clock overflown ?
   if (current_clock < prev_clock)
	  rollover_count++; // omg, it has, we're running for more than 24 days!
 
   // now convert the time to seconds since last rollover
   time_in_seconds = (double) current_clock / CLOCKS_PER_SEC; // convert clock to seconds
 
   prev_clock = current_clock; // keep track of current time for future calls of this function
 
   // and return the time in seconds, adding the overflow differences if necessary.
   return (time_in_seconds + rollover * rollover_count);
}

[botmeister]

Looks much nicer now. You're down to 1 division, one multiplication and one addition per call.

Just one last thing to suggest, since rollovers occure so infrequently, there's a lot of wasted cpu cycles recalcualting rollover * rollover_count for each function call. I suggest that you store the result as static local and update it only after each rollover event.

Tell me if I can give you a hand in return someday

You've already helped me several times now!!! I'm just trying to keep pace with you

one other thing - is current_clock, etc. ever negative? it doesn't seem so, therefore why not make those values unsigned longs instead of just longs, and thus get 2^32-1 clock ticks till rollover instead of 2^31-1?

second thing, the return value should probably be a double if you are passing back time_in_seconds, which is a double.