Re: [PATCH 0/6] Some vfprintf refactoring

["Carlos O'Donell" <carlos at redhat dot com>]

On 03/05/2015 03:53 PM, Florian Weimer wrote:
> On 03/05/2015 08:35 PM, Carlos O'Donell wrote:
>> On 03/01/2015 04:55 PM, Florian Weimer wrote:
>>> These patches move around some code in vfprintf, and finally split off
>>> the positional argument handling code from the main vfprintf function.
>>
>> Why? :-)
> 
> The goal is to remove extend_alloca.  Define a local struct
> scratch_buffer variable involves a large stack allocation.  My initial
> version just put a struct scratch_buffer variable definition roughly in
> the same place as the work_buffer array.  But that would increase stack
> usage in all cases, even with positional parameters.  I expected that
> this would raise a red flag during review.
> 
> So I tried to eliminate the unconditional stack allocation of the
> scratch buffer.  First, I tried to move the scratch buffer definition
> after the do_positional label.  While doing that, I noticed the business
> with the jump tables.  It turns out that both the non-positional and
> positional copy of the tables jump to the all_done label.  In the
> positional case, the scratch buffer is out of scope and cannot be freed.
>  So this does not work with the current scratch buffer functions.  I
> started adding a parameter to the jump table code so that I could inject
> cleanup code specific to the positional case, but I stopped when I
> realized what I was doing.  Rather than writing yet another version of
> the append-to-buffer code, I then realized that separating out the
> positional case into a separate function would allow me to customize the
> action when the all_done label is reached, which eventually led to the
> current patch series.
> 
> GCC will inline the new printf_positional function because it is called
> in just one place, so any differences should be just random noise by
> slightly different optimizer choices.  I will try to verify that and
> report back the results.  But due to the huge size of the function, it
> could be very difficult to get a complete picture.  Benchmarking is
> difficult because I don't know what a representative use case of
> positional arguments would be.
> 

Excellent answer, and exactly the kind of detailed summary
I'm expecting in a 0-th entry for a long series of patches.

Always list the shortcomings, or problems, or attempted
solutions. Don't let the reader infer these or need to
consider them and then reject them just as you did.

You want to make it easy for me to accept the patches.

Thanks for taking the time to write this up.

Cheers,
Carlos.