[gdb/symtab] Fix symbol loading performance regression
Commit Message
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
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" == 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
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> Is this also ok for 8.3.1? The patch applies cleanly.
Yes, I think so. Thanks.
Tom
@@ -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')