appendStringInfo vs appendStringInfoString

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

David Rowley