On Thu, 20 Jul 2023 at 00:56, David Rowley <dgrowle...@gmail.com> wrote:
>
> I noticed that 6fcda9aba added quite a lot of conditions that need to
> be checked before we process a normal decimal integer string. I think
> we could likely do better and code it to assume that most strings will
> be decimal and put that case first rather than last.
That sounds sensible, but ...
> In the attached, I've changed that for the 32-bit version only. A
> more complete patch would need to do the 16 and 64-bit versions too.
>
> Without that, we need to check if the first digit is '0' a total of 3
> times and also check if the 2nd digit is any of 'x', 'X', 'o', 'O',
> 'b' or 'B'.
That's not what I see. For me, the compiler (gcc 11, using either -O2
or -O3) is smart enough to spot that the first part of each of the 3
checks is the same, and it only tests whether the first digit is '0'
once, before immediately jumping to the decimal parsing code. I didn't
test other compilers though, so I can believe that different compilers
might not be so smart.
OTOH, this test in your patch:
+ /* process decimal digits */
+ if (isdigit((unsigned char) ptr[0]) &&
+ (isdigit((unsigned char) ptr[1]) || ptr[1] == '_' || ptr[1]
== '\0' || isspace(ptr[1])))
is doing more work than it needs to, and actually makes things
noticeably worse for me. It needs to do a minimum of 2 isdigit()
checks before it will parse the input as a decimal, whereas before
(for me at least) it just did one simple comparison of ptr[0] against
'0'.
I agree with the principal though. In the attached updated patch, I
replaced that test with a simpler one:
+ /*
+ * Process the number's digits. We optimize for decimal input (expected to
+ * be the most common case) first. Anything that doesn't start with a base
+ * prefix indicator must be decimal.
+ */
+
+ /* process decimal digits */
+ if (likely(ptr[0] != '0' || !isalpha((unsigned char) ptr[1])))
So hopefully any compiler should only need to do the one comparison
against '0' for any non-zero decimal input.
Doing that didn't give any measurable performance improvement for me,
but it did at least not make it noticeably worse, and seems more
likely to generate better code with not-so-smart compilers. I'd be
interested to know how that performs for you (and if your compiler
really does generate 3 "first digit is '0'" tests for the unpatched
code).
Regards,
Dean
---
Here were my test results (where P1 is the "fix_COPY_DEFAULT.patch"),
and I tested COPY loading 50M rows:
HEAD + P1
=========
7137.966 ms
7193.190 ms
7094.491 ms
7123.520 ms
HEAD + P1 + pg_strtoint32_base_10_first.patch
=============================================
7561.658 ms
7548.282 ms
7551.360 ms
7560.671 ms
HEAD + P1 + pg_strtoint32_base_10_first.v2.patch
================================================
7238.775 ms
7184.937 ms
7123.257 ms
7159.618 ms
diff --git a/src/backend/utils/adt/numutils.c b/src/backend/utils/adt/numutils.c
new file mode 100644
index 471fbb7..e2320e6
--- a/src/backend/utils/adt/numutils.c
+++ b/src/backend/utils/adt/numutils.c
@@ -314,113 +314,124 @@ pg_strtoint32_safe(const char *s, Node *
else if (*ptr == '+')
ptr++;
- /* process digits */
- if (ptr[0] == '0' && (ptr[1] == 'x' || ptr[1] == 'X'))
+ /*
+ * Process the number's digits. We optimize for decimal input (expected to
+ * be the most common case) first. Anything that doesn't start with a base
+ * prefix indicator must be decimal.
+ */
+
+ /* process decimal digits */
+ if (likely(ptr[0] != '0' || !isalpha((unsigned char) ptr[1])))
{
- firstdigit = ptr += 2;
+ firstdigit = ptr;
while (*ptr)
{
- if (isxdigit((unsigned char) *ptr))
+ if (isdigit((unsigned char) *ptr))
{
- if (unlikely(tmp > -(PG_INT32_MIN / 16)))
+ if (unlikely(tmp > -(PG_INT32_MIN / 10)))
goto out_of_range;
- tmp = tmp * 16 + hexlookup[(unsigned char) *ptr++];
+ tmp = tmp * 10 + (*ptr++ - '0');
}
else if (*ptr == '_')
{
- /* underscore must be followed by more digits */
+ /* underscore may not be first */
+ if (unlikely(ptr == firstdigit))
+ goto invalid_syntax;
+ /* and it must be followed by more digits */
ptr++;
- if (*ptr == '\0' || !isxdigit((unsigned char) *ptr))
+ if (*ptr == '\0' || !isdigit((unsigned char) *ptr))
goto invalid_syntax;
}
else
break;
}
}
- else if (ptr[0] == '0' && (ptr[1] == 'o' || ptr[1] == 'O'))
+ /* process hex digits */
+ else if (ptr[1] == 'x' || ptr[1] == 'X')
{
firstdigit = ptr += 2;
while (*ptr)
{
- if (*ptr >= '0' && *ptr <= '7')
+ if (isxdigit((unsigned char) *ptr))
{
- if (unlikely(tmp > -(PG_INT32_MIN / 8)))
+ if (unlikely(tmp > -(PG_INT32_MIN / 16)))
goto out_of_range;
- tmp = tmp * 8 + (*ptr++ - '0');
+ tmp = tmp * 16 + hexlookup[(unsigned char) *ptr++];
}
else if (*ptr == '_')
{
/* underscore must be followed by more digits */
ptr++;
- if (*ptr == '\0' || *ptr < '0' || *ptr > '7')
+ if (*ptr == '\0' || !isxdigit((unsigned char) *ptr))
goto invalid_syntax;
}
else
break;
}
}
- else if (ptr[0] == '0' && (ptr[1] == 'b' || ptr[1] == 'B'))
+ /* process octal digits */
+ else if (ptr[1] == 'o' || ptr[1] == 'O')
{
firstdigit = ptr += 2;
while (*ptr)
{
- if (*ptr >= '0' && *ptr <= '1')
+ if (*ptr >= '0' && *ptr <= '7')
{
- if (unlikely(tmp > -(PG_INT32_MIN / 2)))
+ if (unlikely(tmp > -(PG_INT32_MIN / 8)))
goto out_of_range;
- tmp = tmp * 2 + (*ptr++ - '0');
+ tmp = tmp * 8 + (*ptr++ - '0');
}
else if (*ptr == '_')
{
/* underscore must be followed by more digits */
ptr++;
- if (*ptr == '\0' || *ptr < '0' || *ptr > '1')
+ if (*ptr == '\0' || *ptr < '0' || *ptr > '7')
goto invalid_syntax;
}
else
break;
}
}
- else
+ /* process binary digits */
+ else if (ptr[1] == 'b' || ptr[1] == 'B')
{
- firstdigit = ptr;
+ firstdigit = ptr += 2;
while (*ptr)
{
- if (isdigit((unsigned char) *ptr))
+ if (*ptr >= '0' && *ptr <= '1')
{
- if (unlikely(tmp > -(PG_INT32_MIN / 10)))
+ if (unlikely(tmp > -(PG_INT32_MIN / 2)))
goto out_of_range;
- tmp = tmp * 10 + (*ptr++ - '0');
+ tmp = tmp * 2 + (*ptr++ - '0');
}
else if (*ptr == '_')
{
- /* underscore may not be first */
- if (unlikely(ptr == firstdigit))
- goto invalid_syntax;
- /* and it must be followed by more digits */
+ /* underscore must be followed by more digits */
ptr++;
- if (*ptr == '\0' || !isdigit((unsigned char) *ptr))
+ if (*ptr == '\0' || *ptr < '0' || *ptr > '1')
goto invalid_syntax;
}
else
break;
}
}
+ else
+ goto invalid_syntax;
/* require at least one digit */
if (unlikely(ptr == firstdigit))
goto invalid_syntax;
/* allow trailing whitespace, but not other trailing chars */
- while (*ptr != '\0' && isspace((unsigned char) *ptr))
+ while (isspace((unsigned char) *ptr))
ptr++;
if (unlikely(*ptr != '\0'))
