[v3,4/8] Lock the demangled hash table
Commit Message
This introduces a lock that is used when modifying the demangled hash
table.
gdb/ChangeLog
2019-05-29 Tom Tromey <tom@tromey.com>
* symtab.c (demangled_mutex): New global.
(symbol_set_names): Use a lock_guard.
---
gdb/ChangeLog | 5 ++
gdb/symtab.c | 134 ++++++++++++++++++++++++++++----------------------
2 files changed, 81 insertions(+), 58 deletions(-)
Comments
On 5/29/19 10:29 PM, Tom Tromey wrote:
> This introduces a lock that is used when modifying the demangled hash
> table.
>
> gdb/ChangeLog
> 2019-05-29 Tom Tromey <tom@tromey.com>
>
> * symtab.c (demangled_mutex): New global.
> (symbol_set_names): Use a lock_guard.
> ---
> gdb/ChangeLog | 5 ++
> gdb/symtab.c | 134 ++++++++++++++++++++++++++++----------------------
> 2 files changed, 81 insertions(+), 58 deletions(-)
>
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index 130d5cd48ff..6ad024a8a29 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -69,6 +69,9 @@
> #include "arch-utils.h"
> #include <algorithm>
> #include "common/pathstuff.h"
> +#if CXX_STD_THREAD
> +#include <mutex>
> +#endif
>
> /* Forward declarations for local functions. */
>
> @@ -709,6 +712,11 @@ symbol_set_language (struct general_symbol_info *gsymbol,
>
> /* Functions to initialize a symbol's mangled name. */
>
> +#if CXX_STD_THREAD
> +/* Mutex that is used when modifying the demangled hash table. */
> +static std::mutex demangled_mutex;
> +#endif
Not just modifying but when accessing as well.
There's only a single mutex, and the comment says _THE_ demangled
hash table, but AFAICS, each per_bfd has its own table, right?
A single lock for every table compared to a lock per table might be
a good approach, but I'd welcome extended comments to explain
that design choice.
Thanks,
Pedro Alves
On 5/30/19 1:58 PM, Pedro Alves wrote:
> A single lock for every table compared to a lock per table might be
> a good approach, but I'd welcome extended comments to explain
> that design choice.
Just to be sure something wasn't lost in translation here:
by "extended" above, I just meant that I'd like to see comments
extended in that direction, not that I'd like to see super mega long
comments. :-)
Thanks,
Pedro Alves
>> This introduces a lock that is used when modifying the demangled hash
>> table.
Pedro> There's only a single mutex, and the comment says _THE_ demangled
Pedro> hash table, but AFAICS, each per_bfd has its own table, right?
Yes, that's correct. I'll fix the comment.
Pedro> A single lock for every table compared to a lock per table might be
Pedro> a good approach, but I'd welcome extended comments to explain
Pedro> that design choice.
I didn't want to put a mutex into the per-bfd object, mostly because it
uses space for the entire lifetime of that, while the mutex is only used
while demangling minsym names; and that can (currently) only be done for
one objfile at a time.
Tom
@@ -69,6 +69,9 @@
#include "arch-utils.h"
#include <algorithm>
#include "common/pathstuff.h"
+#if CXX_STD_THREAD
+#include <mutex>
+#endif
/* Forward declarations for local functions. */
@@ -709,6 +712,11 @@ symbol_set_language (struct general_symbol_info *gsymbol,
/* Functions to initialize a symbol's mangled name. */
+#if CXX_STD_THREAD
+/* Mutex that is used when modifying the demangled hash table. */
+static std::mutex demangled_mutex;
+#endif
+
/* Objects of this type are stored in the demangled name hash table. */
struct demangled_name_entry
{
@@ -837,9 +845,6 @@ symbol_set_names (struct general_symbol_info *gsymbol,
return;
}
- if (per_bfd->demangled_names_hash == NULL)
- create_demangled_names_hash (per_bfd);
-
if (linkage_name[len] != '\0')
{
char *alloc_name;
@@ -858,64 +863,77 @@ symbol_set_names (struct general_symbol_info *gsymbol,
= 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 (),
- &entry, INSERT));
-
- /* If this name is not in the hash table, add it. */
- if (*slot == NULL
- /* A C version of the symbol may have already snuck into the table.
- This happens to, e.g., main.init (__go_init_main). Cope. */
- || (gsymbol->language == language_go
- && (*slot)->demangled[0] == '\0'))
- {
- int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
-
- /* Suppose we have demangled_name==NULL, copy_name==0, and
- linkage_name_copy==linkage_name. In this case, we already have the
- mangled name saved, and we don't have a demangled name. So,
- you might think we could save a little space by not recording
- this in the hash table at all.
+ struct demangled_name_entry *found_entry;
+
+ {
+#if CXX_STD_THREAD
+ std::lock_guard<std::mutex> guard (demangled_mutex);
+#endif
+
+ if (per_bfd->demangled_names_hash == NULL)
+ create_demangled_names_hash (per_bfd);
+
+ entry.mangled = linkage_name_copy;
+ slot = ((struct demangled_name_entry **)
+ htab_find_slot (per_bfd->demangled_names_hash.get (),
+ &entry, INSERT));
+
+ /* If this name is not in the hash table, add it. */
+ if (*slot == NULL
+ /* A C version of the symbol may have already snuck into the table.
+ This happens to, e.g., main.init (__go_init_main). Cope. */
+ || (gsymbol->language == language_go
+ && (*slot)->demangled[0] == '\0'))
+ {
+ int demangled_len = demangled_name ? strlen (demangled_name.get ()) : 0;
+
+ /* Suppose we have demangled_name==NULL, copy_name==0, and
+ linkage_name_copy==linkage_name. In this case, we already have the
+ mangled name saved, and we don't have a demangled name. So,
+ you might think we could save a little space by not recording
+ this in the hash table at all.
- It turns out that it is actually important to still save such
- an entry in the hash table, because storing this name gives
- us better bcache hit rates for partial symbols. */
- if (!copy_name && linkage_name_copy == linkage_name)
- {
- *slot
- = ((struct demangled_name_entry *)
- obstack_alloc (&per_bfd->storage_obstack,
- offsetof (struct demangled_name_entry, demangled)
- + demangled_len + 1));
- (*slot)->mangled = linkage_name;
- }
- else
- {
- char *mangled_ptr;
-
- /* If we must copy the mangled name, put it directly after
- the demangled name so we can have a single
- allocation. */
- *slot
- = ((struct demangled_name_entry *)
- obstack_alloc (&per_bfd->storage_obstack,
- offsetof (struct demangled_name_entry, demangled)
- + len + demangled_len + 2));
- mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
- strcpy (mangled_ptr, linkage_name_copy);
- (*slot)->mangled = mangled_ptr;
- }
+ It turns out that it is actually important to still save such
+ an entry in the hash table, because storing this name gives
+ us better bcache hit rates for partial symbols. */
+ if (!copy_name && linkage_name_copy == linkage_name)
+ {
+ *slot
+ = ((struct demangled_name_entry *)
+ obstack_alloc (&per_bfd->storage_obstack,
+ offsetof (struct demangled_name_entry, demangled)
+ + demangled_len + 1));
+ (*slot)->mangled = linkage_name;
+ }
+ else
+ {
+ char *mangled_ptr;
+
+ /* If we must copy the mangled name, put it directly after
+ the demangled name so we can have a single
+ allocation. */
+ *slot
+ = ((struct demangled_name_entry *)
+ obstack_alloc (&per_bfd->storage_obstack,
+ offsetof (struct demangled_name_entry, demangled)
+ + len + demangled_len + 2));
+ mangled_ptr = &((*slot)->demangled[demangled_len + 1]);
+ strcpy (mangled_ptr, linkage_name_copy);
+ (*slot)->mangled = mangled_ptr;
+ }
- if (demangled_name != NULL)
- strcpy ((*slot)->demangled, demangled_name.get());
- else
- (*slot)->demangled[0] = '\0';
- }
+ if (demangled_name != NULL)
+ strcpy ((*slot)->demangled, demangled_name.get());
+ else
+ (*slot)->demangled[0] = '\0';
+ }
+
+ found_entry = *slot;
+ }
- gsymbol->name = (*slot)->mangled;
- if ((*slot)->demangled[0] != '\0')
- symbol_set_demangled_name (gsymbol, (*slot)->demangled,
+ gsymbol->name = found_entry->mangled;
+ if (found_entry->demangled[0] != '\0')
+ symbol_set_demangled_name (gsymbol, found_entry->demangled,
&per_bfd->storage_obstack);
else
symbol_set_demangled_name (gsymbol, NULL, &per_bfd->storage_obstack);