[PATCH] libio/vasprintf.c: Always use realloc.

[PATCH] libio/vasprintf.c: Always use realloc.

[Carlos O'Donell]

The asprintf/vasprintf implementations calls into _IO_vasprintf
to carry out operations on a string-based FILE* object. When
the work is complete and the buffer filled with the result the
implementation null-terminates and returns the string which is
passed back to the caller of asprintf.

During underflow the string-based buffer might have grown, and
conversely the buffer might be exactly the right size, but short
by 1-byte for the NULL terminator. Therefore the implementation
chooses to realloc the buffer to return back any excess memory
or get 1-byte more for the NULL terminator.

In 2000 the implementation was changed by Ulrich Drpper to
make a local deicsion to use realloc vs. malloc/memcpy/free
based on a heuristic and in order to reduce memory fragmentation.
The idea is that if the buffer is only just bigger than what the
user needs the realloc will split the chunk and increase
fragmentation. While this is true, this decision should not be
made at the _IO_vasprintf level. The allocator should be making
this decision based on known fragmentation estimates and a tunable
fragmentation parameter which could be used to control when realloc
splits a chunk and when it decides to malloc/memcpy/free.

Therefore we remove the local heuristic. This should increase
performance slightly at the cost of fragmentation. Future malloc
work to handle fragmentation should be pursued. If someone measures
fragmentation and tracks it back down to _IO_vasprintf, we add a
comment to guide the reader to what we consider is best practice:
enhance realloc to handle fragmentation decisions.

OK to checkin?

No regressions on x86_64.

Cheers,
Carlos.

2015-11-27  Carlos O'Donell  <carlos@redhat.com>

	* libio/vasprintf.c (_IO_vasprintf): Always use realloc.

diff --git a/libio/vasprintf.c b/libio/vasprintf.c
index 61cdfdd..5b39ee7 100644
--- a/libio/vasprintf.c
+++ b/libio/vasprintf.c
@@ -41,7 +41,6 @@ _IO_vasprintf (char **result_ptr, const char *format, _IO_va_list args)
   _IO_strfile sf;
   int ret;
   _IO_size_t needed;
-  _IO_size_t allocated;
   /* No need to clear the memory here (unlike for open_memstream) since
      we know we will never seek on the stream.  */
   string = (char *) malloc (init_string_size);
@@ -62,24 +61,18 @@ _IO_vasprintf (char **result_ptr, const char *format, _IO_va_list args)
       free (sf._sbf._f._IO_buf_base);
       return ret;
     }
-  /* Only use realloc if the size we need is of the same (binary)
-     order of magnitude then the memory we allocated.  */
   needed = sf._sbf._f._IO_write_ptr - sf._sbf._f._IO_write_base + 1;
-  allocated = sf._sbf._f._IO_write_end - sf._sbf._f._IO_write_base;
-  if ((allocated >> 1) <= needed)
-    *result_ptr = (char *) realloc (sf._sbf._f._IO_buf_base, needed);
-  else
-    {
-      *result_ptr = (char *) malloc (needed);
-      if (*result_ptr != NULL)
-	{
-	  memcpy (*result_ptr, sf._sbf._f._IO_buf_base, needed - 1);
-	  free (sf._sbf._f._IO_buf_base);
-	}
-      else
-	/* We have no choice, use the buffer we already have.  */
-	*result_ptr = (char *) realloc (sf._sbf._f._IO_buf_base, needed);
-    }
+  /* We are either shrinking the buffer, doing nothing, or growing
+     the buffer by 1 byte for the NULL.  In all of these cases it's
+     fastest to call realloc.  At the cost of more memory we could
+     choose not to realloc and return a larger buffer, but tuning
+     that requires some thought.  At one point this code was expanded
+     to consider realloc vs. malloc/memcpy/free as a method for
+     reducing fragmentation caused by realloc, but was later removed
+     because such decisions are tuning parameters that should be part
+     of the realloc decisions made by malloc e.g. fragmentation vs.
+     making a copy.  */
+  *result_ptr = (char *) realloc (sf._sbf._f._IO_buf_base, needed);
   if (*result_ptr == NULL)
     *result_ptr = sf._sbf._f._IO_buf_base;
   (*result_ptr)[needed - 1] = '\0';

Re: [PATCH] libio/vasprintf.c: Always use realloc.

[Carlos O'Donell]

On 11/27/2015 10:03 AM, Carlos O'Donell wrote:
> The asprintf/vasprintf implementations calls into _IO_vasprintf
> to carry out operations on a string-based FILE* object. When
> the work is complete and the buffer filled with the result the
> implementation null-terminates and returns the string which is
> passed back to the caller of asprintf.
> 
> During underflow the string-based buffer might have grown, and
> conversely the buffer might be exactly the right size, but short
> by 1-byte for the NULL terminator. Therefore the implementation
> chooses to realloc the buffer to return back any excess memory
> or get 1-byte more for the NULL terminator.
> 
> In 2000 the implementation was changed by Ulrich Drpper to
> make a local deicsion to use realloc vs. malloc/memcpy/free
> based on a heuristic and in order to reduce memory fragmentation.
> The idea is that if the buffer is only just bigger than what the
> user needs the realloc will split the chunk and increase
> fragmentation. While this is true, this decision should not be
> made at the _IO_vasprintf level. The allocator should be making
> this decision based on known fragmentation estimates and a tunable
> fragmentation parameter which could be used to control when realloc
> splits a chunk and when it decides to malloc/memcpy/free.
> 
> Therefore we remove the local heuristic. This should increase
> performance slightly at the cost of fragmentation. Future malloc
> work to handle fragmentation should be pursued. If someone measures
> fragmentation and tracks it back down to _IO_vasprintf, we add a
> comment to guide the reader to what we consider is best practice:
> enhance realloc to handle fragmentation decisions.
> 
> OK to checkin?
> 
> No regressions on x86_64.

I'm going to drop this until I can get a benchtest to show this
is better than before.

c.

Re: [PATCH] libio/vasprintf.c: Always use realloc.

[Roland McGrath]

> I'm going to drop this until I can get a benchtest to show this
> is better than before.

IMHO if benchmarks simply show it's not clearly worse, then you should go
ahead because it simplifies the code.  If the motivation for a change is
not performance alone, then it doesn't need to demonstrate a performance
improvement per se.

Re: [PATCH] libio/vasprintf.c: Always use realloc.

[Carlos O'Donell]

On 03/04/2016 01:42 PM, Roland McGrath wrote:
>> I'm going to drop this until I can get a benchtest to show this
>> is better than before.
> 
> IMHO if benchmarks simply show it's not clearly worse, then you should go
> ahead because it simplifies the code.  If the motivation for a change is
> not performance alone, then it doesn't need to demonstrate a performance
> improvement per se.

It's hard to tell. I'd like to prove it to myself that it's generating
better code.

c.