On 18.11.2013 15:40, Marko Kreen wrote:
> On Mon, Nov 18, 2013 at 06:18:01PM +1300, David Rowley wrote:
>> On Mon, Nov 18, 2013 at 1:01 AM, Marko Kreen <markokr@gmail.com> wrote:
>>> I am bit suspicious of performance impact of this patch, but think
>>> that it's still worthwhile as it decreases code style where single
>>> string argument is given to printf-style function without "%s".
>>>
>>>
>> This thread probably did not explain very will the point of this patch.
>> All this kicked up from an earlier patch which added for alignment in the
>> log_line_prefix GUC. After some benchmarks were done on the proposed patch
>> for that, it was discovered that replacing appendStringInfoString with
>> appendStringInfo gave a big enough slowdown to matter in high volume
>> logging scenarios. That patch was revised and the appendStringInfo()'s were
>> only used when they were really needed and performance increased again.
>>
>> I then ran a few benchmarks seen here:
>> http://www.postgresql.org/message-id/CAApHDvp2uLKPuHJnCkonBGG2VXPvxoLOPzhrGXBS-M0r0v4wSA%40mail.gmail.com
>>
>> To compare appendStringInfo(si, "%s", str); with appendStringinfoString(a,
>> str); and appendStringInfo(si, str);
>>
>> The conclusion to those benchmarks were that appendStringInfoString() was
>> the best function to use when no formatting was required, so I submitted a
>> patch which replaced appendStringInfo() with appendStringInfoString() where
>> that was possible and that was accepted.
>>
>> appendPQExpBuffer() and appendPQExpBufferStr are the front end versions of
>> appendStringInfo, so I spent an hour or so replacing these calls too as it
>> should show a similar speedup, though in this case likely the performance
>> is less critical, my thinking was more along the lines of, "it increases
>> performance a little bit with a total of 0 increase in code complexity".
>
> I was actually praising the patch that it reduces complexity,
> so it's worth applying even if there is no performance win.
>
> With performance win, it's doubleplus good.
>
> The patch applies and regtests work fine. So I mark it as
> ready for committer.
Ok, committed.
PS. I'm not 100% convinced that this kind of code churn is worthwhile.
It doesn't make any difference to readability in my eyes, in general. In
some cases it does, but in others it messes with indentation
(describeOneTables in src/bin/psql/describe.c). It also makes
backpatching more laborious. But it's done now, hopefully this is a
one-off thing.
- Heikki
