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

[Tobias Diedrich]

Reimar Döffinger wrote:
> 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.

Since there is already a stream_priv struct that is really just used
for the options and not for stream->priv and I needed to use
stream->priv I thought it best to rename the existing stream_priv
struct to stream_opts_priv instead of naming the new one
stream_another_priv or stream_priv2 or similar...

> >  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.

The idea is to only call fadvise every now and then instead of for
every read. The if should be much less expensive than a syscall.

> > --- /dev/null	2009-11-07 12:06:22.225017004 +0900
> > +++ osdep/fadvise.h	2009-11-08 02:44:46.747027932 +0900
[...]
> > +#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.

#ifdef's in c code are ugly as hell. I very much like the linux
kernel approach of hiding them in header files as much as possible.
Since I don't check the fadvise error code (IMHO it doesn't make
sense to do so) the fadvise will be completely eliminated by the
compiler in the 'we don't have posix_fadvise' case.

However, if you insist I can move the ifdef into stream_file instead.

> 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.

Yep, it's way too late over here (4am now)...

-- 
Tobias						PGP: http://8ef7ddba.uguu.de