Avoid unnecessary calls to time(NULL)

[charles]

libtransmission calls time(NULL) very often, and it's an expensive call on some systems. If we have a timer that updates a session time_t variable once per second, we could save those redundant cycles.

We could make the timer self-correcting: if we call gettimeofday() we'll have the current time in seconds and milliseconds. The former will go to the tr_session time_t variable, and the latter will be used to decide when the timer should next fire.

[charles]

cc'ing jch to get a second opinion on this patch before committing ;)

[jch]

Full agreement on the principle. One major nit, and a few minor doubts.

The major nit is -- why is now a session field rather than a global variable? Are you assuming that time flows at different rates in different sessions?

I'd just go for

    static inline time_t tr_time(void) { return transmission_now; }
    static inline void tr_update_time(time_t now) { transmission_now = now; }

in util.h, and have the global transmission_now shared between all the sessions.

This will also make it possible to more easily change implementations in the future -- see point 3 below.

Minor doubts, which you should feel free to ignore right now, they are things that can easily be changed in the future:

1. While reworking the time infrastructure, do we want to switch to monotonic time on arches that support it? The patch is trivial, but it means that we need to be careful not to use the result of tr_time for real-time computations.

2. Why all the hard work in onNowTimer? Isn't it simpler to document tr_now as being late by no more than one second, and simply run onNowTimer with a one second deadline? Or is there any place in Transmission that would be broken by time being late by one second? While I'm at it, I'd add some jitter to the timer, which will have the beneficial effect of adding jitter to all off Transmission's timeouts.

3. Another way is to update the time in a SIGALRM signal handler. That's what the X server does. I'm not sure what the advantages are, if any.

4. The best way would be to use libevent's internal notion of time rather than computing our own. After all, libevent already has to compute the time whenever it wakes up from a wait on select/poll/ev_wait/kqueue/whatever. I've looked at the libevent sources, and unfortunately it doesn't seem to support that (libev does).

--Juliusz

[charles]

Addressing the nits:

0. why not a global - I agree, as a global makes more sense.

1. monotonic time - I'm not sure I see the advantage. also, OS X doesn't implement clock_gettime().

2. There are a lot of functions that rely on time(NULL) and it's not clear to me that all of them would benefit from jitter. It might be better to add a sibling function to tr_time() that adds jitter.

3. SIGALRM - I'm not sure I see the advantage.

4. I don't understand this. Could you give me a concrete example of what this would look like in libevent? Or are you saying libev has a feature for this that's missing in libevent?

[charles]

revised, globalised patch

[charles]

r9593 libtransmission/ (15 files): (trunk libT) #2610 "avoid unnecessary calls to time(NULL)"

[jch]

monotonic time - I'm not sure I see the advantage

What happens when you step the system time?

If you step the time forwards, Transmission will spuriously trigger a bunch of timeouts, doing things like dropping active connections and discarding recently seen atoms.

If you step the time backwards, a lot of timeouts will be years in the future, and Transmission will never trigger a bunch of timeouts. This will cause connections to hang, atoms never to expire, etc.

OpenWRT, for example, will typically step the time forwards by a decade or so a few minutes after boot. Is that a problem for the Transmission user-base?

OS X doesn't implement clock_gettime().

See the function gettime in ​http://www.pps.jussieu.fr/~jch/software/repos/babeld/kernel.c .

--Juliusz

[jch]

Could you give me a concrete example of what this would look like in libevent? Or are you saying libev has a feature for this that's missing in libevent?

What I'm saying is that it is a feature that's trivial to implement in any event loop, and I'm surprised that libevent doesn't have it.

What an event loop basically does is

  while(1) {
      now = time(NULL);
      timeout = deadline of first timer - now;
      select(bunch of fds, timeout);
      now = time(NULL);
      run any timer handlers whose deadline is before now;
      run any file descriptor handlers that have triggered;

As you can see, just before running any user handlers, the event loop computes the current time. It doesn't cost much to allow the user handlers to use this value.

I think I'm going to have a chat with Nick on the subject.

--Juliusz

[charles]

Looks like r9593 introduced a bug in onNowTimer() on OS X... adding a bounds test for the usec value in r9594.