[1/4] Attribute method inlining

Message ID 20200520174032.9525-2-tromey@adacore.com
State New
Headers show
Series
  • Micro-optimize DWARF partial symbol reading
Related show

Commit Message

Tom Tromey May 20, 2020, 5:40 p.m.
This inlines a couple of methods on struct attribute, improving the
performance of DWARF partial symbol reading.  These methods were
discovered as hot spots using callgrind.

For this patch, and for all the patches in this series, I tested gdb's
performance on three programs:

1. gdb itself -- I built gdb and copied it to /tmp, ensuring that the
   same version was used in all tests.

2. The system libxul.so, the main library of Firefox.  I installed the
   separate debuginfo and ensured that gdb read it.

3. A large-ish Ada program that I happen to have.

I ran gdb 10 times like:

	  /bin/time -f %e \
		    ./gdb/gdb --data-directory ./gdb/data-directory -nx \
		    -iex 'set debug-file-directory /usr/lib/debug' \
		    -batch $X

... where $X was the test executable.  Then I computed the mean time.
This was all done with a standard (-g -O2) build of gdb.

The baseline times were

gdb    1.90
libxul 2.12
Ada    2.61

This patch brings the numbers down to

gdb    1.88
libxul 2.11
Ada    2.60

Not a huge change, but still visible in the results.

gdb/ChangeLog
2020-05-20  Tom Tromey  <tromey@adacore.com>

	* dwarf2/attribute.h (struct attribute) <form_is_ref>: Inline.
	<get_ref_die_offset>: Inline.
	<get_ref_die_offset_complaint>: New method.
	* dwarf2/attribute.c (attribute::form_is_ref): Move to header.
	(attribute::get_ref_die_offset_complaint): Rename from
	get_ref_die_offset.  Just issue complaint.
---
 gdb/ChangeLog          |  9 +++++++++
 gdb/dwarf2/attribute.c | 29 ++---------------------------
 gdb/dwarf2/attribute.h | 25 +++++++++++++++++++++++--
 3 files changed, 34 insertions(+), 29 deletions(-)

-- 
2.21.3

Comments

Tom Tromey May 20, 2020, 7:40 p.m. | #1
>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:


Tom> 2. The system libxul.so, the main library of Firefox.  I installed the
Tom>    separate debuginfo and ensured that gdb read it.

I forgot that this has a .gdb_index, oops :(.  Any improvements seen
here probably come from making CU expansion a bit faster.

Tom
Gary Benson via Gdb-patches May 21, 2020, 1:08 a.m. | #2
Am Mittwoch, 20. Mai 2020, 19:40:44 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> I ran gdb 10 times like:

>

>       /bin/time -f %e \

>             ./gdb/gdb --data-directory ./gdb/data-directory -nx \

>             -iex 'set debug-file-directory /usr/lib/debug' \

>             -batch $X

>

> ... where $X was the test executable.  Then I computed the mean time.

> This was all done with a standard (-g -O2) build of gdb.

>

> The baseline times were

>

> gdb    1.90

> libxul 2.12

> Ada    2.61

>

> This patch brings the numbers down to

>

> gdb    1.88

> libxul 2.11

> Ada    2.60


When I saw this, I thought I could do a similar profiling test on Windows (but
only with gdb itself).

So just: gdb.exe -q -batch gdb.exe

And I was a bit suprised to see that strcmp_iw_ordered (called from
sort_pst_symbols -> std::sort) is in ~24% of the profiling samples.
And only because of the functions isspace and tolower.

So I made a simple test, and added this before strcmp_iw_ordered:

static inline int isspace2 (int c)
{
  return c == 0x20 || (c >= 0x09 && c <= 0x0d);
}
#define isspace isspace2

static inline int tolower2 (int c)
{
  return (c >= 'A' && c <= 'Z') ? c + 0x20 : c;
}
#define tolower tolower2

And the mean time went from 3.7s down to 2.7s.


I'm not saying this is a correct solution, but does strcmp_iw_ordered have to
support anything besides the "C" locale?

Also, are isspace and tolower only on Windows a bottleneck?

(If anyone wants to see them, I can provide some profiler flame-graphs)


Hannes
Gary Benson via Gdb-patches May 21, 2020, 2:26 p.m. | #3
On 5/21/20 2:08 AM, Hannes Domani via Gdb-patches wrote:
>  Am Mittwoch, 20. Mai 2020, 19:40:44 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> 

>> I ran gdb 10 times like:

>>

>>        /bin/time -f %e \

>>              ./gdb/gdb --data-directory ./gdb/data-directory -nx \

>>              -iex 'set debug-file-directory /usr/lib/debug' \

>>              -batch $X

>>

>> ... where $X was the test executable.  Then I computed the mean time.

>> This was all done with a standard (-g -O2) build of gdb.

>>

>> The baseline times were

>>

>> gdb    1.90

>> libxul 2.12

>> Ada    2.61

>>

>> This patch brings the numbers down to

>>

>> gdb    1.88

>> libxul 2.11

>> Ada    2.60

> 

> When I saw this, I thought I could do a similar profiling test on Windows (but

> only with gdb itself).

> 

> So just: gdb.exe -q -batch gdb.exe

> 

> And I was a bit suprised to see that strcmp_iw_ordered (called from

> sort_pst_symbols -> std::sort) is in ~24% of the profiling samples.

> And only because of the functions isspace and tolower.

> 

> So I made a simple test, and added this before strcmp_iw_ordered:

> 

> static inline int isspace2 (int c)

> {

>   return c == 0x20 || (c >= 0x09 && c <= 0x0d);

> }

> #define isspace isspace2

> 

> static inline int tolower2 (int c)

> {

>   return (c >= 'A' && c <= 'Z') ? c + 0x20 : c;

> }

> #define tolower tolower2

> 

> And the mean time went from 3.7s down to 2.7s.

> 

> 

> I'm not saying this is a correct solution, but does strcmp_iw_ordered have to

> support anything besides the "C" locale?

> 

> Also, are isspace and tolower only on Windows a bottleneck?

> 

> (If anyone wants to see them, I can provide some profiler flame-graphs)


There was a patch for this not that long ago.  Let me try to dig it up.

Thanks,
Pedro Alves
Gary Benson via Gdb-patches May 21, 2020, 3:03 p.m. | #4
Am Donnerstag, 21. Mai 2020, 16:26:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 5/21/20 2:08 AM, Hannes Domani via Gdb-patches wrote:

>

> >  Am Mittwoch, 20. Mai 2020, 19:40:44 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >

> >> I ran gdb 10 times like:

> >>

> >>        /bin/time -f %e \

> >>              ./gdb/gdb --data-directory ./gdb/data-directory -nx \

> >>              -iex 'set debug-file-directory /usr/lib/debug' \

> >>              -batch $X

> >>

> >> ... where $X was the test executable.  Then I computed the mean time.

> >> This was all done with a standard (-g -O2) build of gdb.

> >>

> >> The baseline times were

> >>

> >> gdb    1.90

> >> libxul 2.12

> >> Ada    2.61

> >>

> >> This patch brings the numbers down to

> >>

> >> gdb    1.88

> >> libxul 2.11

> >> Ada    2.60

> >

> > When I saw this, I thought I could do a similar profiling test on Windows (but

> > only with gdb itself).

> >

> > So just: gdb.exe -q -batch gdb.exe

> >

> > And I was a bit suprised to see that strcmp_iw_ordered (called from

> > sort_pst_symbols -> std::sort) is in ~24% of the profiling samples.

> > And only because of the functions isspace and tolower.

> >

> > So I made a simple test, and added this before strcmp_iw_ordered:

> >

> > static inline int isspace2 (int c)

> > {

> >   return c == 0x20 || (c >= 0x09 && c <= 0x0d);

> > }

> > #define isspace isspace2

> >

> > static inline int tolower2 (int c)

> > {

> >   return (c >= 'A' && c <= 'Z') ? c + 0x20 : c;

> > }

> > #define tolower tolower2

> >

> > And the mean time went from 3.7s down to 2.7s.

> >

> >

> > I'm not saying this is a correct solution, but does strcmp_iw_ordered have to

> > support anything besides the "C" locale?

> >

> > Also, are isspace and tolower only on Windows a bottleneck?

> >

> > (If anyone wants to see them, I can provide some profiler flame-graphs)

>

>

> There was a patch for this not that long ago.  Let me try to dig it up.


You're right, I found it here:
https://sourceware.org/pipermail/gdb-patches/2019-June/158285.html

So I guess it's not just on Windows that slow.

And you replied that we maybe should use TOLOWER, ISXDIGIT from libiberty
instead:
https://sourceware.org/pipermail/gdb-patches/2019-June/158518.html

I tried to do that, but the safe-ctype.h defines clash with the ones
of chardefs.h:

In file included from C:/src/repos/binutils-gdb.git/gdb/utils.c:62:
c:\src\repos\binutils-gdb.git\include\safe-ctype.h:89: error: "ISALPHA" redefined [-Werror]
   89 | #define ISALPHA(c)  _sch_test(c, _sch_isalpha)
      |
In file included from c:\src\repos\binutils-gdb.git\readline\readline\keymaps.h:35,
                 from c:\src\repos\binutils-gdb.git\readline\readline\readline.h:37,
                 from C:/src/repos/binutils-gdb.git/gdb/utils.c:61:
c:\src\repos\binutils-gdb.git\readline\readline\chardefs.h:91: note: this is the location of the previous definition
   91 | #define ISALPHA(c) (IN_CTYPE_DOMAIN (c) && isalpha ((unsigned char)c))
      |

Not sure how to solve this.


Hannes
Gary Benson via Gdb-patches May 21, 2020, 4:42 p.m. | #5
On 5/21/20 4:03 PM, Hannes Domani via Gdb-patches wrote:
>  Am Donnerstag, 21. Mai 2020, 16:26:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> 

>> On 5/21/20 2:08 AM, Hannes Domani via Gdb-patches wrote:

>>

>>>   Am Mittwoch, 20. Mai 2020, 19:40:44 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

>>>

>>>> I ran gdb 10 times like:

>>>>

>>>>         /bin/time -f %e \

>>>>               ./gdb/gdb --data-directory ./gdb/data-directory -nx \

>>>>               -iex 'set debug-file-directory /usr/lib/debug' \

>>>>               -batch $X

>>>>

>>>> ... where $X was the test executable.  Then I computed the mean time.

>>>> This was all done with a standard (-g -O2) build of gdb.

>>>>

>>>> The baseline times were

>>>>

>>>> gdb    1.90

>>>> libxul 2.12

>>>> Ada    2.61

>>>>

>>>> This patch brings the numbers down to

>>>>

>>>> gdb    1.88

>>>> libxul 2.11

>>>> Ada    2.60

>>>

>>> When I saw this, I thought I could do a similar profiling test on Windows (but

>>> only with gdb itself).

>>>

>>> So just: gdb.exe -q -batch gdb.exe

>>>

>>> And I was a bit suprised to see that strcmp_iw_ordered (called from

>>> sort_pst_symbols -> std::sort) is in ~24% of the profiling samples.

>>> And only because of the functions isspace and tolower.

>>>

>>> So I made a simple test, and added this before strcmp_iw_ordered:

>>>

>>> static inline int isspace2 (int c)

>>> {

>>>    return c == 0x20 || (c >= 0x09 && c <= 0x0d);

>>> }

>>> #define isspace isspace2

>>>

>>> static inline int tolower2 (int c)

>>> {

>>>    return (c >= 'A' && c <= 'Z') ? c + 0x20 : c;

>>> }

>>> #define tolower tolower2

>>>

>>> And the mean time went from 3.7s down to 2.7s.

>>>

>>>

>>> I'm not saying this is a correct solution, but does strcmp_iw_ordered have to

>>> support anything besides the "C" locale?

>>>

>>> Also, are isspace and tolower only on Windows a bottleneck?

>>>

>>> (If anyone wants to see them, I can provide some profiler flame-graphs)

>>

>>

>> There was a patch for this not that long ago.  Let me try to dig it up.

> 

> You're right, I found it here:

> https://sourceware.org/pipermail/gdb-patches/2019-June/158285.html


Yes, that's the one!

> 

> So I guess it's not just on Windows that slow.

> 

> And you replied that we maybe should use TOLOWER, ISXDIGIT from libiberty

> instead:

> https://sourceware.org/pipermail/gdb-patches/2019-June/158518.html


This message is actually older than the patch above -- I wrote the patch
afterwards.

The patch is using the libiberty macros, and avoids the readline clash
you run into.  Could you give it a try?

Thanks,
Pedro Alves

> 

> I tried to do that, but the safe-ctype.h defines clash with the ones

> of chardefs.h:

> 

> In file included from C:/src/repos/binutils-gdb.git/gdb/utils.c:62:

> c:\src\repos\binutils-gdb.git\include\safe-ctype.h:89: error: "ISALPHA" redefined [-Werror]

>    89 | #define ISALPHA(c)  _sch_test(c, _sch_isalpha)

>       |

> In file included from c:\src\repos\binutils-gdb.git\readline\readline\keymaps.h:35,

>                  from c:\src\repos\binutils-gdb.git\readline\readline\readline.h:37,

>                  from C:/src/repos/binutils-gdb.git/gdb/utils.c:61:

> c:\src\repos\binutils-gdb.git\readline\readline\chardefs.h:91: note: this is the location of the previous definition

>    91 | #define ISALPHA(c) (IN_CTYPE_DOMAIN (c) && isalpha ((unsigned char)c))

>       |

> 

> Not sure how to solve this.

>
Gary Benson via Gdb-patches May 21, 2020, 5:18 p.m. | #6
Am Donnerstag, 21. Mai 2020, 18:42:23 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> On 5/21/20 4:03 PM, Hannes Domani via Gdb-patches wrote:

> >  Am Donnerstag, 21. Mai 2020, 16:26:12 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> >

> >> On 5/21/20 2:08 AM, Hannes Domani via Gdb-patches wrote:

> >>

> >>>   Am Mittwoch, 20. Mai 2020, 19:40:44 MESZ hat Tom Tromey <tromey@adacore.com> Folgendes geschrieben:

> >>>

> >>>> I ran gdb 10 times like:

> >>>>

> >>>>         /bin/time -f %e \

> >>>>               ./gdb/gdb --data-directory ./gdb/data-directory -nx \

> >>>>               -iex 'set debug-file-directory /usr/lib/debug' \

> >>>>               -batch $X

> >>>>

> >>>> ... where $X was the test executable.  Then I computed the mean time.

> >>>> This was all done with a standard (-g -O2) build of gdb.

> >>>>

> >>>> The baseline times were

> >>>>

> >>>> gdb    1.90

> >>>> libxul 2.12

> >>>> Ada    2.61

> >>>>

> >>>> This patch brings the numbers down to

> >>>>

> >>>> gdb    1.88

> >>>> libxul 2.11

> >>>> Ada    2.60

> >>>

> >>> When I saw this, I thought I could do a similar profiling test on Windows (but

> >>> only with gdb itself).

> >>>

> >>> So just: gdb.exe -q -batch gdb.exe

> >>>

> >>> And I was a bit suprised to see that strcmp_iw_ordered (called from

> >>> sort_pst_symbols -> std::sort) is in ~24% of the profiling samples.

> >>> And only because of the functions isspace and tolower.

> >>>

> >>> So I made a simple test, and added this before strcmp_iw_ordered:

> >>>

> >>> static inline int isspace2 (int c)

> >>> {

> >>>    return c == 0x20 || (c >= 0x09 && c <= 0x0d);

> >>> }

> >>> #define isspace isspace2

> >>>

> >>> static inline int tolower2 (int c)

> >>> {

> >>>    return (c >= 'A' && c <= 'Z') ? c + 0x20 : c;

> >>> }

> >>> #define tolower tolower2

> >>>

> >>> And the mean time went from 3.7s down to 2.7s.

> >>>

> >>>

> >>> I'm not saying this is a correct solution, but does strcmp_iw_ordered have to

> >>> support anything besides the "C" locale?

> >>>

> >>> Also, are isspace and tolower only on Windows a bottleneck?

> >>>

> >>> (If anyone wants to see them, I can provide some profiler flame-graphs)

> >>

> >>

> >> There was a patch for this not that long ago.  Let me try to dig it up.

> >

> > You're right, I found it here:

> > https://sourceware.org/pipermail/gdb-patches/2019-June/158285.html

>

> Yes, that's the one!

>

> >

> > So I guess it's not just on Windows that slow.

> >

> > And you replied that we maybe should use TOLOWER, ISXDIGIT from libiberty

> > instead:

> > https://sourceware.org/pipermail/gdb-patches/2019-June/158518.html

>

> This message is actually older than the patch above -- I wrote the patch

> afterwards.

>

> The patch is using the libiberty macros, and avoids the readline clash

> you run into.  Could you give it a try?


It wasn't immediately obvious to me, but I think you mean this one:
https://sourceware.org/pipermail/gdb-patches/2019-June/158525.html

I tried it, and as expected, I get the same speedup as with my previous
test, and strcmp_iw_ordered is now in only ~1.5% of the profiling samples.


Hannes
Gary Benson via Gdb-patches May 22, 2020, 3:47 p.m. | #7
On 5/21/20 6:18 PM, Hannes Domani via Gdb-patches wrote:
>  Am Donnerstag, 21. Mai 2020, 18:42:23 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

> 

>> On 5/21/20 4:03 PM, Hannes Domani via Gdb-patches wrote:


>>>> There was a patch for this not that long ago.  Let me try to dig it up.

>>>

>>> You're right, I found it here:

>>> https://sourceware.org/pipermail/gdb-patches/2019-June/158285.html

>>

>> Yes, that's the one!

>>

>>>

>>> So I guess it's not just on Windows that slow.

>>>

>>> And you replied that we maybe should use TOLOWER, ISXDIGIT from libiberty

>>> instead:

>>> https://sourceware.org/pipermail/gdb-patches/2019-June/158518.html

>>

>> This message is actually older than the patch above -- I wrote the patch

>> afterwards.

>>

>> The patch is using the libiberty macros, and avoids the readline clash

>> you run into.  Could you give it a try?

> 

> It wasn't immediately obvious to me, but I think you mean this one:

> https://sourceware.org/pipermail/gdb-patches/2019-June/158525.html


Oh, wow, I confused the "158525" in the url with "158285".  They looked
the same number to me.  Sorry.  Yes, I meant that patch as you found out.

> 

> I tried it, and as expected, I get the same speedup as with my previous

> test, and strcmp_iw_ordered is now in only ~1.5% of the profiling samples.


Awesome.  Let me write some git log / ChangeLog for it and resend it as
a separate thread.

Thanks,
Pedro Alves
Gary Benson via Gdb-patches May 22, 2020, 8:28 p.m. | #8
On 5/22/20 4:47 PM, Pedro Alves via Gdb-patches wrote:
> On 5/21/20 6:18 PM, Hannes Domani via Gdb-patches wrote:

>>  Am Donnerstag, 21. Mai 2020, 18:42:23 MESZ hat Pedro Alves <palves@redhat.com> Folgendes geschrieben:

>>

>>> On 5/21/20 4:03 PM, Hannes Domani via Gdb-patches wrote:

> 

>>>>> There was a patch for this not that long ago.  Let me try to dig it up.

>>>>

>>>> You're right, I found it here:

>>>> https://sourceware.org/pipermail/gdb-patches/2019-June/158285.html

>>>

>>> Yes, that's the one!

>>>

>>>>

>>>> So I guess it's not just on Windows that slow.

>>>>

>>>> And you replied that we maybe should use TOLOWER, ISXDIGIT from libiberty

>>>> instead:

>>>> https://sourceware.org/pipermail/gdb-patches/2019-June/158518.html

>>>

>>> This message is actually older than the patch above -- I wrote the patch

>>> afterwards.

>>>

>>> The patch is using the libiberty macros, and avoids the readline clash

>>> you run into.  Could you give it a try?

>>

>> It wasn't immediately obvious to me, but I think you mean this one:

>> https://sourceware.org/pipermail/gdb-patches/2019-June/158525.html

> 

> Oh, wow, I confused the "158525" in the url with "158285".  They looked

> the same number to me.  Sorry.  Yes, I meant that patch as you found out.

> 

>>

>> I tried it, and as expected, I get the same speedup as with my previous

>> test, and strcmp_iw_ordered is now in only ~1.5% of the profiling samples.

> 

> Awesome.  Let me write some git log / ChangeLog for it and resend it as

> a separate thread.


Sent here:
  https://sourceware.org/pipermail/gdb-patches/2020-May/168897.html

( I borrowed Tromey's git log text a bit.  :-) )

Thanks,
Pedro Alves

Patch

diff --git a/gdb/dwarf2/attribute.c b/gdb/dwarf2/attribute.c
index 9f808aa7904..b39cfe2c00d 100644
--- a/gdb/dwarf2/attribute.c
+++ b/gdb/dwarf2/attribute.c
@@ -120,38 +120,13 @@  attribute::form_is_constant () const
     }
 }
 
-/* DW_ADDR is always stored already as sect_offset; despite for the forms
-   besides DW_FORM_ref_addr it is stored as cu_offset in the DWARF file.  */
-
-bool
-attribute::form_is_ref () const
-{
-  switch (form)
-    {
-    case DW_FORM_ref_addr:
-    case DW_FORM_ref1:
-    case DW_FORM_ref2:
-    case DW_FORM_ref4:
-    case DW_FORM_ref8:
-    case DW_FORM_ref_udata:
-    case DW_FORM_GNU_ref_alt:
-      return true;
-    default:
-      return false;
-    }
-}
-
 /* See attribute.h.  */
 
-sect_offset
-attribute::get_ref_die_offset () const
+void
+attribute::get_ref_die_offset_complaint () const
 {
-  if (form_is_ref ())
-    return (sect_offset) DW_UNSND (this);
-
   complaint (_("unsupported die ref attribute form: '%s'"),
 	     dwarf_form_name (form));
-  return {};
 }
 
 /* See attribute.h.  */
diff --git a/gdb/dwarf2/attribute.h b/gdb/dwarf2/attribute.h
index a9cabd69f9f..ffb91e878f1 100644
--- a/gdb/dwarf2/attribute.h
+++ b/gdb/dwarf2/attribute.h
@@ -82,7 +82,16 @@  struct attribute
   /* DW_ADDR is always stored already as sect_offset; despite for the forms
      besides DW_FORM_ref_addr it is stored as cu_offset in the DWARF file.  */
 
-  bool form_is_ref () const;
+  bool form_is_ref () const
+  {
+    return (form == DW_FORM_ref_addr
+	    || form == DW_FORM_ref1
+	    || form == DW_FORM_ref2
+	    || form == DW_FORM_ref4
+	    || form == DW_FORM_ref8
+	    || form == DW_FORM_ref_udata
+	    || form == DW_FORM_GNU_ref_alt);
+  }
 
   /* Check if the attribute's form is a DW_FORM_block*
      if so return true else false.  */
@@ -92,7 +101,13 @@  struct attribute
   /* Return DIE offset of this attribute.  Return 0 with complaint if
      the attribute is not of the required kind.  */
 
-  sect_offset get_ref_die_offset () const;
+  sect_offset get_ref_die_offset () const
+  {
+    if (form_is_ref ())
+      return (sect_offset) u.unsnd;
+    get_ref_die_offset_complaint ();
+    return {};
+  }
 
   /* Return the constant value held by this attribute.  Return
      DEFAULT_VALUE if the value held by the attribute is not
@@ -119,6 +134,12 @@  struct attribute
       ULONGEST signature;
     }
   u;
+
+private:
+
+  /* Used by get_ref_die_offset to issue a complaint.  */
+
+  void get_ref_die_offset_complaint () const;
 };
 
 /* Get at parts of an attribute structure.  */