[v2] Fix slow and non-deterministic behavior of isspace() and tolower()

Message ID 20190610213017.2021-1-shawn@git.icu
State New
Headers show
Series
  • [v2] Fix slow and non-deterministic behavior of isspace() and tolower()
Related show

Commit Message

Shawn Landden June 10, 2019, 9:30 p.m.
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(-)

-- 
2.20.1

Comments

Pedro Alves June 21, 2019, 5:35 p.m. | #1
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
Shawn Landden June 21, 2019, 5:59 p.m. | #2
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

>
Pedro Alves June 21, 2019, 6:08 p.m. | #3
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

Patch

diff --git a/gdb/utils.c b/gdb/utils.c
index 9686927473..0b68fabe4d 100644
--- a/gdb/utils.c
+++ b/gdb/utils.c
@@ -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;