Thread: appendStringInfoString() micro-opt

This patch replaces a bunch of call sites of appendStringInfo() with
appendStringInfoString(). (For those without the code in front of
you, the difference between these two functions is that the former
takes a sprintf-style format string and a variable list of arguments,
whereas the latter just takes a single NUL-terminated string;
therefore, the latter is faster if you're just appending a single
string to the buffer).

For the sake of minimizing how much of my time I've wasted if someone
objects, this patch only changes a portion of the call sites that
could be changed; I'll do the rest before committing the patch.

I was tempted to make appendStringInfoString() a macro, since (a) it's
just one line of code (b) I'd expect plenty of compilers to be smart
enough to optimize-out a strlen() on a string-literal arg. The
downside is that it would require that appendStringInfoString()
evaluate its arguments more than once. Any comments on whether this is
worth doing?

Unless anyone objects, I intend to apply the full version of this
patch within 48 hours.

-Neil

Neil Conway <neilc@samurai.com> writes:
> This patch replaces a bunch of call sites of appendStringInfo() with
> appendStringInfoString().

I doubt this saves enough cycles to be worth doing, but if it floats
your boat ...

When I'm tempted to make a dubious micro-optimization, I always ask
myself "is it likely that the sum of all machine time saved by this
change will exceed the amount of person-time I am about to put into
making it?"  Given the number of places you're talking about touching,
and the fact that I've never seen appendStringInfo placing high on a
profile, I suspect this doesn't pass that test.

I'm not objecting to your doing it, exactly, just suggesting that there
are better things to spend your time on.

> I was tempted to make appendStringInfoString() a macro, since (a) it's
> just one line of code (b) I'd expect plenty of compilers to be smart
> enough to optimize-out a strlen() on a string-literal arg. The
> downside is that it would require that appendStringInfoString()
> evaluate its arguments more than once. Any comments on whether this is
> worth doing?

This I would object to, since it creates a risk of failure if anyone
is incautious enough to write a non-constant argument to
appendStringInfoString.  As soon as you factor any future debugging
into the equation, the probability that you've made a net savings of
time drops to nil :-(.  You have to have a *very* large payback to
justify putting that kind of booby-trap into the code, and the payback
from this change is not only not large, there's no evidence that it'd
even be measurable.

            regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:
> I'm not objecting to your doing it, exactly, just suggesting that
> there are better things to spend your time on.

Heh, probably true :-)

I'll put this on the back-burner for now, and repost a complete patch
later if I get around to it.

> This I would object to, since it creates a risk of failure if anyone
> is incautious enough to write a non-constant argument to
> appendStringInfoString.

Agreed.

(Semi-OT note: allowing the user to extend the compiler to enforce
rules like this is a cool idea: http://metacomp.stanford.edu/)

-Neil

Tom Lane wrote:
> Neil Conway <neilc@samurai.com> writes:
> > This patch replaces a bunch of call sites of appendStringInfo() with
> > appendStringInfoString().
>
> I doubt this saves enough cycles to be worth doing, but if it floats
> your boat ...
>
> When I'm tempted to make a dubious micro-optimization, I always ask
> myself "is it likely that the sum of all machine time saved by this
> change will exceed the amount of person-time I am about to put into
> making it?"  Given the number of places you're talking about touching,
> and the fact that I've never seen appendStringInfo placing high on a
> profile, I suspect this doesn't pass that test.
>
> I'm not objecting to your doing it, exactly, just suggesting that there
> are better things to spend your time on.

Of course, if it makes the code clearer, that is a win in itself.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom Lane wrote:
>> I'm not objecting to your doing it, exactly, just suggesting that there
>> are better things to spend your time on.

> Of course, if it makes the code clearer, that is a win in itself.

Sure, but I can't see that there's any gain in readability here ...

            regards, tom lane

Neil Conway <neilc@samurai.com> writes:
> I'll put this on the back-burner for now, and repost a complete
> patch later if I get around to it.

I've applied the following patch (since I'd already gone ahead and
done the work) that replaces appendStringInfo(buf, "%s", str) with
appendStringInfoString(buf, str)

It occurred to me that there is a potential security problem with code
like:

char *my_str;
my_str = read_from_an_untrusted_source();
appendStringInfo(buf, my_str);

If my_str contains any formatting characters, this crashes the
backend. I'm not sure if there are any actual exploitable instances of
this in the backend, but the above unsafe coding practise is fairly
common.

-Neil

Neil Conway <neilc@samurai.com> writes:
> It occurred to me that there is a potential security problem with code
> like:

> char *my_str;
> my_str = read_from_an_untrusted_source();
> appendStringInfo(buf, my_str);

> If my_str contains any formatting characters, this crashes the
> backend. I'm not sure if there are any actual exploitable instances of
> this in the backend, but the above unsafe coding practise is fairly
> common.

It is?  I thought I'd gone around and checked for that.  If you see any
remaining cases then I'd say they are must-fix items.

            regards, tom lane