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

[Jeff Law <law at redhat dot com>]

On 09/07/2012 02:37 AM, Siddhesh Poyarekar wrote:
> 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.
+  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))

Shouldn't this be written as

else if (was_writing && _IO_switch_to_get_mode (*fp))

That's how I typically see that formatted for GCC.  That'll also cause a 
minor formatting change to the comment and return EOF line.  Similarly 
for the wfileops.c implementation.  If glibc's style guidelines are 
different, then please ignore my comments.

The normal version is quite simple; the wide version is considerably 
more complex.  Did you do any additional testing on the wide version 
beyond what's already in the glibc testsuite?

NEWS file will need 5298 added to the list of bugs fixed.

In fileops.c:

+	  /* 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);

I had to look at that several times.  The subtraction of _read_end from 
_write_base certainly looks odd.  You do something similar in 
wfileops.c.  I wasn't able to convince myself it was correct, as long as 
you're confident it's correct, I won't object.



In wfileops.c:

+      if (mode !=0 || !was_writing)

Space between != and 0.


Jeff