[v2] Fix slow and non-deterministic behavior of isspace() and tolower()
Commit Message
I was getting 8% and 6% cpu usage in tolower() and isspace(),
respectively, waiting for a breakpoint on ppc64el.
Also, gdb doesn't want non-deterministic behavior here.
v2: do not clash with C99 names
---
gdb/utils.c | 27 +++++++++++++++++++++++----
1 file changed, 23 insertions(+), 4 deletions(-)
Comments
Hi,
On 6/10/19 10:30 PM, Shawn Landden wrote:
> I was getting 8% and 6% cpu usage in tolower() and isspace(),
> respectively, waiting for a breakpoint on ppc64el.
>
> Also, gdb doesn't want non-deterministic behavior here.
>
> v2: do not clash with C99 names
When I was working on the C++ wildmatching support a
couple years ago, I had some testcases that would stress name
parsing that I was running under perf, and I also noticed these functions
higher up on the profile. I wrote a few patches back then:
https://github.com/palves/gdb/commits/palves/ada-decode-speedups
And this one has the same idea as yours:
https://github.com/palves/gdb/commit/f701531b79380356134d53db97adb6f467f9d784
So, I agree that this makes sense.
I also agree that we don't want to depend on the current
locale when parsing symbol names.
In my version I was naming the functions gdb_xxx while in
your version you're using xxx_inline. gdb_xxx naming is
more common as a gdb version of some standard function,
so I would prefer that.
But, meanwhile, last year I merged this patch:
<https://sourceware.org/ml/gdb-patches/2018-05/msg00561.html>
which touches a somewhat subject.
That patch uses the existing libiberty uppercase TOLOWER, ISXDIGIT,
etc. macros, which are inline and locale independent by design.
See include/safe-ctype.h. Can we use those instead of adding new
functions? I don't recall if I benchmarked ISSPACE vs the gdb_isspace
in that optimization patch on my github, but I think I just didn't
remember ISSPACE back then.
Least but not least, the patch as is is not following the
GNU/GDB coding format conventions. Take a look here:
https://sourceware.org/gdb/wiki/ContributionChecklist
Thanks,
Pedro Alves
El vie., 21 de jun. de 2019 12:35, Pedro Alves <palves@redhat.com> escribió:
> Hi,
>
> On 6/10/19 10:30 PM, Shawn Landden wrote:
> > I was getting 8% and 6% cpu usage in tolower() and isspace(),
> > respectively, waiting for a breakpoint on ppc64el.
> >
> > Also, gdb doesn't want non-deterministic behavior here.
> >
> > v2: do not clash with C99 names
>
> When I was working on the C++ wildmatching support a
> couple years ago, I had some testcases that would stress name
> parsing that I was running under perf, and I also noticed these functions
> higher up on the profile. I wrote a few patches back then:
>
> https://github.com/palves/gdb/commits/palves/ada-decode-speedups
>
> And this one has the same idea as yours:
>
>
> https://github.com/palves/gdb/commit/f701531b79380356134d53db97adb6f467f9d784
>
> So, I agree that this makes sense.
>
I don't care how it gets fixed, and the GNU coding standard (which I write
to for glibc) will take more time than writing this patch. (Or your
well-documented response) Also, while I have a copyright assignment for
glibc, mine for GCC and binutils-gdb is only pending.
Go ahead and fix this, and give me credit.
Cheers.
>
> I also agree that we don't want to depend on the current
> locale when parsing symbol names.
>
> In my version I was naming the functions gdb_xxx while in
> your version you're using xxx_inline. gdb_xxx naming is
> more common as a gdb version of some standard function,
> so I would prefer that.
>
> But, meanwhile, last year I merged this patch:
> <https://sourceware.org/ml/gdb-patches/2018-05/msg00561.html>
> which touches a somewhat subject.
>
> That patch uses the existing libiberty uppercase TOLOWER, ISXDIGIT,
> etc. macros, which are inline and locale independent by design.
> See include/safe-ctype.h. Can we use those instead of adding new
> functions? I don't recall if I benchmarked ISSPACE vs the gdb_isspace
> in that optimization patch on my github, but I think I just didn't
> remember ISSPACE back then.
>
> Least but not least, the patch as is is not following the
> GNU/GDB coding format conventions. Take a look here:
>
> https://sourceware.org/gdb/wiki/ContributionChecklist
>
> Thanks,
> Pedro Alves
>
On 6/21/19 6:59 PM, Shawn Landden wrote:
>> On 6/10/19 10:30 PM, Shawn Landden wrote:
>>> I was getting 8% and 6% cpu usage in tolower() and isspace(),
>>> respectively, waiting for a breakpoint on ppc64el.
>>>
>>> Also, gdb doesn't want non-deterministic behavior here.
>>>
>>> v2: do not clash with C99 names
>>
>> When I was working on the C++ wildmatching support a
>> couple years ago, I had some testcases that would stress name
>> parsing that I was running under perf, and I also noticed these functions
>> higher up on the profile. I wrote a few patches back then:
>>
>> https://github.com/palves/gdb/commits/palves/ada-decode-speedups
>>
>> And this one has the same idea as yours:
>>
>>
>> https://github.com/palves/gdb/commit/f701531b79380356134d53db97adb6f467f9d784
>>
>> So, I agree that this makes sense.
>>
> I don't care how it gets fixed, and the GNU coding standard (which I write
> to for glibc) will take more time than writing this patch. (Or your
> well-documented response) Also, while I have a copyright assignment for
> glibc, mine for GCC and binutils-gdb is only pending.
>
> Go ahead and fix this, and give me credit.
The interesting thing to do here is
>> That patch uses the existing libiberty uppercase TOLOWER, ISXDIGIT,
>> etc. macros, which are inline and locale independent by design.
>> See include/safe-ctype.h. Can we use those instead of adding new
>> functions? I don't recall if I benchmarked ISSPACE vs the gdb_isspace
>> in that optimization patch on my github, but I think I just didn't
>> remember ISSPACE back then.
How were you benchmarking this?
Thanks,
Pedro Alves
@@ -2626,10 +2626,29 @@ strcmp_iw (const char *string1, const char *string2)
user searches for "foo", then strcmp will sort "foo" before "foo$".
Then lookup_partial_symbol will notice that strcmp_iw("foo$",
"foo") is false, so it won't proceed to the actual match of
"foo(int)" with "foo". */
+/* glibc versions of these have non-deterministic locale-dependant behavior,
+ and are very slow, taking 8% and 6% of total CPU time with some use-cases */
+
+static inline int isspace_inline(int c)
+{
+ return c == ' ' || (unsigned)c-'\t' < 5;
+}
+
+static inline int isupper_inline(int c)
+{
+ return (unsigned)c-'A' < 26;
+}
+
+static inline int tolower_inline(int c)
+{
+ if (isupper(c)) return c | 32;
+ return c;
+}
+
int
strcmp_iw_ordered (const char *string1, const char *string2)
{
const char *saved_string1 = string1, *saved_string2 = string2;
enum case_sensitivity case_pass = case_sensitive_off;
@@ -2641,20 +2660,20 @@ strcmp_iw_ordered (const char *string1, const char *string2)
strings. */
char c1 = 'X', c2 = 'X';
while (*string1 != '\0' && *string2 != '\0')
{
- while (isspace (*string1))
+ while (isspace_inline (*string1))
string1++;
- while (isspace (*string2))
+ while (isspace_inline (*string2))
string2++;
switch (case_pass)
{
case case_sensitive_off:
- c1 = tolower ((unsigned char) *string1);
- c2 = tolower ((unsigned char) *string2);
+ c1 = tolower_inline ((unsigned char) *string1);
+ c2 = tolower_inline ((unsigned char) *string2);
break;
case case_sensitive_on:
c1 = *string1;
c2 = *string2;
break;