[gdb/symtab] Fix symbol loading performance regression

Message ID 20190513092700.GA9733@delia
State New
Headers show
Series
  • [gdb/symtab] Fix symbol loading performance regression
Related show

Commit Message

Tom de Vries May 13, 2019, 9:27 a.m.
Hi,

The commit "[gdb/symtab] Fix language of duplicate static minimal symbol"
introduces a performance regression, when loading a cc1 executable build with
-O0 -g and gcc 7.4.0.  The performance regression, measured in 'real' time is
about 175%.

The slower execution comes from the fact that the fix in symbol_set_names
makes the call to symbol_find_demangled_name unconditional.

Fix this by reverting the commit, and redoing the fix as follows.

Recapturing the original problem, the first time symbol_set_names is called
with gsymbol.language == lang_auto and linkage_name == "_ZL3foov", the name is
not present in the per_bfd->demangled_names_hash hash table, so
symbol_find_demangled_name is called to demangle the name, after which the
mangled/demangled pair is added to the hashtable.  The call to
symbol_find_demangled_name also sets gsymbol.language to lang_cplus.
The second time symbol_set_names is called with gsymbol.language == lang_auto
and linkage_name == "_ZL3foov", the name is present in the hash table, so the
demangled name from the hash table is used.  However, the language of the
symbol remains lang_auto.

Fix this by adding a field language in struct demangled_name_entry, and using
the field in symbol_set_names to set the language of gsymbol, if necessary.

Tested on x86_64-linux.

OK for trunk?

Thanks,
- Tom

[gdb/symtab] Fix symbol loading performance regression

gdb/ChangeLog:

2019-05-11  Tom de Vries  <tdevries@suse.de>

	PR symtab/24545
	* symtab.c (struct demangled_name_entry): Add language field.
	(symbol_set_names):  Revert "[gdb/symtab] Fix language of duplicate
	static minimal symbol".  Set and use language field.

---
 gdb/symtab.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Tom de Vries May 27, 2019, 6:46 a.m. | #1
On 13-05-19 11:27, Tom de Vries wrote:
> Hi,

> 

> The commit "[gdb/symtab] Fix language of duplicate static minimal symbol"

> introduces a performance regression, when loading a cc1 executable build with

> -O0 -g and gcc 7.4.0.  The performance regression, measured in 'real' time is

> about 175%.

> 

> The slower execution comes from the fact that the fix in symbol_set_names

> makes the call to symbol_find_demangled_name unconditional.

> 

> Fix this by reverting the commit, and redoing the fix as follows.

> 

> Recapturing the original problem, the first time symbol_set_names is called

> with gsymbol.language == lang_auto and linkage_name == "_ZL3foov", the name is

> not present in the per_bfd->demangled_names_hash hash table, so

> symbol_find_demangled_name is called to demangle the name, after which the

> mangled/demangled pair is added to the hashtable.  The call to

> symbol_find_demangled_name also sets gsymbol.language to lang_cplus.

> The second time symbol_set_names is called with gsymbol.language == lang_auto

> and linkage_name == "_ZL3foov", the name is present in the hash table, so the

> demangled name from the hash table is used.  However, the language of the

> symbol remains lang_auto.

> 

> Fix this by adding a field language in struct demangled_name_entry, and using

> the field in symbol_set_names to set the language of gsymbol, if necessary.

> 

> Tested on x86_64-linux.

> 

> OK for trunk?

> 


Ping.

Thanks,
- Tom

> [gdb/symtab] Fix symbol loading performance regression

> 

> gdb/ChangeLog:

> 

> 2019-05-11  Tom de Vries  <tdevries@suse.de>

> 

> 	PR symtab/24545

> 	* symtab.c (struct demangled_name_entry): Add language field.

> 	(symbol_set_names):  Revert "[gdb/symtab] Fix language of duplicate

> 	static minimal symbol".  Set and use language field.

> 

> ---

>  gdb/symtab.c | 19 ++++++++++++-------

>  1 file changed, 12 insertions(+), 7 deletions(-)

> 

> diff --git a/gdb/symtab.c b/gdb/symtab.c

> index 130d5cd48f..44964533ee 100644

> --- a/gdb/symtab.c

> +++ b/gdb/symtab.c

> @@ -713,6 +713,7 @@ symbol_set_language (struct general_symbol_info *gsymbol,

>  struct demangled_name_entry

>  {

>    const char *mangled;

> +  ENUM_BITFIELD(language) language : LANGUAGE_BITS;

>    char demangled[1];

>  };

>  

> @@ -853,11 +854,6 @@ symbol_set_names (struct general_symbol_info *gsymbol,

>    else

>      linkage_name_copy = linkage_name;

>  

> -  /* Set the symbol language.  */

> -  char *demangled_name_ptr

> -    = symbol_find_demangled_name (gsymbol, linkage_name_copy);

> -  gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);

> -

>    entry.mangled = linkage_name_copy;

>    slot = ((struct demangled_name_entry **)

>  	  htab_find_slot (per_bfd->demangled_names_hash.get (),

> @@ -870,7 +866,9 @@ symbol_set_names (struct general_symbol_info *gsymbol,

>        || (gsymbol->language == language_go

>  	  && (*slot)->demangled[0] == '\0'))

>      {

> -      int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;

> +      char *demangled_name = symbol_find_demangled_name (gsymbol,

> +							 linkage_name_copy);

> +      int demangled_len = demangled_name ? strlen (demangled_name) : 0;

>  

>        /* Suppose we have demangled_name==NULL, copy_name==0, and

>  	 linkage_name_copy==linkage_name.  In this case, we already have the

> @@ -906,12 +904,19 @@ symbol_set_names (struct general_symbol_info *gsymbol,

>  	  strcpy (mangled_ptr, linkage_name_copy);

>  	  (*slot)->mangled = mangled_ptr;

>  	}

> +      (*slot)->language = gsymbol->language;

>  

>        if (demangled_name != NULL)

> -	strcpy ((*slot)->demangled, demangled_name.get());

> +	{

> +	  strcpy ((*slot)->demangled, demangled_name);

> +	  xfree (demangled_name);

> +	}

>        else

>  	(*slot)->demangled[0] = '\0';

>      }

> +  else if (gsymbol->language == language_unknown

> +	   || gsymbol->language == language_auto)

> +    gsymbol->language = (*slot)->language;

>  

>    gsymbol->name = (*slot)->mangled;

>    if ((*slot)->demangled[0] != '\0')

>
Tom Tromey May 29, 2019, 6:36 p.m. | #2
>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:


Tom> 2019-05-11  Tom de Vries  <tdevries@suse.de>

Tom> 	PR symtab/24545
Tom> 	* symtab.c (struct demangled_name_entry): Add language field.
Tom> 	(symbol_set_names):  Revert "[gdb/symtab] Fix language of duplicate
Tom> 	static minimal symbol".  Set and use language field.

Thanks for doing this.

Tom> +      char *demangled_name = symbol_find_demangled_name (gsymbol,
Tom> +							 linkage_name_copy);

I think it would be better to do

    gdb::unique_xmalloc_ptr<char> demangled_name (...);

and then adjust the code to use ".get ()" as needed.

This is ok with that change.

Tom
Tom de Vries July 18, 2019, 2:23 p.m. | #3
On 29-05-19 20:36, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

> 

> Tom> 2019-05-11  Tom de Vries  <tdevries@suse.de>

> 

> Tom> 	PR symtab/24545

> Tom> 	* symtab.c (struct demangled_name_entry): Add language field.

> Tom> 	(symbol_set_names):  Revert "[gdb/symtab] Fix language of duplicate

> Tom> 	static minimal symbol".  Set and use language field.

> 

> Thanks for doing this.

> 

> Tom> +      char *demangled_name = symbol_find_demangled_name (gsymbol,

> Tom> +							 linkage_name_copy);

> 

> I think it would be better to do

> 

>     gdb::unique_xmalloc_ptr<char> demangled_name (...);

> 

> and then adjust the code to use ".get ()" as needed.

> 

> This is ok with that change.


Is this also ok for 8.3.1? The patch applies cleanly.

Thanks,
- Tom
Tom Tromey July 19, 2019, 5:02 p.m. | #4
Tom> Is this also ok for 8.3.1? The patch applies cleanly.

Yes, I think so.  Thanks.

Tom

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 130d5cd48f..44964533ee 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -713,6 +713,7 @@  symbol_set_language (struct general_symbol_info *gsymbol,
 struct demangled_name_entry
 {
   const char *mangled;
+  ENUM_BITFIELD(language) language : LANGUAGE_BITS;
   char demangled[1];
 };
 
@@ -853,11 +854,6 @@  symbol_set_names (struct general_symbol_info *gsymbol,
   else
     linkage_name_copy = linkage_name;
 
-  /* Set the symbol language.  */
-  char *demangled_name_ptr
-    = symbol_find_demangled_name (gsymbol, linkage_name_copy);
-  gdb::unique_xmalloc_ptr<char> demangled_name (demangled_name_ptr);
-
   entry.mangled = linkage_name_copy;
   slot = ((struct demangled_name_entry **)
 	  htab_find_slot (per_bfd->demangled_names_hash.get (),
@@ -870,7 +866,9 @@  symbol_set_names (struct general_symbol_info *gsymbol,
       || (gsymbol->language == language_go
 	  && (*slot)->demangled[0] == '\0'))
     {
-      int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
+      char *demangled_name = symbol_find_demangled_name (gsymbol,
+							 linkage_name_copy);
+      int demangled_len = demangled_name ? strlen (demangled_name) : 0;
 
       /* Suppose we have demangled_name==NULL, copy_name==0, and
 	 linkage_name_copy==linkage_name.  In this case, we already have the
@@ -906,12 +904,19 @@  symbol_set_names (struct general_symbol_info *gsymbol,
 	  strcpy (mangled_ptr, linkage_name_copy);
 	  (*slot)->mangled = mangled_ptr;
 	}
+      (*slot)->language = gsymbol->language;
 
       if (demangled_name != NULL)
-	strcpy ((*slot)->demangled, demangled_name.get());
+	{
+	  strcpy ((*slot)->demangled, demangled_name);
+	  xfree (demangled_name);
+	}
       else
 	(*slot)->demangled[0] = '\0';
     }
+  else if (gsymbol->language == language_unknown
+	   || gsymbol->language == language_auto)
+    gsymbol->language = (*slot)->language;
 
   gsymbol->name = (*slot)->mangled;
   if ((*slot)->demangled[0] != '\0')