Re: [PATCH] sort: use posix_fadvise to announce access patterns on files opened for reading

[Joey Degges]

2010/2/27 Pádraig Brady <address@hidden>

> Well as long as the reads are big enough,
> read-process-read-process-... won't be significantly different than
> read-read-process-process-... However in the case where the
> storage and CPU are contended for by other processes the former
> is more desirable. You could test that by spinning the CPU with
> another process while doing the test. It still seems to me
> that WILLNEED is not appropriate for the sequential case.
>
> Here are the results of sorting when two other processes are running at
100% each (dual core) for the duration of the sort:
NORMAL       : 279.03s, 12.0% CPU
SEQUENTIAL: 230.92s, 14.5% CPU
WILLNEED    : 186.26s, 18.5% CPU

I suspect this trend may also repeat in situations where other processes are
contending for both CPU and storage. Perhaps the priority of the WILLNEED
read ahead is much higher than that of typical reads? Just a guess.


> While we're looking at this it would also be worth considering
> DONTNEED which could be used to drop the cache for temp files
> when we're finished reading them. Note one shouldn't specify
> a len of 0 until the file is completely read so as not to
> invalidate any read ahead cache associated with the file.
>

Thanks for the suggestion. I have tested DONTNEED both in xfclose and in
fillbuf but do not see any improvements. It may be that this will only
improve performance in situations where resources are very tight. A patch
containing both WILLNEED and DONTNEED is included below.

To accommodate different advice flags I added some defines for the different
flags. Would you recommend doing this any other way?

Thanks,
Joey


>From bd934f42675739d064776bae4e631b0b2f02f7ab Mon Sep 17 00:00:00 2001
From: Joey Degges <address@hidden>
Date: Thu, 25 Feb 2010 22:54:50 -0800
Subject: [PATCH] sort: use posix_fadvise to announce access patterns on file
data

---
 configure.ac |    3 +++
 src/sort.c   |   39 ++++++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/configure.ac b/configure.ac
index b07a52b..c07fbd4 100644
--- a/configure.ac
+++ b/configure.ac
@@ -344,6 +344,9 @@ if test "$elf_sys" = "yes" && \
   gl_ADD_PROG([optional_pkglib_progs], [libstdbuf.so])
 fi

+# Check for fcntl.h/posix_fadvise
+AC_CHECK_HEADERS(fcntl.h, [AC_CHECK_FUNCS(posix_fadvise)])
+
 ############################################################################
 mk="$srcdir/src/Makefile.am"
 # Extract all literal names from the definition of $(EXTRA_PROGRAMS)
diff --git a/src/sort.c b/src/sort.c
index 481fdb8..caf4610 100644
--- a/src/sort.c
+++ b/src/sort.c
@@ -32,6 +32,7 @@
 #include "filevercmp.h"
 #include "hard-locale.h"
 #include "hash.h"
+#include "ignore-value.h"
 #include "md5.h"
 #include "physmem.h"
 #include "posixver.h"
@@ -47,6 +48,15 @@
 #include "xnanosleep.h"
 #include "xstrtol.h"

+#if HAVE_FCNTL_H && HAVE_POSIX_FADVISE
+# include <fcntl.h>
+# define XFADV_DONTNEED POSIX_FADV_DONTNEED
+# define XFADV_WILLNEED POSIX_FADV_WILLNEED
+#else
+# define XFADV_DONTNEED 0
+# define XFADV_WILLNEED 0
+#endif
+
 #if HAVE_SYS_RESOURCE_H
 # include <sys/resource.h>
 #endif
@@ -794,6 +804,17 @@ create_temp_file (int *pfd, bool survive_fd_exhaustion)
   return node;
 }

+/* Wrapper for posix_fadvise. Used to predeclare an access pattern for file
+   data. Ignore any errors -- this is only advisory.  */
+
+static void
+xfadvise (int fd, off_t offset, off_t len, int advice)
+{
+#if HAVE_POSIX_FADVISE
+  ignore_value (posix_fadvise (fd, offset, len, advice));
+#endif
+}
+
 /* Return a stream for FILE, opened with mode HOW.  A null FILE means
    standard output; HOW should be "w".  When opening for input, "-"
    means standard input.  To avoid confusion, do not return file
@@ -805,10 +826,18 @@ stream_open (const char *file, const char *how)
 {
   if (!file)
     return stdout;
-  if (STREQ (file, "-") && *how == 'r')
+  if (*how == 'r')
     {
-      have_read_stdin = true;
-      return stdin;
+      FILE *fp;
+      if (STREQ (file, "-"))
+        {
+          have_read_stdin = true;
+          fp =  stdin;
+        }
+      else
+        fp = fopen (file, how);
+      xfadvise (fileno (fp), 0, 0, XFADV_WILLNEED);
+      return fp;
     }
   return fopen (file, how);
 }
@@ -845,6 +874,7 @@ xfclose (FILE *fp, char const *file)
       break;

     default:
+      xfadvise (fileno (fp), 0, 0, XFADV_DONTNEED);
       if (fclose (fp) != 0)
         die (_("close failed"), file);
       break;
@@ -1044,6 +1074,7 @@ open_temp (const char *name, pid_t pid)
       break;
     }

+  xfadvise (fileno (fp), 0, 0, XFADV_WILLNEED);
   return fp;
 }

@@ -1612,6 +1643,8 @@ fillbuf (struct buffer *buf, FILE *fp, char const
*file)
                 }
             }

+          xfadvise (fileno (fp), 0, ftell (fp), XFADV_DONTNEED);
+
           /* Find and record each line in the just-read input.  */
           while ((p = memchr (ptr, eol, ptrlim - ptr)))
             {
-- 
1.6.3.3