Re: appendStringInfo vs appendStringInfoString

On Sat, Sep 28, 2013 at 9:44 PM, David Rowley <dgrowleyml(at)gmail(dot)com> wrote:

> I did some benchmarking earlier in the week for the new patch which was
> just commited to allow formatting in the log_line_prefix string. In version
> 0.4 of the patch there was some performance regression as I was doing
> appendStringInfo(buf, "%*s", padding, variable); instead of
> appendStringInfoString(buf, variable); This regression was fixed in a later
> version of the patch by only using appendStringInfo when the padding was 0.
>
> More details here:
> http://www.postgresql.org/message-id/CAApHDvreSGYvtXJvqHcXZL8_tXiKKiFXhQyXgqtnQ5Yo=MEfMg@mail.gmail.com
>
> Today I started looking through the entire source tree to look for places
> where appendStringInfo() could be replaced by appendStringInfoString(), I
> now have a patch which is around 100 KB in size which changes a large
> number of these instances.
>
> Example:
>
> diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
> index 3107f9c..d0dea4f 100644
> --- a/src/backend/utils/misc/guc.c
> +++ b/src/backend/utils/misc/guc.c
> @@ -6174,7 +6174,7 @@ flatten_set_variable_args(const char *name, List
> *args)
> A_Const *con;
>
> if (l != list_head(args))
> - appendStringInfo(&buf, ", ");
> + appendStringInfoString(&buf, ", ");
>
>
> I know lots of these places are not really in performance critical parts
> of the code, so it's likely not too big a performance gain in most places,
> though to be honest I didn't pay a great deal of attention to how hot the
> code path might be when I was editing.
>
> I thought I would post here just to test the water on this type of change.
> I personally don't think it makes the code any less readable, but if
> performance is not going to be greatly improved then perhaps people would
> have objections to code churn.
>
> I didn't run any benchmarks on the core code, but I did pull out all the
> stringinfo functions and write my own test application. I also did a trial
> on a new macro which could further improve performance when the string
> being appended in a string constant, although I just wrote this to gather
> opinions about the idea. It is not currently a part of the patch.
>
> In my benchmarks I was just appending a 8 char string constant 1000 times
> onto a stringinfo, I did this 3000 times in my tests. The results were as
> follows:
>
> Results:
> 1. appendStringInfoString in 0.404000 sec
> 2. appendStringInfo with %s in 1.118000 sec
> 3 .appendStringInfo in 1.300000 sec
> 4. appendStringInfoStringConst with in 0.221000 sec
>
>
> You can see that appendStringInfoString() is far faster than
> appendStringInfo() this will be down to appendStringInfo using va_args
> whereas appendStringInfoString() just does a strlen() then memcpy(). Test 4
> bypasses the strlen() by using sizeof() which of course can only be used
> with string constants.
>
> Actual code:
> 1. appendStringInfoString(str, "test1234");
> 2. appendStringInfo(str, "%s", "test1234");
> 3. appendStringInfo(str, "test1234");
> 4. appendStringInfoStringConst(str, "test1234");
>
>
> The macro for test 4 was as follows:
> #define appendStringInfoStringConst(buf, s) appendBinaryStringInfo(buf,
> (s), sizeof(s)-1)
>
> I should say at this point that I ran these benchmarks on windows 7 64bit,
> though my tests for the log_line_prefix patch were all on Linux and saw a
> similar slowdown on appendStringInfo() vs appendStringInfoString().
>
> So let this be the thread to gather opinions on if my 100kb patch which
> replaces appendStringInfo with appendStringInfoString where possible would
> be welcomed by the community. Also would using
> appendStringInfoStringConst() be going 1 step too far?
>
> Regards
>
>
I've attached a the cleanup patch for this. This one just converts
instances of appendStringInfo into appendStringInfoString where
appendStringInfo does no formatting or just has the format "%s".

> David Rowley
>