Re: Performance improvements for src/port/snprintf.c

Andres Freund <andres@anarazel.de>

From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Alexander Kuzmenkov <a.kuzmenkov@postgrespro.ru>, pgsql-hackers@lists.postgresql.org
Date: 2018-10-03T17:39:36Z
Lists: pgsql-hackers
Hi,

On 2018-10-03 13:18:35 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.de> writes:
> >> - I know it's not new, but is it actually correct to use va_arg(args, int64)
> >> for ATYPE_LONGLONG?
> 
> > Well, the problem with just doing s/int64/long long/g is that the
> > code would then fail on compilers without a "long long" type.
> > We could ifdef our way around that, but I don't think the code would
> > end up prettier.
> 
> I spent a bit more time thinking about that point.  My complaint about
> lack of long long should be moot given that we're now requiring C99.

True, I didn't think of that.


> So the two cases we need to worry about are (1) long long exists and
> is 64 bits, and (2) long long exists and is wider than 64 bits.  In
> case (1) there's nothing actively wrong with the code as it stands.
> In case (2), if we were to fix the problem by s/int64/long long/g,
> the result would be that we'd be doing the arithmetic for all
> integer-to-text conversions in 128 bits, which seems likely to be
> pretty expensive.

Yea, that seems quite undesirable.


> So a "real" fix would probably require having separate versions of
> fmtint for long and long long.  I'm not terribly excited about
> going there.  I can see it happening some day when/if we need to
> use 128-bit math more extensively than today, but I do not think
> that day is close.

Right, that seems a bit off.


> (Are there *any* platforms where "long long" is 128 bits today?)

Not that I'm aware off.


> Having said that, maybe there's a case for changing the type spec
> in only the va_arg() call, and leaving snprintf's related local
> variables as int64.  (Is that what you actually meant?)  Then,
> if long long really is different from int64, at least we have
> predictable truncation behavior after fetching the value, rather
> than undefined behavior while fetching it.

Hm. I guess that'd be a bit better, but I'm not sure it's worth it. How
about we simply add a static assert that long long isn't bigger than
int64?

Greetings,

Andres Freund


Commits

  1. Improve snprintf.c's handling of NaN, Infinity, and minus zero.

  2. Rationalize snprintf.c's handling of "ll" formats.

  3. Provide fast path in snprintf.c for conversion specs that are just "%s".

  4. Make assorted performance improvements in snprintf.c.

  5. Set snprintf.c's maximum number of NL arguments to be 31.

  6. Always use our own versions of *printf().