Re: [PATCH] Do not break buffers in fvwrite for unbuffered files

[Corinna Vinschen <vinschen at redhat dot com>]

On Oct 31 10:53, Federico Terraneo wrote:
> Sorry for the delay, I've been quite busy.

No worries.

> Anyway, here's the patch that makes both unbuffered and fully buffered
> writes faster. It writes in multiples of fp->_bf._size, and clamps at
> INT_MAX, as requested.

Thanks for working on this.  I have a few comments inline.

> Here's the changelog entry:
> 
> 2013-10-31  Terraneo Federico  <...>
> 
> 	* libc/stdio/fvwrite.c: Allow writing in multiples of
> 	 fp->_bf._size for fully buffered and unbuffered files,
> 	 to improve write performance.
> [...]
> diff -ruN a/newlib/libc/stdio/fvwrite.c b/newlib/libc/stdio/fvwrite.c
> --- a/newlib/libc/stdio/fvwrite.c	2013-10-31 01:10:06.110679241 +0100
> +++ b/newlib/libc/stdio/fvwrite.c	2013-10-31 09:25:04.543332198 +0100
> @@ -21,6 +21,7 @@
>  #include <string.h>
>  #include <stdlib.h>
>  #include <errno.h>
> +#include <limits.h>
>  #include "local.h"
>  #include "fvwrite.h"
>  
> @@ -89,12 +90,16 @@
>    if (fp->_flags & __SNBF)
>      {
>        /*
> -       * Unbuffered: write up to BUFSIZ bytes at a time.
> +       * Unbuffered: split the buffer in the larger chunk that
> +       * is both less than INT_MAX, as some legacy code may
> +       * expect int instead of size_t, and is a multiple of
> +       * fp->_bf._size.
>         */
>        do
>  	{
>  	  GETIOV (;);
> -	  w = fp->_write (ptr, fp->_cookie, p, MIN (len, BUFSIZ));
> +	  w = fp->_write (ptr, fp->_cookie, p,
> +	    ((int)MIN(len, INT_MAX)) / fp->_bf._size * fp->_bf._size);

What if len is < fp->_bf._size?  AFAICS, this would result in writing
0 bytes.

>  	  if (w <= 0)
>  	    goto err;
>  	  p += w;
> @@ -177,30 +182,24 @@
>  	      fp->_p += w;
>  	      w = len;		/* but pretend copied all */
>  	    }
> -	  else if (fp->_p > fp->_bf._base && len > w)
> +	  else if (fp->_p > fp->_bf._base || len < fp->_bf._size)
>  	    {
> -	      /* fill and flush */
> +	      /* pass through the buffer */
> +	      w = MIN(len, w);
>  	      COPY (w);
> -	      /* fp->_w -= w; *//* unneeded */
> +	      fp->_w -= w;
>  	      fp->_p += w;
> -	      if (_fflush_r (ptr, fp))
> +	      if(len > w) if(_fflush_r (ptr, fp))

This looks weird.  Shouldn't that be

              if (len > w && _fflush_r (ptr, fp))

or, for more clearness

              if (fp->_w == 0 && _fflush_r (ptr, fp))

?   Here's why:

The original test was performed before changing the buffer.  By testing
len > w, it made sure to fill the buffer.  Now the test is performed after
changing the buffer, so it can precisely test if the buffer is full.  In
other words, check for fp->_w == 0.  Testing len > w is kind of circuitous
at this point, isn't it?

>  		goto err;
>  	    }
> -	  else if (len >= (w = fp->_bf._size))
> +	  else
>  	    {
>  	      /* write directly */
> +	      w = (int)MIN(len, INT_MAX) / fp->_bf._size * fp->_bf._size;

Again, what if len < fp->_bf._size?

>  	      w = fp->_write (ptr, fp->_cookie, p, w);
>  	      if (w <= 0)
>  		goto err;
>  	    }
> -	  else
> -	    {
> -	      /* fill and done */
> -	      w = len;
> -	      COPY (w);
> -	      fp->_w -= w;
> -	      fp->_p += w;
> -	    }
>  	  p += w;
>  	  len -= w;
>  	}

Other than that, your formatting is missing a couple of spaces.  Make
that "if (...)" rather than "if(...)" or "MIN (...)" rather than
"MIN(...)".


Thanks,
Corinna

-- 
Corinna Vinschen
Cygwin Maintainer
Red Hat

Attachment: pgpSphYxjrXhh.pgp
Description: PGP signature