[MPlayer-dev-eng] [PATCH] Use posix_fadvise in stream_file if available

[Reimar Döffinger]

On Sat, Nov 07, 2009 at 07:37:30PM +0100, Tobias Diedrich wrote:
> @@ -13,14 +14,22 @@
>  #include "m_option.h"
>  #include "m_struct.h"
>  
> -static struct stream_priv_s {
> +#include "osdep/fadvise.h"
> +
> +static struct stream_opts_priv_s {
>    char* filename;
>    char *filename2;
> -} stream_priv_dflts = {
> +} stream_opts_priv_dflts = {
>    NULL, NULL
>  };

I completely fail to see the point of this part.

>  static int fill_buffer(stream_t *s, char* buffer, int max_len){
> +  struct stream_priv_s *p = s->priv;
>    int r = read(s->fd,buffer,max_len);
> +  if (s->pos > p->prefetch_last_pos + PREFETCH_LEN/2) {
> +    p->prefetch_last_pos = s->pos;
> +    fadvise(s->fd, s->pos, PREFETCH_LEN, POSIX_FADV_WILLNEED);
> +  }

I'll only accept that if you can show that having this extra check
will actually have a performance advantage.

> --- /dev/null	2009-11-07 12:06:22.225017004 +0900
> +++ osdep/fadvise.h	2009-11-08 02:44:46.747027932 +0900
> @@ -0,0 +1,31 @@
> +/*
> + * This file is part of MPlayer.
> + *
> + * MPlayer is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * MPlayer is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License along
> + * with MPlayer; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#ifndef MPLAYER_FADVISE_H
> +#define MPLAYER_FADVISE_H
> +
> +#if HAVE_POSIX_FADVISE == 1
> +int fadvise(int fd, off_t offset, off_t len, int advice);
> +#else
> +static int fadvise(int fd, off_t offset, off_t len, int advice)
> +{
> +	return -1;
> +}

Having this in a separate osdep file is completely pointless when the
API can only work properly with the POSIX interface.
The way you implemented it, a simple
#ifdef POSIX_FADV_WILLNEED
would probably work just as well.
And the == 1 really is pointless, too, it'll stay the same no matter if
you write
> +#if HAVE_POSIX_FADVISE == 1
or
> +#if HAVE_POSIX_FADVISE == 1 == 1 == 1 == 1 == 1 == 1
though
> #if HAVE_POSIX_FADVISE
is more correct since if HAVE_POSIX_FADVISE should ever be e.g. 2 that
probably _won't_ mean it is not available.