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

[Reimar Döffinger]

On Sat, Nov 07, 2009 at 08:10:15PM +0100, Tobias Diedrich wrote:
> 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...

Not a particularly good name still though.
And the new one could have been called "struct priv" or some such.

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

The read is already a syscall, and if the syscall (or even 10s of those)
in this function is relevant for performance there is a serious bug.

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

The point is that since you use the POSIX_FADV_.. constants it is
completely impossible to use this interface if e.g. Windows added
a WindowsFadvise (ignoring the fact that your proposed code also
wouldn't compile without it).
I don't mind doing it like this, but then you'll have to define a proper
API - minimum criteria would be that it can without issues be used on
a system that only has readahead().
You don't need to add support for that, but it should be possible
without much of an issue.
You should probably also consider a different type for len, so that e.g.
-1 can be used to mean "some default value".
And it will have to have proper doxygen-compatible documentation.
Or you can use my attached ugly hack, assuming it works (I can't test,
not even my DVDs have any problem with playing and seeking perfectly
smooth even without cache).
-------------- next part --------------
Index: stream/stream_file.c
===================================================================
--- stream/stream_file.c	(revision 29850)
+++ stream/stream_file.c	(working copy)
@@ -4,6 +4,8 @@
 #include <stdio.h>
 #include <sys/types.h>
 #include <sys/stat.h>
+// for posix_fadvise
+#define _XOPEN_SOURCE 600
 #include <fcntl.h>
 #include <unistd.h>
 
@@ -34,8 +36,15 @@
   stream_opts_fields
 };
 
+#define PREFETCH_LEN (1024*1024)
+
 static int fill_buffer(stream_t *s, char* buffer, int max_len){
+  off_t pos = lseek(s->fd, 0, SEEK_CUR);
   int r = read(s->fd,buffer,max_len);
+#ifdef POSIX_FADV_WILLNEED
+  posix_fadvise(s->fd, pos, max_len, POSIX_FADV_DONTNEED);
+  posix_fadvise(s->fd, pos + max_len, PREFETCH_LEN, POSIX_FADV_WILLNEED);
+#endif
   return (r <= 0) ? -1 : r;
 }
 
@@ -50,6 +59,9 @@
     s->eof=1;
     return 0;
   }
+#ifdef POSIX_FADV_WILLNEED
+  posix_fadvise(s->fd, s->pos, PREFETCH_LEN, POSIX_FADV_WILLNEED);
+#endif
   return 1;
 }
 
@@ -152,6 +164,9 @@
   }
 
   len=lseek(f,0,SEEK_END); lseek(f,0,SEEK_SET);
+#ifdef POSIX_FADV_WILLNEED
+  posix_fadvise(f, 0, PREFETCH_LEN, POSIX_FADV_WILLNEED);
+#endif
 #ifdef __MINGW32__
   if(f==0 || len == -1) {
 #else