On Mon, Sep 23, 2019 at 01:16:36PM +0100, Andrew Gierth wrote: > >>>>> "David" == David Fetter <da...@fetter.org> writes: > > David> + return pg_ltostr_zeropad(str, (uint32)0 - (uint32)value, minwidth - > 1); > > No, this is just reintroducing the undefined behavior again. Once the > value has been converted to unsigned you can't cast it back to signed or > pass it to a function expecting a signed value, since it will overflow > in the INT_MIN case. (and in this example would probably output '-' > signs until it ran off the end of memory). > > Here's how I would do it: > > char * > pg_ltostr_zeropad(char *str, int32 value, int32 minwidth) > { > int32 len; > uint32 uvalue = value; > > Assert(minwidth > 0); > > if (value >= 0) > { > if (value < 100 && minwidth == 2) /* Short cut for common case > */ > { > memcpy(str, DIGIT_TABLE + value*2, 2); > return str + 2; > } > } > else > { > *str++ = '-'; > minwidth -= 1; > uvalue = (uint32)0 - uvalue; > } > > len = pg_ultoa_n(uvalue, str); > if (len >= minwidth) > return str + len; > > memmove(str + minwidth - len, str, len); > memset(str, '0', minwidth - len); > return str + minwidth; > }
Done pretty much that way. > David> pg_ltostr(char *str, int32 value) > David> + int32 len = pg_ultoa_n(value, str); > David> + return str + len; > > This seems to have lost its handling of negative numbers entirely Given the comment that precedes it and all the use cases in the code, I changed the signature to take an unsigned integer instead. It's pretty clear that the intent was to add digits and only digits to the passed-in string. > (which doesn't say much for the regression test coverage) I didn't see any obvious way to test functions not surfaced to SQL. Should we have one? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
>From a0eecd74d1dd3f6a4bb1a07f41740c4f6d34ceac Mon Sep 17 00:00:00 2001 From: David Fetter <da...@fetter.org> Date: Sun, 15 Sep 2019 00:06:29 -0700 Subject: [PATCH v12] Make int4 and int8 operations more efficent To: hackers MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="------------2.21.0" This is a multi-part message in MIME format. --------------2.21.0 Content-Type: text/plain; charset=UTF-8; format=fixed Content-Transfer-Encoding: 8bit - Output routines now do more digits per iteration, and - Code determines the number of decimal digits in int4/int8 efficiently - Split off pg_ltoa_n from pg_ltoa - Use same to make other functions shorter diff --git a/src/backend/access/common/printsimple.c b/src/backend/access/common/printsimple.c index 651ade14dd..5c5b6d33b2 100644 --- a/src/backend/access/common/printsimple.c +++ b/src/backend/access/common/printsimple.c @@ -112,7 +112,7 @@ printsimple(TupleTableSlot *slot, DestReceiver *self) case INT8OID: { int64 num = DatumGetInt64(value); - char str[23]; /* sign, 21 digits and '\0' */ + char str[MAXINT8LEN + 1]; pg_lltoa(num, str); pq_sendcountedtext(&buf, str, strlen(str), false); diff --git a/src/backend/utils/adt/int8.c b/src/backend/utils/adt/int8.c index 0ff9394a2f..6230807906 100644 --- a/src/backend/utils/adt/int8.c +++ b/src/backend/utils/adt/int8.c @@ -27,8 +27,6 @@ #include "utils/builtins.h" -#define MAXINT8LEN 25 - typedef struct { int64 current; diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c index 70138feb29..5dfbe8dfcd 100644 --- a/src/backend/utils/adt/numutils.c +++ b/src/backend/utils/adt/numutils.c @@ -20,6 +20,68 @@ #include "common/int.h" #include "utils/builtins.h" +#include "port/pg_bitutils.h" + +/* + * A table of all two-digit numbers. This is used to speed up decimal digit + * generation by copying pairs of digits into the final output. + */ +static const char DIGIT_TABLE[200] = +"00" "01" "02" "03" "04" "05" "06" "07" "08" "09" +"10" "11" "12" "13" "14" "15" "16" "17" "18" "19" +"20" "21" "22" "23" "24" "25" "26" "27" "28" "29" +"30" "31" "32" "33" "34" "35" "36" "37" "38" "39" +"40" "41" "42" "43" "44" "45" "46" "47" "48" "49" +"50" "51" "52" "53" "54" "55" "56" "57" "58" "59" +"60" "61" "62" "63" "64" "65" "66" "67" "68" "69" +"70" "71" "72" "73" "74" "75" "76" "77" "78" "79" +"80" "81" "82" "83" "84" "85" "86" "87" "88" "89" +"90" "91" "92" "93" "94" "95" "96" "97" "98" "99"; + +/* + * Adapted from http://graphics.stanford.edu/~seander/bithacks.html#IntegerLog10 + */ +static inline int +decimalLength32(const uint32 v) +{ + int t; + static uint32 PowersOfTen[] = + {1, 10, 100, + 1000, 10000, 100000, + 1000000, 10000000, 100000000, + 1000000000}; + /* + * Compute base-10 logarithm by dividing the base-2 logarithm + * by a good-enough approximation of the base-2 logarithm of 10 + */ + t = (pg_leftmost_one_pos32(v) + 1)*1233/4096; + return t + (v >= PowersOfTen[t]); +} + +static inline int +decimalLength64(const uint64 v) +{ + int t; + static uint64 PowersOfTen[] = { + UINT64CONST(1), UINT64CONST(10), + UINT64CONST(100), UINT64CONST(1000), + UINT64CONST(10000), UINT64CONST(100000), + UINT64CONST(1000000), UINT64CONST(10000000), + UINT64CONST(100000000), UINT64CONST(1000000000), + UINT64CONST(10000000000), UINT64CONST(100000000000), + UINT64CONST(1000000000000), UINT64CONST(10000000000000), + UINT64CONST(100000000000000), UINT64CONST(1000000000000000), + UINT64CONST(10000000000000000), UINT64CONST(100000000000000000), + UINT64CONST(1000000000000000000), UINT64CONST(10000000000000000000) + }; + + /* + * Compute base-10 logarithm by dividing the base-2 logarithm + * by a good-enough approximation of the base-2 logarithm of 10 + */ + t = (pg_leftmost_one_pos64(v) + 1)*1233/4096; + return t + (v >= PowersOfTen[t]); +} /* * pg_atoi: convert string to integer @@ -276,111 +338,191 @@ pg_itoa(int16 i, char *a) } /* - * pg_ltoa: converts a signed 32-bit integer to its string representation + * pg_ultoa_n: converts an unsigned 32-bit integer to its string representation, + * not NUL-terminated, and returns the length of that string representation * - * Caller must ensure that 'a' points to enough memory to hold the result - * (at least 12 bytes, counting a leading sign and trailing NUL). + * Caller must ensure that 'a' points to enough memory to hold the result (at + * least 10 bytes) + */ +int +pg_ultoa_n(uint32 value, char *a) +{ + int olength, i = 0; + + /* Degenerate case */ + if (value == 0) + { + *a = '0'; + return 1; + } + + olength = decimalLength32(value); + + /* Compute the result string. */ + while (value >= 10000) + { + const uint32 c = value - 10000 * (value / 10000); + const uint32 c0 = (c % 100) << 1; + const uint32 c1 = (c / 100) << 1; + + char *pos = a + olength - i; + + value /= 10000; + + memcpy(pos - 2, DIGIT_TABLE + c0, 2); + memcpy(pos - 4, DIGIT_TABLE + c1, 2); + i += 4; + } + if (value >= 100) + { + const uint32 c = (value % 100) << 1; + + char *pos = a + olength - i; + + value /= 100; + + memcpy(pos - 2, DIGIT_TABLE + c, 2); + i += 2; + } + if (value >= 10) + { + const uint32 c = value << 1; + + char *pos = a + olength - i; + memcpy(pos - 2, DIGIT_TABLE + c, 2); + } + else + { + *a = (char) ('0' + value); + } + + return olength; +} + +/* + * NUL-terminate the output of pg_ultoa_n. + * + * It is the caller's responsibility to ensure that a is at least 12 bytes long, + * which is enough room to hold a minus sign, a maximally long int32, and the + * above terminating NUL. */ void pg_ltoa(int32 value, char *a) { - char *start = a; - bool neg = false; - /* - * Avoid problems with the most negative integer not being representable - * as a positive integer. - */ - if (value == PG_INT32_MIN) + uint32 uvalue = (uint32)value; + int len; + if (value < 0) { - memcpy(a, "-2147483648", 12); - return; - } - else if (value < 0) - { - value = -value; - neg = true; - } - - /* Compute the result string backwards. */ - do - { - int32 remainder; - int32 oldval = value; - - value /= 10; - remainder = oldval - value * 10; - *a++ = '0' + remainder; - } while (value != 0); - - if (neg) + uvalue = (uint32)0 - uvalue; *a++ = '-'; + } + len = pg_ultoa_n(uvalue, a); + a[len] = '\0'; +} + +/* + * Get the decimal representation, not NUL-terminated, and return the length of + * same. Caller must ensure that a points to at least MAXINT8LEN bytes. + */ +int +pg_ulltoa_n(uint64 value, char *a) +{ + int olength, i = 0; + uint32 value2; + + + /* Degenerate case */ + if (value == 0) + { + *a = '0'; + return 1; + } + + olength = decimalLength64(value); + + /* Compute the result string. */ + while (value >= 100000000) + { + const uint64 q = value / 100000000; + uint32 value2 = (uint32) (value - 100000000 * q); + + const uint32 c = value2 % 10000; + const uint32 d = value2 / 10000; + const uint32 c0 = (c % 100) << 1; + const uint32 c1 = (c / 100) << 1; + const uint32 d0 = (d % 100) << 1; + const uint32 d1 = (d / 100) << 1; + + char *pos = a + olength - i; + + value = q; - /* Add trailing NUL byte, and back up 'a' to the last character. */ - *a-- = '\0'; + memcpy(pos - 2, DIGIT_TABLE + c0, 2); + memcpy(pos - 4, DIGIT_TABLE + c1, 2); + memcpy(pos - 6, DIGIT_TABLE + d0, 2); + memcpy(pos - 8, DIGIT_TABLE + d1, 2); + i += 8; + } + + /* Switch to 32-bit for speed */ + value2 = (uint32) value; + + if (value2 >= 10000) + { + const uint32 c = value2 - 10000 * (value2 / 10000); + const uint32 c0 = (c % 100) << 1; + const uint32 c1 = (c / 100) << 1; + + char *pos = a + olength - i; + + value2 /= 10000; - /* Reverse string. */ - while (start < a) + memcpy(pos - 2, DIGIT_TABLE + c0, 2); + memcpy(pos - 4, DIGIT_TABLE + c1, 2); + i += 4; + } + if (value2 >= 100) + { + const uint32 c = (value2 % 100) << 1; + char *pos = a + olength - i; + + value2 /= 100; + + memcpy(pos - 2, DIGIT_TABLE + c, 2); + i += 2; + } + if (value2 >= 10) { - char swap = *start; + const uint32 c = value2 << 1; + char *pos = a + olength - i; - *start++ = *a; - *a-- = swap; + memcpy(pos - 2, DIGIT_TABLE + c, 2); } + else + *a = (char) ('0' + value2); + + return olength; } /* * pg_lltoa: convert a signed 64-bit integer to its string representation * * Caller must ensure that 'a' points to enough memory to hold the result - * (at least MAXINT8LEN+1 bytes, counting a leading sign and trailing NUL). + * (at least MAXINT8LEN + 1 bytes, counting a leading sign and trailing NUL). */ void pg_lltoa(int64 value, char *a) { - char *start = a; - bool neg = false; - - /* - * Avoid problems with the most negative integer not being representable - * as a positive integer. - */ - if (value == PG_INT64_MIN) + int len; + uint64 uvalue = value; + if (value < 0) { - memcpy(a, "-9223372036854775808", 21); - return; - } - else if (value < 0) - { - value = -value; - neg = true; - } - - /* Compute the result string backwards. */ - do - { - int64 remainder; - int64 oldval = value; - - value /= 10; - remainder = oldval - value * 10; - *a++ = '0' + remainder; - } while (value != 0); - - if (neg) *a++ = '-'; - - /* Add trailing NUL byte, and back up 'a' to the last character. */ - *a-- = '\0'; - - /* Reverse string. */ - while (start < a) - { - char swap = *start; - - *start++ = *a; - *a-- = swap; + uvalue = (uint64)0 - uvalue; } + len = pg_ulltoa_n(uvalue, a); + a[len] = 0; } @@ -409,60 +551,33 @@ pg_lltoa(int64 value, char *a) char * pg_ltostr_zeropad(char *str, int32 value, int32 minwidth) { - char *start = str; - char *end = &str[minwidth]; - int32 num = value; + int len; + uint32 uvalue = value; Assert(minwidth > 0); - /* - * Handle negative numbers in a special way. We can't just write a '-' - * prefix and reverse the sign as that would overflow for INT32_MIN. - */ - if (num < 0) + if (value >= 0) { - *start++ = '-'; + if (value < 100 && minwidth == 2) /* Short cut for common case */ + { + memcpy(str, DIGIT_TABLE + value * 2, 2); + return str + 2; + } + } + else + { + *str++ = '-'; minwidth--; - - /* - * Build the number starting at the last digit. Here remainder will - * be a negative number, so we must reverse the sign before adding '0' - * in order to get the correct ASCII digit. - */ - while (minwidth--) - { - int32 oldval = num; - int32 remainder; - - num /= 10; - remainder = oldval - num * 10; - start[minwidth] = '0' - remainder; - } - } - else - { - /* Build the number starting at the last digit */ - while (minwidth--) - { - int32 oldval = num; - int32 remainder; - - num /= 10; - remainder = oldval - num * 10; - start[minwidth] = '0' + remainder; - } + uvalue = (uint32)0 - uvalue; } - /* - * If minwidth was not high enough to fit the number then num won't have - * been divided down to zero. We punt the problem to pg_ltostr(), which - * will generate a correct answer in the minimum valid width. - */ - if (num != 0) - return pg_ltostr(str, value); + len = pg_ultoa_n(value, str); + if (len >= minwidth) + return str + len; - /* Otherwise, return last output character + 1 */ - return end; + memmove(str + minwidth - len, str, len); + memset(str, '0', minwidth-len); + return str + minwidth; } /* @@ -484,64 +599,10 @@ pg_ltostr_zeropad(char *str, int32 value, int32 minwidth) * result. */ char * -pg_ltostr(char *str, int32 value) +pg_ltostr(char *str, uint32 value) { - char *start; - char *end; - - /* - * Handle negative numbers in a special way. We can't just write a '-' - * prefix and reverse the sign as that would overflow for INT32_MIN. - */ - if (value < 0) - { - *str++ = '-'; - - /* Mark the position we must reverse the string from. */ - start = str; - - /* Compute the result string backwards. */ - do - { - int32 oldval = value; - int32 remainder; - - value /= 10; - remainder = oldval - value * 10; - /* As above, we expect remainder to be negative. */ - *str++ = '0' - remainder; - } while (value != 0); - } - else - { - /* Mark the position we must reverse the string from. */ - start = str; - - /* Compute the result string backwards. */ - do - { - int32 oldval = value; - int32 remainder; - - value /= 10; - remainder = oldval - value * 10; - *str++ = '0' + remainder; - } while (value != 0); - } - - /* Remember the end+1 and back up 'str' to the last character. */ - end = str--; - - /* Reverse string. */ - while (start < str) - { - char swap = *start; - - *start++ = *str; - *str-- = swap; - } - - return end; + int len = pg_ultoa_n(value, str); + return str + len; } /* diff --git a/src/include/utils/builtins.h b/src/include/utils/builtins.h index 937ddb7ef0..7f27cdd80f 100644 --- a/src/include/utils/builtins.h +++ b/src/include/utils/builtins.h @@ -18,6 +18,8 @@ #include "nodes/nodes.h" #include "utils/fmgrprotos.h" +/* Sign + the most decimal digits an 8-byte number could have */ +#define MAXINT8LEN 20 /* bool.c */ extern bool parse_bool(const char *value, bool *result); @@ -46,10 +48,12 @@ extern int32 pg_atoi(const char *s, int size, int c); extern int16 pg_strtoint16(const char *s); extern int32 pg_strtoint32(const char *s); extern void pg_itoa(int16 i, char *a); +int pg_ultoa_n(uint32 l, char *a); +int pg_ulltoa_n(uint64 l, char *a); extern void pg_ltoa(int32 l, char *a); extern void pg_lltoa(int64 ll, char *a); extern char *pg_ltostr_zeropad(char *str, int32 value, int32 minwidth); -extern char *pg_ltostr(char *str, int32 value); +extern char *pg_ltostr(char *str, uint32 value); extern uint64 pg_strtouint64(const char *str, char **endptr, int base); /* oid.c */ --------------2.21.0--