On Monday 01 November 2010 04:04:51 Itagaki Takahiro wrote: > On Mon, Nov 1, 2010 at 6:41 AM, Andres Freund <and...@anarazel.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
