[PATCH][BZ #5298] Don't flush write buffer for ftell

[Siddhesh Poyarekar <siddhesh at redhat dot com>]

Hi,

The current implementation of ftell is basically equivalent to
fseek(fp, 0, SEEK_CUR). While this is not incorrect, it results in
inheritance of limitations of fseek, which is summarized in the
following comment in the source:

  /* Flush unwritten characters.
     (This may do an unneeded write if we seek within the buffer.
     But to be able to switch to reading, we would need to set
     egptr to ptr.  That can't be done in the current design,
     which assumes file_ptr() is eGptr.  Anyway, since we probably
     end up flushing when we close(), it doesn't make much difference.)
     FIXME: simulate mem-papped files. */

This is not needed for ftell since it does not need to set or
modify buffer state, so this flush can be avoided. Attached patch
computes current position for ftell (within the file_seekoff functions
as a special case) without flushing the buffers when in write mode. I
have used a modified version of the sample program in the bz (appended
to this email) to check the improvement in performance in each call and
the average reads as below on my Fedora 16 x86_64 core i5 laptop with
4GB RAM:

Without patch:
Total time: 9174470.000000 ns. AVG 1819.609282 ns per call

With patch:
Total time: 1047375.000000 ns. AVG 207.730067 ns per call

I have verified that the change does not cause any regressions in the
testsuite.

Regards,
Siddhesh

ChangeLog:

	[BZ #5298]
	* libio/fileops.c (_IO_new_file_seekoff): Don't flush buffer
	for ftell.  Compute offsets from write pointers instead.
	* libio/wfileops.c (_IO_wfile_seekoff): Likewise.

Test program: Run as follows:

./a.out 1 1

to write 1MB and do an ftell with each write.


#include <stdlib.h>
#include <stdio.h>
#include <time.h>
#include <string.h>

#define MEGABYTE 1048576

int main(int argc, char *argv[]) {
  int ret;
  char filename[512];
  char *data;
  int dsize, size, chunksize, count;
  FILE *myfile;
  double total = 0.0;

  int ftell_on = 0;
  chunksize = 208;

  //===============================================================
  // Process arguments
  //   <output_file>
  //   <write_size>
  //   <0/1>
  //===============================================================
  char *usage_str = "Usage arguments : <output_file> <file_size> <0/1> \n";
  if (argc < 3) {
    printf ("Invalid arguments : \n");
    printf ("\t %s \n", usage_str);
    return 1;
  }

  sscanf(argv[1],"%s",filename);
  sscanf(argv[2],"%d",&dsize);
  sscanf(argv[3],"%d",&ftell_on);

  dsize *= MEGABYTE;

  data=malloc(sizeof(char) * dsize);

  if(!data) {
    perror("malloc() failed");
    abort;
  }

  myfile = fopen(filename,"w");

  if(myfile == NULL) {
    perror("fopen failed()");
    abort;
  }

  int i = 0;
  for(count=0; count<dsize; count+=chunksize, i++) {
    if(count+chunksize <= dsize) {
      size = chunksize;
    }
    else {
      size = dsize-count;
    }

    ret = fwrite(data,size,1,myfile);

    if(ret != 1) {
      perror("Write failed\n");
      abort();
    }

   if (ftell_on == 1) {
     int pos;
     double elapsed;
     struct timespec before, after;
     memset (&before, 0, sizeof(before));
     memset (&after, 0, sizeof(after));
     clock_gettime (CLOCK_MONOTONIC_RAW, &before);
     pos = ftell(myfile);
     clock_gettime (CLOCK_MONOTONIC_RAW, &after);
     if (pos < 0) {
        perror("ftell() failed");
        abort();

     }
     elapsed = (after.tv_sec - before.tv_sec) * 1000000000;
     elapsed += (after.tv_nsec - before.tv_nsec);
     total += elapsed;
     printf ("%d: %lf ns\n", i, elapsed);
   }
  }
  printf ("Total time: %lf ns. AVG %f ns per call\n", total, total / i);
  ret = fclose(myfile);

}

diff --git a/libio/fileops.c b/libio/fileops.c
index 43973c5..4968a20 100644
--- a/libio/fileops.c
+++ b/libio/fileops.c
@@ -978,19 +978,20 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
   int must_be_exact = (fp->_IO_read_base == fp->_IO_read_end
 		       && fp->_IO_write_base == fp->_IO_write_ptr);
 
+  bool was_writing = (fp->_IO_write_ptr > fp->_IO_write_base
+		      || _IO_in_put_mode (fp));
+
   if (mode == 0)
     dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
-
-  /* Flush unwritten characters.
-     (This may do an unneeded write if we seek within the buffer.
-     But to be able to switch to reading, we would need to set
-     egptr to ptr.  That can't be done in the current design,
-     which assumes file_ptr() is eGptr.  Anyway, since we probably
-     end up flushing when we close(), it doesn't make much difference.)
-     FIXME: simulate mem-papped files. */
-
-  if (fp->_IO_write_ptr > fp->_IO_write_base || _IO_in_put_mode (fp))
-    if (_IO_switch_to_get_mode (fp))
+  else
+    /* Flush unwritten characters.
+       (This may do an unneeded write if we seek within the buffer.
+       But to be able to switch to reading, we would need to set
+       egptr to ptr.  That can't be done in the current design,
+       which assumes file_ptr() is eGptr.  Anyway, since we probably
+       end up flushing when we close(), it doesn't make much difference.)
+       FIXME: simulate mem-mapped files. */
+    if (was_writing && _IO_switch_to_get_mode (fp))
       return EOF;
 
   if (fp->_IO_buf_base == NULL)
@@ -1010,7 +1011,18 @@ _IO_new_file_seekoff (fp, offset, dir, mode)
     {
     case _IO_seek_cur:
       /* Adjust for read-ahead (bytes is buffer). */
-      offset -= fp->_IO_read_end - fp->_IO_read_ptr;
+      if (mode != 0 || !was_writing)
+	offset -= fp->_IO_read_end - fp->_IO_read_ptr;
+      else
+	{
+	  /* We're doing ftell and we're in write mode.  Add the unflushed
+	     buffer contents to the file position and the amount by which the
+	     file offset would have moved if the write had flushed.  The
+	     latter is usually non-positive.  */
+	  offset += ((fp->_IO_write_ptr - fp->_IO_write_base)
+		     + fp->_IO_write_base - fp->_IO_read_end);
+	}
+
       if (fp->_offset == _IO_pos_BAD)
 	{
 	  if (mode != 0)
diff --git a/libio/wfileops.c b/libio/wfileops.c
index b790029..9082b6c 100644
--- a/libio/wfileops.c
+++ b/libio/wfileops.c
@@ -562,6 +562,10 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
 		       && (fp->_wide_data->_IO_write_base
 			   == fp->_wide_data->_IO_write_ptr));
 
+  bool was_writing = ((fp->_wide_data->_IO_write_ptr
+		       > fp->_wide_data->_IO_write_base)
+		      || _IO_in_put_mode (fp));
+
   if (mode == 0)
     {
       /* XXX For wide stream with backup store it is not very
@@ -585,18 +589,16 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
 
       dir = _IO_seek_cur, offset = 0; /* Don't move any pointers. */
     }
-
-  /* Flush unwritten characters.
-     (This may do an unneeded write if we seek within the buffer.
-     But to be able to switch to reading, we would need to set
-     egptr to pptr.  That can't be done in the current design,
-     which assumes file_ptr() is eGptr.  Anyway, since we probably
-     end up flushing when we close(), it doesn't make much difference.)
-     FIXME: simulate mem-mapped files. */
-
-  if (fp->_wide_data->_IO_write_ptr > fp->_wide_data->_IO_write_base
-      || _IO_in_put_mode (fp))
-    if (_IO_switch_to_wget_mode (fp))
+  else
+    /* Flush unwritten characters.
+       (This may do an unneeded write if we seek within the buffer.
+       But to be able to switch to reading, we would need to set
+       egptr to pptr.  That can't be done in the current design,
+       which assumes file_ptr() is eGptr.  Anyway, since we probably
+       end up flushing when we close(), it doesn't make much difference.)
+       FIXME: simulate mem-mapped files. */
+
+    if (was_writing && _IO_switch_to_wget_mode (fp))
       return WEOF;
 
   if (fp->_wide_data->_IO_buf_base == NULL)
@@ -628,29 +630,98 @@ _IO_wfile_seekoff (fp, offset, dir, mode)
       cv = fp->_codecvt;
       clen = (*cv->__codecvt_do_encoding) (cv);
 
-      if (clen > 0)
+      if (mode !=0 || !was_writing)
 	{
-	  offset -= (fp->_wide_data->_IO_read_end
-		     - fp->_wide_data->_IO_read_ptr) * clen;
-	  /* Adjust by readahead in external buffer.  */
-	  offset -= fp->_IO_read_end - fp->_IO_read_ptr;
+	  if (clen > 0)
+	    {
+	      offset -= (fp->_wide_data->_IO_read_end
+			 - fp->_wide_data->_IO_read_ptr) * clen;
+	      /* Adjust by readahead in external buffer.  */
+	      offset -= fp->_IO_read_end - fp->_IO_read_ptr;
+	    }
+	  else
+	    {
+	      int nread;
+
+	    flushed:
+	      delta = fp->_wide_data->_IO_read_ptr - fp->_wide_data->_IO_read_base;
+	      fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;
+	      nread = (*cv->__codecvt_do_length) (cv, &fp->_wide_data->_IO_state,
+						  fp->_IO_read_base,
+						  fp->_IO_read_end, delta);
+	      fp->_IO_read_ptr = fp->_IO_read_base + nread;
+	      fp->_wide_data->_IO_read_end = fp->_wide_data->_IO_read_ptr;
+	      offset -= fp->_IO_read_end - fp->_IO_read_base - nread;
+	    }
 	}
       else
 	{
-	  int nread;
+	  char *new_write_ptr = fp->_IO_write_ptr;
 
-	  delta = fp->_wide_data->_IO_read_ptr - fp->_wide_data->_IO_read_base;
-	  fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;
-	  nread = (*cv->__codecvt_do_length) (cv, &fp->_wide_data->_IO_state,
-					      fp->_IO_read_base,
-					      fp->_IO_read_end, delta);
-	  fp->_IO_read_ptr = fp->_IO_read_base + nread;
-	  fp->_wide_data->_IO_read_end = fp->_wide_data->_IO_read_ptr;
-	  offset -= fp->_IO_read_end - fp->_IO_read_base - nread;
+	  if (clen > 0)
+	    offset += (fp->_wide_data->_IO_write_ptr
+		       - fp->_wide_data->_IO_write_base) / clen;
+	  else
+	    {
+	      enum __codecvt_result status;
+	      delta = (fp->_wide_data->_IO_write_ptr
+		       - fp->_wide_data->_IO_write_base);
+	      const wchar_t *write_base = fp->_wide_data->_IO_write_base;
+
+	      /* FIXME: This actually ends up in two iterations of conversion,
+		 one here and the next when the buffer actually gets flushed.
+		 It may be possible to optimize this in future so that
+		 wdo_write identifies already converted content and does not
+		 redo it.  In any case, this is much better than having to
+		 flush buffers for every ftell.  */
+	      do
+		{
+		  /* Ugh, no point trying to avoid the flush.  Just do it
+		     and go back to how it was with the read mode.  */
+		  if (delta > 0 && new_write_ptr == fp->_IO_buf_end)
+		    {
+		      if (_IO_switch_to_wget_mode (fp))
+			return WEOF;
+		      goto flushed;
+		    }
+
+		  const wchar_t *new_write_base = fp->_wide_data->_IO_write_base;
+		  fp->_wide_data->_IO_state = fp->_wide_data->_IO_last_state;
+		  status = (*cv->__codecvt_do_out) (cv, &fp->_wide_data->_IO_state,
+						    write_base,
+						    write_base + delta,
+						    &new_write_base,
+						    new_write_ptr,
+						    fp->_IO_buf_end,
+						    &new_write_ptr);
+
+		  delta -= new_write_base - write_base;
+
+		  /* If there was an error, then return WEOF.
+		     TODO: set buffer state.  */
+		  if (__builtin_expect (status == __codecvt_error, 0))
+		      return WEOF;
+		}
+	      while (delta > 0);
+	    }
+
+	  offset += ((new_write_ptr - fp->_IO_write_base)
+		     + fp->_IO_write_base - fp->_IO_read_end);
 	}
 
       if (fp->_offset == _IO_pos_BAD)
-	goto dumb;
+	{
+	  if (mode != 0)
+	    goto dumb;
+	  else
+	    {
+	      result = _IO_SYSSEEK (fp, 0, dir);
+	      if (result == EOF)
+		return result;
+	      fp->_offset = result;
+	    }
+	}
+
       /* Make offset absolute, assuming current pointer is file_ptr(). */
       offset += fp->_offset;