[vlc-devel] commit: Asynchronous timer API ( R??mi Denis-Courmont )

[Rémi Denis-Courmont]

Le mercredi 3 juin 2009 17:37:25 jpd at videolan.org, vous avez écrit :
> On Wed, Jun 03, 2009 at 03:00:10PM +0200, R??mi Denis-Courmont wrote:
> > The layout of VLC threading structures is supposed to be opaque to source
> > code, and OS-dependent. The only reason it's exposed in vlc_threads.h is
> > so that we don't need a pair of dedicated malloc()/free() for each and
> > every thread, mutex, variable, spin, key and timer.
>
> Providing an accessor would take care of that.

Of all options, the accessor is the one that needs the longest source code, 
the largest binary code, occupies to most stack and has the worst locality.

vlc_timer_getoverrun() uses that pattern because it is not always needed yet 
it requires a system call on some implementations. To the contrary, private 
data is always needed if the timer is to do anything useful - or almost. Why 
would we make it more intricate to obtain?

> > In the first case, the compiler can be clever enough to alias realdata to
> > data. In the second case, it cannot. Hence, the stack consumption should
> > be the same. That is not to mention that the calling convention might
> > well put both parameters in registers.
>
> None of that is guaranteed, nor knowable without exhaustive survey,
> and no more than clever clean up after avoidable action. I don't see
> significant difference in cache line use whether that deref happens
> before or after the calling branch.

Yes. The cache line use is most certainly irrelevant and the calling 
convention is platform-dependent and hence also mostly irrelevant. But so is 
the extra pointer on the stack.

> > You're not supposed to change the data across calls. I put the opaque
> > data in the structure instead of giving it up to the system because
> > neither POSIX nor Win32 timers are compatible with void(*)(void *). As
> > such, one level of indirection is required. Again, this is an
> > implementation detail.
>
> And so is whether to pass a vlc_timer_t * to the function. If there's
> nothing in there for the callee to look at, it shouldn't be there. If
> there's merely nothing to touch, it should be const.

For the second or third time, the callback could fetch the overrun counter 
(const), although there are no such occurences at the moment. It could also 
reschedule the timer (not const).

My first point is: the data pointer is always used currently, and likely will 
almost always be needed. The timer pointer is never used currently, although 
it maybe useful in a few specific cases. Hence it is more logical to give the 
data pointer than the timer pointer. My second point is: it is more consistent 
and conventional to give the opaque data (see vlc_clone(), see also the 
underlying implementations); if we were to provide only the timer pointer, we 
should *get rid* of the opaque data altogether and use something like Linux 
kernel's container_of or VLC's own vlc_object_internals() and libvlc_priv(), 
but it certainly would be less convenient.

-- 
Rémi Denis-Courmont
http://www.remlab.net/