Re: [PATCH] Custom code int(32|64) => text conversions out of performance reasons

On Monday 01 November 2010 04:04:51 Itagaki Takahiro wrote:
> On Mon, Nov 1, 2010 at 6:41 AM, Andres Freund <andres(at)anarazel(dot)de> wrote:
> > While looking at binary COPY performance I forgot to add BINARY and was a
> > bit shocked to see printf that high in the profile...
> >
> > A change from 9192.476ms 5309.928ms seems to be pretty good indication
> > that a change in that area is waranted given integer columns are quite
> > ubiquous...
>
> Good optimization. Here is the result on my machine:
> * before: 13057.190 ms, 12429.092 ms, 12622.374 ms
> * after: 8261.688 ms, 8427.024 ms, 8622.370 ms
Thanks.

> > * I renamed pg_[il]toa to pg_s(16|32|64)toa - I found the names
> > confusing. Not sure if its worth it.
>
> Agreed, but how about pg_i(16|32|64)toa? 'i' might be more popular than
> 's'. See also
> http://msdn.microsoft.com/en-US/library/yakksftt(VS.100).aspx
I find itoa not as clear about signedness as stoa, but if you insist, I dont
feel strongly about it.

> I have a couple of questions and comments:
>
> * Why did you change "MAXINT8LEN + 1" to "+ 2" ?
> Are there possibility of buffer overflow in the current code?
> @@ -158,12 +159,9 @@ int8out(PG_FUNCTION_ARGS)
> - char buf[MAXINT8LEN + 1];
> + char buf[MAXINT8LEN + 2];
Argh. That should have never gotten into the patch. I was playing around with
another optimization which would have needed more buffer space (but was quite a
bit slower).

> * The buffer reordering seems a bit messy.
> //have to reorder the string, but not 0byte.
> I'd suggest to fill a fixed-size local buffer from right to left
> and copy it to the actual output.
Hm.

while(bufstart < buf){
char swap = *bufstart;
*bufstart++ = *buf;
*buf-- = swap;
}

Is a bit cleaner maybe, but I dont see much point in putting it into its own
function... But again, I don't feel strongly.

> * C++-style comments should be cleaned up.
Will clean up.

Andres