Re: appendPQExpBufferVA vs appendStringInfoVA

Tom commited some changes to appendStringInfoVA a few weeks ago which
allows it to return the required buffer size if the current buffer is not
big enough.

On looking at appendPQExpBufferVA I'm thinking it would be nice if it could
make use of the new pvsnprintf function to bring the same potential
performance improvement in to there too. My vision of how
appendPQExpBufferVA would look after the change is pretty much exactly the
same as appendStringInfoVA, which make me think... Why do we even have
appendPQExpBufferVA ? The only reason that I can think of is that it is
used in front end applications and allocates memory differently... Is this
the only reason or is there some other special reason for this function
that I can't think of?

If someone wants to give me some guidance on how or if all this should
re-factored, I'll happily supply a patch.

Regards

David Rowley

David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> Tom commited some changes to appendStringInfoVA a few weeks ago which
> allows it to return the required buffer size if the current buffer is not
> big enough.

> On looking at appendPQExpBufferVA I'm thinking it would be nice if it could
> make use of the new pvsnprintf function to bring the same potential
> performance improvement in to there too.

Uh ... it does contain pretty much the same algorithm now. We can't
simply use pvsnprintf there because exit-on-error is no good for
libpq's purposes, so unless we want to rethink that, a certain
amount of code duplication is unavoidable. But they both understand
about C99 vsnprintf semantics now.

regards, tom lane

On Fri, Nov 1, 2013 at 9:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
>> Tom commited some changes to appendStringInfoVA a few weeks ago which
>> allows it to return the required buffer size if the current buffer is not
>> big enough.
>
>> On looking at appendPQExpBufferVA I'm thinking it would be nice if it could
>> make use of the new pvsnprintf function to bring the same potential
>> performance improvement in to there too.
>
> Uh ... it does contain pretty much the same algorithm now. We can't
> simply use pvsnprintf there because exit-on-error is no good for
> libpq's purposes, so unless we want to rethink that, a certain
> amount of code duplication is unavoidable. But they both understand
> about C99 vsnprintf semantics now.

I have often found it frustrating that we have appendStringInfo* for
the backend and appendPQExpBuffer* for the frontend. It'd be nice to
have one API that could be used in both places, somehow. There seems
to be a lot of interest (including on my part) in writing code that
can be compiled in either environment.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Nov 2, 2013 at 2:27 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> David Rowley <dgrowleyml(at)gmail(dot)com> writes:
> > Tom commited some changes to appendStringInfoVA a few weeks ago which
> > allows it to return the required buffer size if the current buffer is not
> > big enough.
>
> > On looking at appendPQExpBufferVA I'm thinking it would be nice if it
> could
> > make use of the new pvsnprintf function to bring the same potential
> > performance improvement in to there too.
>
> Uh ... it does contain pretty much the same algorithm now. We can't
> simply use pvsnprintf there because exit-on-error is no good for
> libpq's purposes, so unless we want to rethink that, a certain
> amount of code duplication is unavoidable. But they both understand
> about C99 vsnprintf semantics now.
>
>
I only just noticed the changes you made to appendPQExpBufferVA().
I had wondered if making pvsnprintf return int instead of size_t and having
it return -1 if there are problems, then letting the caller deal with
those, but I'm starting to see why you did it the way you did it... There's
also quite a few subtle differences with things like max allocation size
that would have to be dealt with differently I guess.

I'm low on ideas on how to improve things much around here for now, but for
what it's worth, I did create a patch which changes unnecessary calls to
appendPQExpBuffer() into calls to appendPQExpBufferStr() similar to the
recent one for appendStringInfo and appendStringInfoString.

Regards

David Rowley

regards, tom lane
>

On Sun, Nov 3, 2013 at 3:18 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
>
> I'm low on ideas on how to improve things much around here for now, but
> for what it's worth, I did create a patch which changes unnecessary calls
> to appendPQExpBuffer() into calls to appendPQExpBufferStr() similar to the
> recent one for appendStringInfo and appendStringInfoString.
>
>
>
Attached is a re-based version of this.

Regards

David Rowley

> Regards
>
> David Rowley
>
>
>

On Thu, Nov 14, 2013 at 09:33:59PM +1300, David Rowley wrote:
> On Sun, Nov 3, 2013 at 3:18 AM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> > I'm low on ideas on how to improve things much around here for now, but
> > for what it's worth, I did create a patch which changes unnecessary calls
> > to appendPQExpBuffer() into calls to appendPQExpBufferStr() similar to the
> > recent one for appendStringInfo and appendStringInfoString.
> >
> Attached is a re-based version of this.

It does not apply anymore, could you resend it?

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".

--
marko

On Mon, Nov 18, 2013 at 1:01 AM, Marko Kreen <markokr(at)gmail(dot)com> wrote:

> On Thu, Nov 14, 2013 at 09:33:59PM +1300, David Rowley wrote:
> > On Sun, Nov 3, 2013 at 3:18 AM, David Rowley <dgrowleyml(at)gmail(dot)com>
> wrote:
> > > I'm low on ideas on how to improve things much around here for now, but
> > > for what it's worth, I did create a patch which changes unnecessary
> calls
> > > to appendPQExpBuffer() into calls to appendPQExpBufferStr() similar to
> the
> > > recent one for appendStringInfo and appendStringInfoString.
> > >
> > Attached is a re-based version of this.
>
> It does not apply anymore, could you resend it?
>
>
I've attached a re-based version.

> 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@mail.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".

The findings in the benchmarks in the link above also showed that we might
want to look into turning appendStringInfoString into a macro
around appendBinaryStringInfo() to allow us to skip the strlen() call for
string constants... It's unclear at the moment if this would be a good idea
or much of an improvement, so it was left for something to think about for
the future.

Regards

David Rowley

--
> marko
>
>

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(at)gmail(dot)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@mail.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.

> The findings in the benchmarks in the link above also showed that we might
> want to look into turning appendStringInfoString into a macro
> around appendBinaryStringInfo() to allow us to skip the strlen() call for
> string constants... It's unclear at the moment if this would be a good idea
> or much of an improvement, so it was left for something to think about for
> the future.

In any case it should be separate patch. Perhaps applying the same
optimization for all such functions.

--
marko

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(at)gmail(dot)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