public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Micro-optimize DWARF partial symbol reading
@ 2020-05-20 17:40 Tom Tromey
  2020-05-20 17:40 ` [PATCH 1/4] Attribute method inlining Tom Tromey
                   ` (6 more replies)
  0 siblings, 7 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-20 17:40 UTC (permalink / raw)
  To: gdb-patches

A personal goal of mine is to improve the startup time of gdb.  In the
long run, I think the answer lies partly with threading, and perhaps
with a more radical rewrite of the DWARF psymbol reader.  However,
those are difficult goals; and in the short term, I found that just
profiling the reader and making small improvements can make a
difference.

This series improves the performance of the DWARF partial symbol
reader about 10% (more in one case) on some real-world executables.
See the first patch for details (I chose to put the details there so
they would end up in the eventual git log).

Regression tested on x86-64 Fedora 30.

Let me know what you think.

Tom



^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/4] Attribute method inlining
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
@ 2020-05-20 17:40 ` Tom Tromey
  2020-05-20 19:40   ` Tom Tromey
  2020-05-21  1:08   ` Hannes Domani
  2020-05-20 17:40 ` [PATCH 2/4] Lazily compute partial DIE name Tom Tromey
                   ` (5 subsequent siblings)
  6 siblings, 2 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-20 17:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

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(-)

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.  */
-- 
2.21.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/4] Lazily compute partial DIE name
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
  2020-05-20 17:40 ` [PATCH 1/4] Attribute method inlining Tom Tromey
@ 2020-05-20 17:40 ` Tom Tromey
  2020-05-20 17:40 ` [PATCH 3/4] Inline abbrev lookup Tom Tromey
                   ` (4 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-20 17:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Currently the name of a partial DIE is computed eagerly.  However, the
name is not always needed.  This patch changes partial DIEs to compute
their name lazily, improving performance by avoiding unnecessary name
computations.

The run previous to this had times of (see the first patch in the
series for an explanation):

gdb    1.88
libxul 2.11
Ada    2.60

This patch improves the times to:

gdb    1.69
libxul 2.02
Ada    2.52

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

	* dwarf2/read.c (struct partial_die_info) <name>: Declare new
	method.
	<canonical_name>: New member.
	<raw_name>: Rename from "name".
	(partial_die_info): Initialize canonical_name.
	(scan_partial_symbols): Check raw_name.
	(partial_die_parent_scope, partial_die_full_name)
	(add_partial_symbol, add_partial_subprogram)
	(add_partial_enumeration, load_partial_dies): Use "name" method.
	(partial_die_info::name): New method.
	(partial_die_info::read, guess_partial_die_structure_name)
	(partial_die_info::fixup): Update.
---
 gdb/ChangeLog     | 15 ++++++++
 gdb/dwarf2/read.c | 93 ++++++++++++++++++++++++++++++-----------------
 2 files changed, 75 insertions(+), 33 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index ded71f53b59..b40857be5e4 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -962,6 +962,10 @@ struct partial_die_info : public allocate_on_obstack
 			  const struct abbrev_info &abbrev,
 			  const gdb_byte *info_ptr);
 
+    /* Compute the name of this partial DIE.  This memoizes the
+       result, so it is safe to call multiple times.  */
+    const char *name (dwarf2_cu *cu);
+
     /* Offset of this DIE.  */
     const sect_offset sect_off;
 
@@ -1003,9 +1007,11 @@ struct partial_die_info : public allocate_on_obstack
     /* Flag set if spec_offset uses DW_FORM_GNU_ref_alt.  */
     unsigned int spec_is_dwz : 1;
 
+    unsigned int canonical_name : 1;
+
     /* The name of this DIE.  Normally the value of DW_AT_name, but
        sometimes a default name for unnamed DIEs.  */
-    const char *name = nullptr;
+    const char *raw_name = nullptr;
 
     /* The linkage name, if present.  */
     const char *linkage_name = nullptr;
@@ -1074,6 +1080,7 @@ struct partial_die_info : public allocate_on_obstack
       fixup_called = 0;
       is_dwz = 0;
       spec_is_dwz = 0;
+      canonical_name = 0;
     }
   };
 
@@ -8044,7 +8051,7 @@ scan_partial_symbols (struct partial_die_info *first_die, CORE_ADDR *lowpc,
 	 children, so we need to look at them.  Ditto for anonymous
 	 enums.  */
 
-      if (pdi->name != NULL || pdi->tag == DW_TAG_namespace
+      if (pdi->raw_name != NULL || pdi->tag == DW_TAG_namespace
 	  || pdi->tag == DW_TAG_module || pdi->tag == DW_TAG_enumeration_type
 	  || pdi->tag == DW_TAG_imported_unit
 	  || pdi->tag == DW_TAG_inlined_subroutine)
@@ -8189,7 +8196,7 @@ partial_die_parent_scope (struct partial_die_info *pdi,
      Work around this problem here.  */
   if (cu->language == language_cplus
       && parent->tag == DW_TAG_namespace
-      && strcmp (parent->name, "::") == 0
+      && strcmp (parent->name (cu), "::") == 0
       && grandparent_scope == NULL)
     {
       parent->scope = NULL;
@@ -8213,11 +8220,11 @@ partial_die_parent_scope (struct partial_die_info *pdi,
 	  && pdi->tag == DW_TAG_subprogram))
     {
       if (grandparent_scope == NULL)
-	parent->scope = parent->name;
+	parent->scope = parent->name (cu);
       else
 	parent->scope = typename_concat (&cu->comp_unit_obstack,
 					 grandparent_scope,
-					 parent->name, 0, cu);
+					 parent->name (cu), 0, cu);
     }
   else
     {
@@ -8251,7 +8258,7 @@ partial_die_full_name (struct partial_die_info *pdi,
     {
       pdi->fixup (cu);
 
-      if (pdi->name != NULL && strchr (pdi->name, '<') == NULL)
+      if (pdi->name (cu) != NULL && strchr (pdi->name (cu), '<') == NULL)
 	{
 	  struct die_info *die;
 	  struct attribute attr;
@@ -8272,7 +8279,8 @@ partial_die_full_name (struct partial_die_info *pdi,
     return NULL;
   else
     return gdb::unique_xmalloc_ptr<char> (typename_concat (NULL, parent_scope,
-							   pdi->name, 0, cu));
+							   pdi->name (cu),
+							   0, cu));
 }
 
 static void
@@ -8294,7 +8302,7 @@ add_partial_symbol (struct partial_die_info *pdi, struct dwarf2_cu *cu)
     actual_name = built_actual_name.get ();
 
   if (actual_name == NULL)
-    actual_name = pdi->name;
+    actual_name = pdi->name (cu);
 
   partial_symbol psymbol;
   memset (&psymbol, 0, sizeof (psymbol));
@@ -8556,7 +8564,7 @@ add_partial_subprogram (struct partial_die_info *pdi,
 	    /* Ignore subprogram DIEs that do not have a name, they are
 	       illegal.  Do not emit a complaint at this point, we will
 	       do so when we convert this psymtab into a symtab.  */
-	    if (pdi->name)
+	    if (pdi->name (cu))
 	      add_partial_symbol (pdi, cu);
         }
     }
@@ -8587,13 +8595,13 @@ add_partial_enumeration (struct partial_die_info *enum_pdi,
 {
   struct partial_die_info *pdi;
 
-  if (enum_pdi->name != NULL)
+  if (enum_pdi->name (cu) != NULL)
     add_partial_symbol (enum_pdi, cu);
 
   pdi = enum_pdi->die_child;
   while (pdi)
     {
-      if (pdi->tag != DW_TAG_enumerator || pdi->name == NULL)
+      if (pdi->tag != DW_TAG_enumerator || pdi->raw_name == NULL)
 	complaint (_("malformed enumerator DIE ignored"));
       else
 	add_partial_symbol (pdi, cu);
@@ -18181,8 +18189,8 @@ load_partial_dies (const struct die_reader_specs *reader,
 	      || pdi.tag == DW_TAG_base_type
 	      || pdi.tag == DW_TAG_subrange_type))
 	{
-	  if (building_psymtab && pdi.name != NULL)
-	    add_psymbol_to_list (pdi.name, false,
+	  if (building_psymtab && pdi.raw_name != NULL)
+	    add_psymbol_to_list (pdi.name (cu), false,
 				 VAR_DOMAIN, LOC_TYPEDEF, -1,
 				 psymbol_placement::STATIC,
 				 0, cu->language, objfile);
@@ -18213,10 +18221,10 @@ load_partial_dies (const struct die_reader_specs *reader,
 	  && parent_die->tag == DW_TAG_enumeration_type
 	  && parent_die->has_specification == 0)
 	{
-	  if (pdi.name == NULL)
+	  if (pdi.raw_name == NULL)
 	    complaint (_("malformed enumerator DIE ignored"));
 	  else if (building_psymtab)
-	    add_psymbol_to_list (pdi.name, false,
+	    add_psymbol_to_list (pdi.name (cu), false,
 				 VAR_DOMAIN, LOC_CONST, -1,
 				 cu->language == language_cplus
 				 ? psymbol_placement::GLOBAL
@@ -18300,8 +18308,8 @@ load_partial_dies (const struct die_reader_specs *reader,
 	      || last_die->tag == DW_TAG_enumeration_type
 	      || (cu->language == language_cplus
 		  && last_die->tag == DW_TAG_subprogram
-		  && (last_die->name == NULL
-		      || strchr (last_die->name, '<') == NULL))
+		  && (last_die->raw_name == NULL
+		      || strchr (last_die->raw_name, '<') == NULL))
 	      || (cu->language != language_c
 		  && (last_die->tag == DW_TAG_class_type
 		      || last_die->tag == DW_TAG_interface_type
@@ -18330,6 +18338,22 @@ partial_die_info::partial_die_info (sect_offset sect_off_,
 {
 }
 
+/* See class definition.  */
+
+const char *
+partial_die_info::name (dwarf2_cu *cu)
+{
+  if (!canonical_name && raw_name != nullptr)
+    {
+      struct objfile *objfile
+	= cu->per_cu->dwarf2_per_objfile->objfile;
+      raw_name = dwarf2_canonicalize_name (raw_name, cu, objfile);
+      canonical_name = 1;
+    }
+
+  return raw_name;
+}
+
 /* Read a minimal amount of information into the minimal die structure.
    INFO_PTR should point just after the initial uleb128 of a DIE.  */
 
@@ -18372,15 +18396,12 @@ partial_die_info::read (const struct die_reader_specs *reader,
 	    case DW_TAG_enumerator:
 	      /* These tags always have simple identifiers already; no need
 		 to canonicalize them.  */
-	      name = DW_STRING (&attr);
+	      canonical_name = 1;
+	      raw_name = DW_STRING (&attr);
 	      break;
 	    default:
-	      {
-		struct objfile *objfile = dwarf2_per_objfile->objfile;
-
-		name
-		  = dwarf2_canonicalize_name (DW_STRING (&attr), cu, objfile);
-	      }
+	      canonical_name = 0;
+	      raw_name = DW_STRING (&attr);
 	      break;
 	    }
 	  break;
@@ -18532,7 +18553,7 @@ partial_die_info::read (const struct die_reader_specs *reader,
      Really, though, this is just a workaround for the fact that gdb
      doesn't store both the name and the linkage name.  */
   if (cu->language == language_ada && linkage_name != nullptr)
-    name = linkage_name;
+    raw_name = linkage_name;
 
   if (high_pc_relative)
     highpc += lowpc;
@@ -18709,7 +18730,8 @@ guess_partial_die_structure_name (struct partial_die_info *struct_pdi,
 	  if (actual_class_name != NULL)
 	    {
 	      struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
-	      struct_pdi->name = objfile->intern (actual_class_name.get ());
+	      struct_pdi->raw_name = objfile->intern (actual_class_name.get ());
+	      struct_pdi->canonical_name = 1;
 	    }
 	  break;
 	}
@@ -18747,7 +18769,7 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
   /* If we found a reference attribute and the DIE has no name, try
      to find a name in the referred to DIE.  */
 
-  if (name == NULL && has_specification)
+  if (raw_name == NULL && has_specification)
     {
       struct partial_die_info *spec_die;
 
@@ -18757,9 +18779,10 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
 
       spec_die->fixup (cu);
 
-      if (spec_die->name)
+      if (spec_die->raw_name)
 	{
-	  name = spec_die->name;
+	  raw_name = spec_die->raw_name;
+	  canonical_name = spec_die->canonical_name;
 
 	  /* Copy DW_AT_external attribute if it is set.  */
 	  if (spec_die->is_external)
@@ -18787,8 +18810,11 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
 
   /* Set default names for some unnamed DIEs.  */
 
-  if (name == NULL && tag == DW_TAG_namespace)
-    name = CP_ANONYMOUS_NAMESPACE_STR;
+  if (raw_name == NULL && tag == DW_TAG_namespace)
+    {
+      raw_name = CP_ANONYMOUS_NAMESPACE_STR;
+      canonical_name = 1;
+    }
 
   /* If there is no parent die to provide a namespace, and there are
      children, see if we can determine the namespace from their linkage
@@ -18804,7 +18830,7 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
 
   /* GCC might emit a nameless struct or union that has a linkage
      name.  See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47510.  */
-  if (name == NULL
+  if (raw_name == NULL
       && (tag == DW_TAG_class_type
 	  || tag == DW_TAG_interface_type
 	  || tag == DW_TAG_structure_type
@@ -18826,7 +18852,8 @@ partial_die_info::fixup (struct dwarf2_cu *cu)
 	    base = demangled.get ();
 
 	  struct objfile *objfile = cu->per_cu->dwarf2_per_objfile->objfile;
-	  name = objfile->intern (base);
+	  raw_name = objfile->intern (base);
+	  canonical_name = 1;
 	}
     }
 
-- 
2.21.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/4] Inline abbrev lookup
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
  2020-05-20 17:40 ` [PATCH 1/4] Attribute method inlining Tom Tromey
  2020-05-20 17:40 ` [PATCH 2/4] Lazily compute partial DIE name Tom Tromey
@ 2020-05-20 17:40 ` Tom Tromey
  2020-05-20 17:40 ` [PATCH 4/4] Use add_partial_symbol in load_partial_dies Tom Tromey
                   ` (3 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-20 17:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

Profiling showed that calls to abbrev_table::lookup_abbrev were "too
visible".  As these are just forwarding calls to the hash table, this
patch inlines the lookup.  Also, htab_find_with_hash is used, avoiding
another call.

The run previous to this had times of (see the first patch in the
series for an explanation):

gdb    1.69
libxul 2.02
Ada    2.52

This patch improves the times to:

gdb    1.64
libxul 1.99
Ada    2.47

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

	* dwarf2/abbrev.h (struct abbrev_table) <lookup_abbrev>: Inline.
	Use htab_find_with_hash.
	<add_abbrev>: Remove "abbrev_number" parameter.
	* dwarf2/abbrev.c (abbrev_table::add_abbrev): Remove
	"abbrev_number" parameter.  Use htab_find_slot_with_hash.
	(hash_abbrev): Add comment.
	(abbrev_table::lookup_abbrev): Move to header file.
	(abbrev_table::read): Update.
---
 gdb/ChangeLog       | 11 +++++++++++
 gdb/dwarf2/abbrev.c | 22 ++++++----------------
 gdb/dwarf2/abbrev.h | 13 +++++++++++--
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/gdb/dwarf2/abbrev.c b/gdb/dwarf2/abbrev.c
index b85018060fa..1552594efb5 100644
--- a/gdb/dwarf2/abbrev.c
+++ b/gdb/dwarf2/abbrev.c
@@ -36,6 +36,8 @@ static hashval_t
 hash_abbrev (const void *item)
 {
   const struct abbrev_info *info = (const struct abbrev_info *) item;
+  /* Warning: if you change this next line, you must also update the
+     other code in this class using the _with_hash functions.  */
   return info->number;
 }
 
@@ -79,25 +81,13 @@ abbrev_table::alloc_abbrev ()
 /* Add an abbreviation to the table.  */
 
 void
-abbrev_table::add_abbrev (unsigned int abbrev_number,
-			  struct abbrev_info *abbrev)
+abbrev_table::add_abbrev (struct abbrev_info *abbrev)
 {
-  void **slot = htab_find_slot (m_abbrevs.get (), abbrev, INSERT);
+  void **slot = htab_find_slot_with_hash (m_abbrevs.get (), abbrev,
+					  abbrev->number, INSERT);
   *slot = abbrev;
 }
 
-/* Look up an abbrev in the table.
-   Returns NULL if the abbrev is not found.  */
-
-struct abbrev_info *
-abbrev_table::lookup_abbrev (unsigned int abbrev_number)
-{
-  struct abbrev_info search;
-  search.number = abbrev_number;
-
-  return (struct abbrev_info *) htab_find (m_abbrevs.get (), &search);
-}
-
 /* Read in an abbrev table.  */
 
 abbrev_table_up
@@ -172,7 +162,7 @@ abbrev_table::read (struct objfile *objfile,
 	memcpy (cur_abbrev->attrs, cur_attrs.data (),
 		cur_abbrev->num_attrs * sizeof (struct attr_abbrev));
 
-      abbrev_table->add_abbrev (abbrev_number, cur_abbrev);
+      abbrev_table->add_abbrev (cur_abbrev);
 
       /* Get next abbreviation.
          Under Irix6 the abbreviations for a compilation unit are not
diff --git a/gdb/dwarf2/abbrev.h b/gdb/dwarf2/abbrev.h
index b9ace64b448..888f04ebebb 100644
--- a/gdb/dwarf2/abbrev.h
+++ b/gdb/dwarf2/abbrev.h
@@ -27,6 +27,8 @@
 #ifndef GDB_DWARF2_ABBREV_H
 #define GDB_DWARF2_ABBREV_H
 
+#include "hashtab.h"
+
 /* This data structure holds the information of an abbrev.  */
 struct abbrev_info
   {
@@ -60,8 +62,15 @@ struct abbrev_table
   /* Look up an abbrev in the table.
      Returns NULL if the abbrev is not found.  */
 
-  struct abbrev_info *lookup_abbrev (unsigned int abbrev_number);
+  struct abbrev_info *lookup_abbrev (unsigned int abbrev_number)
+  {
+    struct abbrev_info search;
+    search.number = abbrev_number;
 
+    return (struct abbrev_info *) htab_find_with_hash (m_abbrevs.get (),
+						       &search,
+						       abbrev_number);
+  }
 
   /* Where the abbrev table came from.
      This is used as a sanity check when the table is used.  */
@@ -78,7 +87,7 @@ struct abbrev_table
   struct abbrev_info *alloc_abbrev ();
 
   /* Add an abbreviation to the table.  */
-  void add_abbrev (unsigned int abbrev_number, struct abbrev_info *abbrev);
+  void add_abbrev (struct abbrev_info *abbrev);
 
   /* Hash table of abbrevs.  */
   htab_up m_abbrevs;
-- 
2.21.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 4/4] Use add_partial_symbol in load_partial_dies
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
                   ` (2 preceding siblings ...)
  2020-05-20 17:40 ` [PATCH 3/4] Inline abbrev lookup Tom Tromey
@ 2020-05-20 17:40 ` Tom Tromey
  2020-05-20 19:30 ` [PATCH 0/4] Micro-optimize DWARF partial symbol reading Christian Biesinger
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-20 17:40 UTC (permalink / raw)
  To: gdb-patches; +Cc: Tom Tromey

An earlier patch added the add_partial_symbol helper function to
dwarf2/read.c.  However, a couple of calls to add_psymbol_to_list were
left in place.  It turns out that these calls slow down partial symbol
reading, because they still go via the path that tries to needlessly
demangle already-demangled names.

This patch improves the performance of partial symbol reading by
changing this code to use add_partial_symbol instead.

The run previous to this had times of (see the first patch in the
series for an explanation):

gdb    1.64
libxul 1.99
Ada    2.47

This patch improves the times to:

gdb    1.47
libxul 1.89
Ada    2.39

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

	* dwarf2/read.c (load_partial_dies): Use add_partial_symbol.
---
 gdb/ChangeLog     |  4 ++++
 gdb/dwarf2/read.c | 13 +++----------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index b40857be5e4..363468f3683 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18190,10 +18190,8 @@ load_partial_dies (const struct die_reader_specs *reader,
 	      || pdi.tag == DW_TAG_subrange_type))
 	{
 	  if (building_psymtab && pdi.raw_name != NULL)
-	    add_psymbol_to_list (pdi.name (cu), false,
-				 VAR_DOMAIN, LOC_TYPEDEF, -1,
-				 psymbol_placement::STATIC,
-				 0, cu->language, objfile);
+	    add_partial_symbol (&pdi, cu);
+
 	  info_ptr = locate_pdi_sibling (reader, &pdi, info_ptr);
 	  continue;
 	}
@@ -18224,12 +18222,7 @@ load_partial_dies (const struct die_reader_specs *reader,
 	  if (pdi.raw_name == NULL)
 	    complaint (_("malformed enumerator DIE ignored"));
 	  else if (building_psymtab)
-	    add_psymbol_to_list (pdi.name (cu), false,
-				 VAR_DOMAIN, LOC_CONST, -1,
-				 cu->language == language_cplus
-				 ? psymbol_placement::GLOBAL
-				 : psymbol_placement::STATIC,
-				 0, cu->language, objfile);
+	    add_partial_symbol (&pdi, cu);
 
 	  info_ptr = locate_pdi_sibling (reader, &pdi, info_ptr);
 	  continue;
-- 
2.21.3


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] Micro-optimize DWARF partial symbol reading
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
                   ` (3 preceding siblings ...)
  2020-05-20 17:40 ` [PATCH 4/4] Use add_partial_symbol in load_partial_dies Tom Tromey
@ 2020-05-20 19:30 ` Christian Biesinger
  2020-05-20 21:08 ` Simon Marchi
  2020-05-27 17:48 ` Tom Tromey
  6 siblings, 0 replies; 16+ messages in thread
From: Christian Biesinger @ 2020-05-20 19:30 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

On Wed, May 20, 2020 at 12:40 PM Tom Tromey <tromey@adacore.com> wrote:
>
> A personal goal of mine is to improve the startup time of gdb.  In the
> long run, I think the answer lies partly with threading, and perhaps
> with a more radical rewrite of the DWARF psymbol reader.  However,
> those are difficult goals; and in the short term, I found that just
> profiling the reader and making small improvements can make a
> difference.
>
> This series improves the performance of the DWARF partial symbol
> reader about 10% (more in one case) on some real-world executables.
> See the first patch for details (I chose to put the details there so
> they would end up in the eventual git log).
>
> Regression tested on x86-64 Fedora 30.
>
> Let me know what you think.

These look good to me, for what it's worth.

Christian

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-20 17:40 ` [PATCH 1/4] Attribute method inlining Tom Tromey
@ 2020-05-20 19:40   ` Tom Tromey
  2020-05-21  1:08   ` Hannes Domani
  1 sibling, 0 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-20 19:40 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

>>>>> "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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] Micro-optimize DWARF partial symbol reading
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
                   ` (4 preceding siblings ...)
  2020-05-20 19:30 ` [PATCH 0/4] Micro-optimize DWARF partial symbol reading Christian Biesinger
@ 2020-05-20 21:08 ` Simon Marchi
  2020-05-27 17:48 ` Tom Tromey
  6 siblings, 0 replies; 16+ messages in thread
From: Simon Marchi @ 2020-05-20 21:08 UTC (permalink / raw)
  To: Tom Tromey, gdb-patches

On 2020-05-20 1:40 p.m., Tom Tromey wrote:
> A personal goal of mine is to improve the startup time of gdb.  In the
> long run, I think the answer lies partly with threading, and perhaps
> with a more radical rewrite of the DWARF psymbol reader.  However,
> those are difficult goals; and in the short term, I found that just
> profiling the reader and making small improvements can make a
> difference.
> 
> This series improves the performance of the DWARF partial symbol
> reader about 10% (more in one case) on some real-world executables.
> See the first patch for details (I chose to put the details there so
> they would end up in the eventual git log).
> 
> Regression tested on x86-64 Fedora 30.
> 
> Let me know what you think.
> 
> Tom

I tried the series as a whole, with these two files, libxul.so, which reads this debug info file:

$ l /usr/lib/debug/.build-id/06/bc3dd11d2331977ff78ce8e18c59216a8b9a61.debug
-rwxrwxr-x 1 root root 1.5G May  8 12:21 /usr/lib/debug/.build-id/06/bc3dd11d2331977ff78ce8e18c59216a8b9a61.debug

and libwebkit2gtk-4.0.so.37.28.5, which reads this debug info file:

$ l /usr/lib/debug/.build-id/77/5b4022ee4a85d12697b8791001b40570c25f98.debug
-rwxrwxr-x 1 root root 1.4G Aug 15  2018 /usr/lib/debug/.build-id/77/5b4022ee4a85d12697b8791001b40570c25f98.debug

So both are about the same size.  This is without the patchset applied

$ for i in 1 2 3 4 5; do time ./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch; done
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  97.10s user 1.81s system 102% cpu 1:36.94 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  97.61s user 1.96s system 102% cpu 1:37.55 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  99.33s user 1.90s system 101% cpu 1:39.34 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  96.87s user 1.95s system 101% cpu 1:36.92 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  97.19s user 1.94s system 102% cpu 1:37.10 total

$ for i in 1 2 3 4 5; do time ./gdb -nx --data-directory=data-directory /usr/lib/x86_64-linux-gnu/libwebkit2gtk-4.0.so.37.28.5 -batch; done
./gdb -nx --data-directory=data-directory  -batch  96.66s user 1.27s system 101% cpu 1:36.76 total
./gdb -nx --data-directory=data-directory  -batch  95.63s user 1.45s system 101% cpu 1:35.92 total
./gdb -nx --data-directory=data-directory  -batch  92.45s user 1.24s system 101% cpu 1:32.62 total
./gdb -nx --data-directory=data-directory  -batch  96.55s user 1.45s system 101% cpu 1:36.85 total
./gdb -nx --data-directory=data-directory  -batch  92.75s user 1.34s system 101% cpu 1:32.93 total

And this is with the patchset applied:

$ for i in 1 2 3 4 5; do time ./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch; done
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  58.08s user 1.71s system 103% cpu 57.780 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  57.89s user 1.75s system 103% cpu 57.618 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  57.85s user 1.67s system 103% cpu 57.492 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  58.03s user 1.85s system 103% cpu 57.883 total
./gdb -nx --data-directory=data-directory /usr/lib/firefox/libxul.so -batch  58.16s user 1.73s system 103% cpu 57.833 total

$ for i in 1 2 3 4 5; do time ./gdb -nx --data-directory=data-directory /usr/lib/x86_64-linux-gnu/libwebkit2gtk-4.0.so.37.28.5 -batch; do
ne
./gdb -nx --data-directory=data-directory  -batch  57.81s user 1.17s system 102% cpu 57.788 total
./gdb -nx --data-directory=data-directory  -batch  57.60s user 1.27s system 101% cpu 57.728 total
./gdb -nx --data-directory=data-directory  -batch  57.75s user 1.18s system 101% cpu 57.847 total
./gdb -nx --data-directory=data-directory  -batch  57.33s user 1.19s system 102% cpu 57.318 total
./gdb -nx --data-directory=data-directory  -batch  57.95s user 1.17s system 101% cpu 57.967 total

It's still a bit too long for an interactive user to wait, but it's quite an improvement!

Simon

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-20 17:40 ` [PATCH 1/4] Attribute method inlining Tom Tromey
  2020-05-20 19:40   ` Tom Tromey
@ 2020-05-21  1:08   ` Hannes Domani
  2020-05-21 14:26     ` Pedro Alves
  1 sibling, 1 reply; 16+ messages in thread
From: Hannes Domani @ 2020-05-21  1:08 UTC (permalink / raw)
  To: Gdb-patches

 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-21  1:08   ` Hannes Domani
@ 2020-05-21 14:26     ` Pedro Alves
  2020-05-21 15:03       ` Hannes Domani
  0 siblings, 1 reply; 16+ messages in thread
From: Pedro Alves @ 2020-05-21 14:26 UTC (permalink / raw)
  To: Hannes Domani, Gdb-patches

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-21 14:26     ` Pedro Alves
@ 2020-05-21 15:03       ` Hannes Domani
  2020-05-21 16:42         ` Pedro Alves
  0 siblings, 1 reply; 16+ messages in thread
From: Hannes Domani @ 2020-05-21 15:03 UTC (permalink / raw)
  To: Gdb-patches

 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-21 15:03       ` Hannes Domani
@ 2020-05-21 16:42         ` Pedro Alves
  2020-05-21 17:18           ` Hannes Domani
  0 siblings, 1 reply; 16+ messages in thread
From: Pedro Alves @ 2020-05-21 16:42 UTC (permalink / raw)
  To: Hannes Domani, Gdb-patches

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.
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-21 16:42         ` Pedro Alves
@ 2020-05-21 17:18           ` Hannes Domani
  2020-05-22 15:47             ` Pedro Alves
  0 siblings, 1 reply; 16+ messages in thread
From: Hannes Domani @ 2020-05-21 17:18 UTC (permalink / raw)
  To: Gdb-patches

 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-21 17:18           ` Hannes Domani
@ 2020-05-22 15:47             ` Pedro Alves
  2020-05-22 20:28               ` Pedro Alves
  0 siblings, 1 reply; 16+ messages in thread
From: Pedro Alves @ 2020-05-22 15:47 UTC (permalink / raw)
  To: Hannes Domani, Gdb-patches

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/4] Attribute method inlining
  2020-05-22 15:47             ` Pedro Alves
@ 2020-05-22 20:28               ` Pedro Alves
  0 siblings, 0 replies; 16+ messages in thread
From: Pedro Alves @ 2020-05-22 20:28 UTC (permalink / raw)
  To: Hannes Domani, Gdb-patches

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/4] Micro-optimize DWARF partial symbol reading
  2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
                   ` (5 preceding siblings ...)
  2020-05-20 21:08 ` Simon Marchi
@ 2020-05-27 17:48 ` Tom Tromey
  6 siblings, 0 replies; 16+ messages in thread
From: Tom Tromey @ 2020-05-27 17:48 UTC (permalink / raw)
  To: Tom Tromey; +Cc: gdb-patches

>>>>> "Tom" == Tom Tromey <tromey@adacore.com> writes:

Tom> This series improves the performance of the DWARF partial symbol
Tom> reader about 10% (more in one case) on some real-world executables.
Tom> See the first patch for details (I chose to put the details there so
Tom> they would end up in the eventual git log).

I've rebased these and re-run the performance tests to make sure they
are still improvements.  So, I'm checking them in.

Tom

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-05-27 17:48 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 17:40 [PATCH 0/4] Micro-optimize DWARF partial symbol reading Tom Tromey
2020-05-20 17:40 ` [PATCH 1/4] Attribute method inlining Tom Tromey
2020-05-20 19:40   ` Tom Tromey
2020-05-21  1:08   ` Hannes Domani
2020-05-21 14:26     ` Pedro Alves
2020-05-21 15:03       ` Hannes Domani
2020-05-21 16:42         ` Pedro Alves
2020-05-21 17:18           ` Hannes Domani
2020-05-22 15:47             ` Pedro Alves
2020-05-22 20:28               ` Pedro Alves
2020-05-20 17:40 ` [PATCH 2/4] Lazily compute partial DIE name Tom Tromey
2020-05-20 17:40 ` [PATCH 3/4] Inline abbrev lookup Tom Tromey
2020-05-20 17:40 ` [PATCH 4/4] Use add_partial_symbol in load_partial_dies Tom Tromey
2020-05-20 19:30 ` [PATCH 0/4] Micro-optimize DWARF partial symbol reading Christian Biesinger
2020-05-20 21:08 ` Simon Marchi
2020-05-27 17:48 ` Tom Tromey

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).