Re: [PATCH v5 08/11] libio: Convert __vasprintf_internal to buffers

[Florian Weimer <fweimer@redhat.com>]

* Adhemerval Zanella Netto:

> On 12/12/22 12:23, Florian Weimer via Libc-alpha wrote:
>> The buffer resizing algorithm is slightly different.  The initial
>> buffer is on the stack, and small buffers are directly allocated
>> on the heap using the exact required size.  The overhead of the
>> additional copy is compensated by the lowered setup cost for buffers
>> compared to libio streams.
>
> Patch look good, some comments below.

Thanks.

>> +struct __printf_buffer_asprintf
>> +{
>> +  /* base.write_base points either to a heap-allocated buffer, or to
>> +     the direct array below.  */
>> +  struct __printf_buffer base;
>> +
>> +  /* Initial allocation.  200 should be large enough to copy almost
>> +     all asprintf usages with just a single (final, correctly sized)
>> +     heap allocation.  */
>> +  char direct[200];
>> +};
>
> There are couple or more buffer sizes scattered in this patchset, maybe it
> would be better to consolidate them in one place so it can be easily tuned.

I see what I can do.

> For instance, since it is used with memcpy below, maybe it would be better
> to use a size that compiler can inline (it would be arch-dependent, but 
> using a size that most usual architecture inline is a net gain).

It would need additional optimization hints for GCC, I believe.

>> +void
>> +__printf_buffer_flush_asprintf (struct __printf_buffer_asprintf *buf)
>> +{
>> +  size_t current_pos = buf->base.write_ptr - buf->base.write_base;
>> +  if (current_pos >= INT_MAX)
>
> Shouldn't it be SSIZE_MAX?

Nope, asprintf has the same incorrect return type as printf. 8-(

>
>> +    {
>> +      /* The result is not representable.  No need to continue.  */
>> +      __set_errno (EOVERFLOW);
>> +      __printf_buffer_mark_failed (&buf->base);
>> +      return;
>> +    }
>> +
>> +  size_t current_size = buf->base.write_end - buf->base.write_base;
>> +  /* Implement an exponentiatial sizing policy.  Keep the size
>
> s/exponentiatial/exponential

Fixed.

>> +     congruent 8 (mod 16), to account for the footer in glibc
>> +     malloc.  */
>> +  size_t new_size = ((current_size + current_size / 2) & -15) | 8;
>
> Maybe use ALIGN_UP ((current_size + current_size / 2), 16) here?

Changed.

Thanks,
Florian