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

[Rémi Denis-Courmont]

On Wed, 3 Jun 2009 14:14:15 +0200, jpd at videolan.org wrote:
> The point is that the opaque data is already in the timer passed,

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.

> so you're merely doing a deref on behalf of the client whether it needs
> it or not.

The client pretty much always needs it anyway (currently all two users do).
Besides, that pointer is most probably in the same cache line as the
function pointer, which we need to dereference in any case.

> I don't agree on your assessment of ugly here, though.
> Opaque cookie pointers tend to get treated this way:

In some constrained environment, such as kernels, the opaque data pointer
is sometimes completely absent, in favor of the timer pointer. That works
if the callback knows the offset of the timer structure within its own
private data. The container_of() macro on Linux is one example. VLC is not
that constrained (and has much much worse performance bottlenecks): it has
at least 256kb of stack (Linux has 8kb) and it runs in virtual memory. 

>   void func0(vlc_timer_t *timer, void *data)
>   {
>     realstruct *realdata = (mystruct *)data; /* last time we use data */
>     ...
>   }
> 
> versus
> 
>   void func1(vlc_timer_t *timer)
>   {
>     realstruct *realdata = (mystruct *)timer->data;
>     ...
>   }
> 
> Passing two arguments doesn't seem to add substance, in fact the extra
> void * is effectively dead space on the stack.

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.

> To me one argument, either one, would be enough,
> but I'd likely choose the timer one all the same.

In practice, the opaque data is always needed. The timer is hardly ever
needed (not used currently). I decided to add the timer parameter so that
calling vlc_timer_getoverrun() or vlc_timer_destroy() would be easy
(vlc_timer_destroy() won't work on Windows currently though).

> Passing vlc_timer_t * allows the callee to modify timer->data, (and
> the rest of the timer struct) so if that is a problem then pass only
> the void * cookie. Or maybe make the timer const and document that Bad
> Things Will Happen when casting away constness.

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.

>> > As a sidenote, some systems have hard limits on the number of timers
>> A timer requires fewer resources than a permanent thread, which is what
>> we had earlier.

> One permanent thread for all timers still beats using many timers in
> terms of resource pressure.

The glibc implementation creates a common thread internally, and Win32 uses
a thread pool. I trust the OS run-time better than you or me as regards
what the most efficient implementation is. Hence, I prefer using
semantically closest lower-level API over re-inventing the wheel.

> Of course it would be simpler to not care,
> but how many timers does a random vlc chain use?

Zero.

Currently, there is one timer per instance (screensaver) on Linux, and one
timer per DVD (dvdnav still image handler). Live555 RTSP pinging seems like
a good third candidate (if someone bothers).

-- 
Rémi Denis-Courmont