[MPlayer-dev-eng] threaded cache

[Reimar Döffinger]

On Sat, Nov 26, 2011 at 07:41:00PM +0100, Dan Oscarsson wrote:
> +#if defined(PTHREAD_CACHE)

Just use #ifdef

> +#if defined(PTHREAD_CACHE)
> +  if (pthread_mutex_init(&s->go_ahead_mutex, NULL) || pthread_cond_init(&s->go_ahead, NULL)) {
> +    shared_free(s->buffer, s->buffer_size);
> +    shared_free(s, sizeof(cache_vars_t));

TO be pedantic if only thread init fails you'd have to destroy the
mutex.

> +    return NULL;

Might be too much effort but in principle it would be nice if it could
fall-back, both to pthreads without mutex/condition and to forked
thread. Not really important though.
Also, shouldn't cache_uninit already be called and handle this?
Also, cache_uninit NULLs these after freeing them.

> @@ -383,10 +402,14 @@
>   */
>  static void cache_mainloop(cache_vars_t *s) {
>      int sleep_count = 0;
> +    int sleep_time;
>  #if FORKED_CACHE
>      struct sigaction sa = { .sa_handler = SIG_IGN };
>      sigaction(SIGUSR1, &sa, NULL);
>  #endif
> +#if defined(PTHREAD_CACHE)
> +    struct timespec ts;
> +#endif

I prefer declarations to be in the innermost block they can be,
it tends to make it easier to follow the data flow and also
to refactor into new functions.

> @@ -398,9 +421,18 @@
>  #endif
>              if (sleep_count < INITIAL_FILL_USLEEP_COUNT) {
>                  sleep_count++;
> -                usec_sleep(INITIAL_FILL_USLEEP_TIME);
> +                sleep_time = INITIAL_FILL_USLEEP_TIME;
>              } else
> -                usec_sleep(FILL_USLEEP_TIME); // idle
> +                sleep_time = FILL_USLEEP_TIME; // idle
> +#if defined(PTHREAD_CACHE)
> +            pthread_mutex_lock(&s->go_ahead_mutex);
> +            clock_gettime(CLOCK_REALTIME, &ts);
> +            ts.tv_nsec += sleep_time*1000;
> +            pthread_cond_timedwait(&s->go_ahead, &s->go_ahead_mutex, &ts);
> +            pthread_mutex_unlock(&s->go_ahead_mutex);
> +#else
> +            usec_sleep(sleep_time);
> +#endif

I'd expect that the latency for command-handling is more relevant than
here - under normal operation we should never get into this loop (ok,
except for seeking, but there it should never really matter among other
delays).
The problem is that the critical wait is "hidden" inside
stream_check_interrupt.

> --- configure.org	2011-11-05 18:04:43.207091149 +0100
> +++ configure	2011-11-20 13:13:20.626348701 +0100
> @@ -3590,14 +3590,12 @@
>  fi
>  echores "$_pthreads"
>  
> -if cygwin ; then
>    if test "$_pthreads" = yes ; then
>      def_pthread_cache="#define PTHREAD_CACHE 1"
>    else
>      _stream_cache=no
>      def_stream_cache="#undef CONFIG_STREAM_CACHE"
>    fi
> -fi

That is wrong, cache shouldn't be disabled completely when pthreads
are not available, only when cygwin _and_ pthreads not available,
your code will disable thread-based cache on Windows and possibly also cache
on other OS.