[Asterisk-Dev] timeval stuff

[Luigi Rizzo]

On Thu, Jun 09, 2005 at 10:22:13AM -0500, Kevin P. Fleming wrote:
> Luigi Rizzo wrote:
> 
> > compared to the use of pointers, there is an extra copy of 64-bit values,
> > however 1) on modern machines that cost is probably negligible,
> > and 2) these things are generally associated to more expensive
> > computations, such as system calls etc. so i would not worry.
> 
> I like this idea (in fact I'm about to pull the ms_diff function out of 
> app_alarmreceiver.c and into utils.h so I can use it in some other 
> code). However, I think we need to stick with passing struct pointers 
> around whenever we can, unless it's really ugly.

i started with passing pointers, but then had to give up.
The problem with passing pointers is that you cannot nest function
calls, so readability suffers a lot. We have many places where
we need to convert from samples or milliseconds to timeval and
vice-versa, which with pointers would require separate variables
and statements, whereas this way can be done in line.

Look at how the new approach simplifies code:

    -   s->delivery.tv_sec += (len * s->samplesperbyte) / 8000.0;
    -   s->delivery.tv_usec += (((int)(len * s->samplesperbyte)) % 8000) * 125;
    -   if (s->delivery.tv_usec > 1000000) {
    -       s->delivery.tv_usec -= 1000000;
    -       s->delivery.tv_sec += 1;
    -   }
    +   s->delivery = ast_tvadd(s->delivery, ast_samp2tv(s->f.samples, 8000));

or here...

    -       gettimeofday(&rtp->dtmfmute, NULL);
    -       rtp->dtmfmute.tv_usec += (500 * 1000);
    -       if (rtp->dtmfmute.tv_usec > 1000000) {
    -               rtp->dtmfmute.tv_usec -= 1000000;
    -               rtp->dtmfmute.tv_sec += 1;
    -       }
    +       rtp->dtmfmute = ast_tvadd(ast_tvnow(), ast_tv(0, 500000));

or here...

    -       long sdiff;
    -       long udiff;
    -       sdiff = f->delivery.tv_sec - path->nextin.tv_sec;
    -       udiff = f->delivery.tv_usec - path->nextin.tv_usec;
    -       path->nextin.tv_sec = f->delivery.tv_sec;
    -       path->nextin.tv_usec = f->delivery.tv_usec;
    -       path->nextout.tv_sec += sdiff;
    -       path->nextout.tv_usec += udiff;
    -       if (path->nextout.tv_usec < 0) {
    -           path->nextout.tv_usec += 1000000;
    -           path->nextout.tv_sec--;
    -       } else if (path->nextout.tv_usec >= 1000000) {
    -           path->nextout.tv_usec -= 1000000;
    -           path->nextout.tv_sec++;
    -       }
    +       path->nextout = ast_tvadd(path->nextout,
    +               ast_tvsub(f->delivery, path->nextin));
    +       path->nextin = f->delivery;

or here...

    -       if ( (!fromtrunk) && (iaxs[fr->callno]->rxcore.tv_sec || iaxs[fr->callno]->rxcore.tv_usec) ) {
    -               fr->af.delivery.tv_sec = iaxs[fr->callno]->rxcore.tv_sec;
    -               fr->af.delivery.tv_usec = iaxs[fr->callno]->rxcore.tv_usec;
    -               fr->af.delivery.tv_sec += fr->ts / 1000;
    -               fr->af.delivery.tv_usec += (fr->ts % 1000) * 1000;
    -               if (fr->af.delivery.tv_usec >= 1000000) {
    -                       fr->af.delivery.tv_usec -= 1000000;
    -                       fr->af.delivery.tv_sec += 1;
    -               }
    +       if ( !fromtrunk && !ast_tvzero(iaxs[fr->callno]->rxcore)) {
    +               fr->af.delivery = ast_tvadd(iaxs[fr->callno]->rxcore,
    +                        ast_samp2tv(fr->ts, 1000));

and many other pieces of code like this.

In terms of performance, i doubt you'd see a measurable difference - even
assuming we could care, if
you inline the simplest functions such as the constructors, I bet
gcc optimzes away the extra assignment, and for other cases, you copy
an extra 4 bytes once but then save a pointer dereference on each
access (again the compiler will likely do the copy at the first access
to save the dereference).

	cheers
	luigi