* [review] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
@ 2019-10-29 19:28 ` Tom Tromey (Code Review)
2019-10-29 21:56 ` [review v2] " Christian Biesinger (Code Review)
` (8 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Tom Tromey (Code Review) @ 2019-10-29 19:28 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Patch Set 1: Code-Review+1
(4 comments)
This looks good. I found a few nits, see below.
I'll probably put in the threading patches "soon", they've
been few a through rounds of review and have been sitting for
a while now.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c
File gdb/minsyms.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c@1252
PS1, Line 1252:
1243 | else
1244 | *copyto++ = *copyfrom++;
1245 | }
1246 | *copyto++ = *copyfrom++;
1247 | mcount = copyto - msymbol;
1248 | }
1249 | return (mcount);
1250 | }
1251 |
1252 | struct computed_hash_values {
{ on next line.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c@1392
PS1, Line 1392:
1370 | #endif
| ...
1383 | /* This will be freed later, by symbol_set_names. */
1384 | char *demangled_name
1385 | = symbol_find_demangled_name (msym, msym->name);
1386 | symbol_set_demangled_name
1387 | (msym, demangled_name,
1388 | &m_objfile->per_bfd->storage_obstack);
1389 | msym->name_set = 1;
1390 |
1391 | size_t idx = msym - msymbols;
1392 | hash_values[idx].mangled_name_hash = htab_hash_string (msym->name);
over-long line? Also, fast_hash
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h
File gdb/symtab.h:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h@518
PS1, Line 518:
509 | it. Used for constructs which do not have a mangled name,
510 | e.g. struct tags. Unlike SYMBOL_SET_NAMES, linkage_name must
511 | be terminated and either already on the objfile's obstack or
512 | permanently allocated. */
513 | #define SYMBOL_SET_LINKAGE_NAME(symbol,linkage_name) \
514 | (symbol)->ginfo.name = (linkage_name)
515 |
516 | /* Set the linkage and natural names of a symbol, by demangling
517 | the linkage name. Optionally, HASH can be set to the value
518 | of htab_hash_string (linkage_name) (if nullterminated), to
This has to refer to `fast_hash` now.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h@526
PS1, Line 526:
517 | the linkage name. Optionally, HASH can be set to the value
518 | of htab_hash_string (linkage_name) (if nullterminated), to
519 | speed up this function. */
520 | #define SYMBOL_SET_NAMES(symbol,linkage_name,len,copy_name,objfile) \
521 | symbol_set_names (&(symbol)->ginfo, linkage_name, len, copy_name, \
522 | (objfile)->per_bfd)
523 | extern void symbol_set_names (struct general_symbol_info *symbol,
524 | const char *linkage_name, int len, bool copy_name,
525 | struct objfile_per_bfd_storage *per_bfd,
526 | hashval_t hash = 0);
I suppose an optional<hashval_t> would avoid the problem
when the hash value is actually 0
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 1
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 19:28:03 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v2] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
2019-10-29 19:28 ` Tom Tromey (Code Review)
@ 2019-10-29 21:56 ` Christian Biesinger (Code Review)
2019-10-29 21:56 ` Christian Biesinger (Code Review)
` (7 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-29 21:56 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Uploaded patch set 2.
(4 comments)
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c
File gdb/minsyms.c:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c@1252
PS1, Line 1252:
1243 | else
1244 | *copyto++ = *copyfrom++;
1245 | }
1246 | *copyto++ = *copyfrom++;
1247 | mcount = copyto - msymbol;
1248 | }
1249 | return (mcount);
1250 | }
1251 |
1252 | struct computed_hash_values {
> { on next line.
Done
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/minsyms.c@1392
PS1, Line 1392:
1370 | #endif
| ...
1383 | /* This will be freed later, by symbol_set_names. */
1384 | char *demangled_name
1385 | = symbol_find_demangled_name (msym, msym->name);
1386 | symbol_set_demangled_name
1387 | (msym, demangled_name,
1388 | &m_objfile->per_bfd->storage_obstack);
1389 | msym->name_set = 1;
1390 |
1391 | size_t idx = msym - msymbols;
1392 | hash_values[idx].mangled_name_hash = htab_hash_string (msym->name);
> over-long line? Also, fast_hash
Great catch, thanks!
Since I now need strlen (msym->name) in two places, I decided to store that in hash_values[idx] as well. Let me know if you dislike that.
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h
File gdb/symtab.h:
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h@518
PS1, Line 518:
509 | it. Used for constructs which do not have a mangled name,
510 | e.g. struct tags. Unlike SYMBOL_SET_NAMES, linkage_name must
511 | be terminated and either already on the objfile's obstack or
512 | permanently allocated. */
513 | #define SYMBOL_SET_LINKAGE_NAME(symbol,linkage_name) \
514 | (symbol)->ginfo.name = (linkage_name)
515 |
516 | /* Set the linkage and natural names of a symbol, by demangling
517 | the linkage name. Optionally, HASH can be set to the value
518 | of htab_hash_string (linkage_name) (if nullterminated), to
> This has to refer to `fast_hash` now.
Done
https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307/1/gdb/symtab.h@526
PS1, Line 526:
517 | the linkage name. Optionally, HASH can be set to the value
518 | of htab_hash_string (linkage_name) (if nullterminated), to
519 | speed up this function. */
520 | #define SYMBOL_SET_NAMES(symbol,linkage_name,len,copy_name,objfile) \
521 | symbol_set_names (&(symbol)->ginfo, linkage_name, len, copy_name, \
522 | (objfile)->per_bfd)
523 | extern void symbol_set_names (struct general_symbol_info *symbol,
524 | const char *linkage_name, int len, bool copy_name,
525 | struct objfile_per_bfd_storage *per_bfd,
526 | hashval_t hash = 0);
> I suppose an optional<hashval_t> would avoid the problem [â¦]
Done
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 29 Oct 2019 21:56:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v2] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
2019-10-29 19:28 ` Tom Tromey (Code Review)
2019-10-29 21:56 ` [review v2] " Christian Biesinger (Code Review)
@ 2019-10-29 21:56 ` Christian Biesinger (Code Review)
2019-10-31 0:23 ` [review v3] " Christian Biesinger (Code Review)
` (6 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-29 21:56 UTC (permalink / raw)
To: Christian Biesinger, Tom Tromey, gdb-patches
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Precompute hash value for symbol_set_names
We can also compute the hash for the mangled name on a background
thread so make this function even faster (about a 7% speedup).
gdb/ChangeLog:
2019-10-03 Christian Biesinger <cbiesinger@google.com>
* minsyms.c (minimal_symbol_reader::install): Also compute the hash
of the mangled name on the background thread.
* symtab.c (symbol_set_names): Allow passing in the hash of the
linkage_name.
* symtab.h (symbol_set_names): Likewise.
Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
---
M gdb/minsyms.c
M gdb/symtab.c
M gdb/symtab.h
3 files changed, 28 insertions(+), 7 deletions(-)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index bfcd5d5..c53094b 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1249,6 +1249,12 @@
return (mcount);
}
+struct computed_hash_values
+{
+ size_t name_length;
+ hashval_t mangled_name_hash;
+};
+
/* Build (or rebuild) the minimal symbol hash tables. This is necessary
after compacting or sorting the table since the entries move around
thus causing the internal minimal_symbol pointers to become jumbled. */
@@ -1365,6 +1371,8 @@
std::mutex demangled_mutex;
#endif
+ std::vector<computed_hash_values> hash_values (mcount);
+
msymbols = m_objfile->per_bfd->msymbols.get ();
gdb::parallel_for_each
(&msymbols[0], &msymbols[mcount],
@@ -1372,6 +1380,8 @@
{
for (minimal_symbol *msym = start; msym < end; ++msym)
{
+ size_t idx = msym - msymbols;
+ hash_values[idx].name_length = strlen (msym->name);
if (!msym->name_set)
{
/* This will be freed later, by symbol_set_names. */
@@ -1381,6 +1391,9 @@
(msym, demangled_name,
&m_objfile->per_bfd->storage_obstack);
msym->name_set = 1;
+
+ hash_values[idx].mangled_name_hash
+ = fast_hash (msym->name, hash_values[idx].name_length);
}
}
{
@@ -1391,9 +1404,11 @@
#endif
for (minimal_symbol *msym = start; msym < end; ++msym)
{
+ size_t idx = msym - msymbols;
symbol_set_names (msym, msym->name,
- strlen (msym->name), 0,
- m_objfile->per_bfd);
+ hash_values[idx].name_length, 0,
+ m_objfile->per_bfd,
+ hash_values[idx].mangled_name_hash);
}
}
});
diff --git a/gdb/symtab.c b/gdb/symtab.c
index abc6a77..c2701db 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -825,7 +825,8 @@
void
symbol_set_names (struct general_symbol_info *gsymbol,
const char *linkage_name, int len, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd)
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash)
{
struct demangled_name_entry **slot;
/* A 0-terminated copy of the linkage name. */
@@ -868,9 +869,11 @@
linkage_name_copy = linkage_name;
struct demangled_name_entry entry (gdb::string_view (linkage_name_copy, len));
+ if (!hash.has_value ())
+ hash = hash_demangled_name_entry (&entry);
slot = ((struct demangled_name_entry **)
- htab_find_slot (per_bfd->demangled_names_hash.get (),
- &entry, INSERT));
+ htab_find_slot_with_hash (per_bfd->demangled_names_hash.get (),
+ &entry, *hash, INSERT));
/* If this name is not in the hash table, add it. */
if (*slot == NULL
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 7d41de9..e3a8d89 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -514,13 +514,16 @@
(symbol)->ginfo.name = (linkage_name)
/* Set the linkage and natural names of a symbol, by demangling
- the linkage name. */
+ the linkage name. Optionally, HASH can be set to the value
+ of fast_hash (linkage_name), to speed up this function. */
#define SYMBOL_SET_NAMES(symbol,linkage_name,len,copy_name,objfile) \
symbol_set_names (&(symbol)->ginfo, linkage_name, len, copy_name, \
(objfile)->per_bfd)
extern void symbol_set_names (struct general_symbol_info *symbol,
const char *linkage_name, int len, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd);
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash
+ = gdb::optional<hashval_t> ());
/* Now come lots of name accessor macros. Short version as to when to
use which: Use SYMBOL_NATURAL_NAME to refer to the name of the
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 2
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v3] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (2 preceding siblings ...)
2019-10-29 21:56 ` Christian Biesinger (Code Review)
@ 2019-10-31 0:23 ` Christian Biesinger (Code Review)
2019-11-26 22:08 ` [review v4] " Tom Tromey (Code Review)
` (5 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-10-31 0:23 UTC (permalink / raw)
To: Christian Biesinger, Tom Tromey, gdb-patches
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Precompute hash value for symbol_set_names
We can also compute the hash for the mangled name on a background
thread so make this function even faster (about a 7% speedup).
gdb/ChangeLog:
2019-10-03 Christian Biesinger <cbiesinger@google.com>
* minsyms.c (minimal_symbol_reader::install): Also compute the hash
of the mangled name on the background thread.
* symtab.c (symbol_set_names): Allow passing in the hash of the
linkage_name.
* symtab.h (symbol_set_names): Likewise.
Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
---
M gdb/minsyms.c
M gdb/symtab.c
M gdb/symtab.h
3 files changed, 30 insertions(+), 6 deletions(-)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index f9d1172..d2d8bf5 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1258,6 +1258,12 @@
}
}
+struct computed_hash_values
+{
+ size_t name_length;
+ hashval_t mangled_name_hash;
+};
+
/* Build (or rebuild) the minimal symbol hash tables. This is necessary
after compacting or sorting the table since the entries move around
thus causing the internal minimal_symbol pointers to become jumbled. */
@@ -1370,6 +1376,8 @@
std::mutex demangled_mutex;
#endif
+ std::vector<computed_hash_values> hash_values (mcount);
+
msymbols = m_objfile->per_bfd->msymbols.get ();
gdb::parallel_for_each
(&msymbols[0], &msymbols[mcount],
@@ -1377,6 +1385,8 @@
{
for (minimal_symbol *msym = start; msym < end; ++msym)
{
+ size_t idx = msym - msymbols;
+ hash_values[idx].name_length = strlen (msym->name);
if (!msym->name_set)
{
/* This will be freed later, by symbol_set_names. */
@@ -1386,6 +1396,9 @@
(msym, demangled_name,
&m_objfile->per_bfd->storage_obstack);
msym->name_set = 1;
+
+ hash_values[idx].mangled_name_hash
+ = fast_hash (msym->name, hash_values[idx].name_length);
}
}
{
@@ -1396,8 +1409,14 @@
#endif
for (minimal_symbol *msym = start; msym < end; ++msym)
{
- symbol_set_names (msym, msym->name, false,
- m_objfile->per_bfd);
+ size_t idx = msym - msymbols;
+ symbol_set_names
+ (msym,
+ gdb::string_view(msym->name,
+ hash_values[idx].name_length),
+ false,
+ m_objfile->per_bfd,
+ hash_values[idx].mangled_name_hash);
}
}
});
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 3502827..f5a759d 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -825,7 +825,8 @@
void
symbol_set_names (struct general_symbol_info *gsymbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd)
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash)
{
struct demangled_name_entry **slot;
@@ -853,9 +854,11 @@
create_demangled_names_hash (per_bfd);
struct demangled_name_entry entry (linkage_name);
+ if (!hash.has_value ())
+ hash = hash_demangled_name_entry (&entry);
slot = ((struct demangled_name_entry **)
- htab_find_slot (per_bfd->demangled_names_hash.get (),
- &entry, INSERT));
+ htab_find_slot_with_hash (per_bfd->demangled_names_hash.get (),
+ &entry, *hash, INSERT));
/* If this name is not in the hash table, add it. */
if (*slot == NULL
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 4172a25..fe74204 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -522,7 +522,9 @@
(objfile)->per_bfd)
extern void symbol_set_names (struct general_symbol_info *symbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd);
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash
+ = gdb::optional<hashval_t> ());
/* Now come lots of name accessor macros. Short version as to when to
use which: Use SYMBOL_NATURAL_NAME to refer to the name of the
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 3
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v4] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (3 preceding siblings ...)
2019-10-31 0:23 ` [review v3] " Christian Biesinger (Code Review)
@ 2019-11-26 22:08 ` Tom Tromey (Code Review)
2019-11-26 22:23 ` [review v5] " Christian Biesinger (Code Review)
` (4 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Tom Tromey (Code Review) @ 2019-11-26 22:08 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Thanks for doing this. This looks good to me. I have one nit;
this is ok with that fixed.
| --- gdb/minsyms.c
| +++ gdb/minsyms.c
| @@ -1252,13 +1252,19 @@ static void
| clear_minimal_symbol_hash_tables (struct objfile *objfile)
| {
| for (size_t i = 0; i < MINIMAL_SYMBOL_HASH_SIZE; i++)
| {
| objfile->per_bfd->msymbol_hash[i] = 0;
| objfile->per_bfd->msymbol_demangled_hash[i] = 0;
| }
| }
|
| +struct computed_hash_values
PS4, Line 1261:
This should have a comment explaining its purpose.
Also the fields should have comments.
| +{
| + size_t name_length;
| + hashval_t mangled_name_hash;
| +};
| +
| /* Build (or rebuild) the minimal symbol hash tables. This is necessary
| after compacting or sorting the table since the entries move around
| thus causing the internal minimal_symbol pointers to become jumbled. */
|
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 4
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 22:08:35 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v5] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (4 preceding siblings ...)
2019-11-26 22:08 ` [review v4] " Tom Tromey (Code Review)
@ 2019-11-26 22:23 ` Christian Biesinger (Code Review)
2019-11-26 22:24 ` Christian Biesinger (Code Review)
` (3 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-11-26 22:23 UTC (permalink / raw)
To: Christian Biesinger, Tom Tromey, gdb-patches
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Precompute hash value for symbol_set_names
We can also compute the hash for the mangled name on a background
thread so make this function even faster (about a 7% speedup).
gdb/ChangeLog:
2019-10-03 Christian Biesinger <cbiesinger@google.com>
* minsyms.c (minimal_symbol_reader::install): Also compute the hash
of the mangled name on the background thread.
* symtab.c (symbol_set_names): Allow passing in the hash of the
linkage_name.
* symtab.h (symbol_set_names): Likewise.
Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
---
M gdb/minsyms.c
M gdb/symtab.c
M gdb/symtab.h
3 files changed, 34 insertions(+), 6 deletions(-)
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 03a1932..141c3d2 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1258,6 +1258,16 @@
}
}
+/* This struct is used to store values we compute for msymbols on the
+ background threads but don't need to keep around long term. */
+struct computed_hash_values
+{
+ /* Length of the linkage_name of the symbol. */
+ size_t name_length;
+ /* Hash code (using fast_hash) of the linkage_name. */
+ hashval_t mangled_name_hash;
+};
+
/* Build (or rebuild) the minimal symbol hash tables. This is necessary
after compacting or sorting the table since the entries move around
thus causing the internal minimal_symbol pointers to become jumbled. */
@@ -1370,6 +1380,8 @@
std::mutex demangled_mutex;
#endif
+ std::vector<computed_hash_values> hash_values (mcount);
+
msymbols = m_objfile->per_bfd->msymbols.get ();
gdb::parallel_for_each
(&msymbols[0], &msymbols[mcount],
@@ -1377,6 +1389,8 @@
{
for (minimal_symbol *msym = start; msym < end; ++msym)
{
+ size_t idx = msym - msymbols;
+ hash_values[idx].name_length = strlen (msym->name);
if (!msym->name_set)
{
/* This will be freed later, by symbol_set_names. */
@@ -1386,6 +1400,9 @@
(msym, demangled_name,
&m_objfile->per_bfd->storage_obstack);
msym->name_set = 1;
+
+ hash_values[idx].mangled_name_hash
+ = fast_hash (msym->name, hash_values[idx].name_length);
}
}
{
@@ -1396,8 +1413,14 @@
#endif
for (minimal_symbol *msym = start; msym < end; ++msym)
{
- symbol_set_names (msym, msym->name, false,
- m_objfile->per_bfd);
+ size_t idx = msym - msymbols;
+ symbol_set_names
+ (msym,
+ gdb::string_view(msym->name,
+ hash_values[idx].name_length),
+ false,
+ m_objfile->per_bfd,
+ hash_values[idx].mangled_name_hash);
}
}
});
diff --git a/gdb/symtab.c b/gdb/symtab.c
index b5c8109..b39492c 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -836,7 +836,8 @@
void
symbol_set_names (struct general_symbol_info *gsymbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd)
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash)
{
struct demangled_name_entry **slot;
@@ -864,9 +865,11 @@
create_demangled_names_hash (per_bfd);
struct demangled_name_entry entry (linkage_name);
+ if (!hash.has_value ())
+ hash = hash_demangled_name_entry (&entry);
slot = ((struct demangled_name_entry **)
- htab_find_slot (per_bfd->demangled_names_hash.get (),
- &entry, INSERT));
+ htab_find_slot_with_hash (per_bfd->demangled_names_hash.get (),
+ &entry, *hash, INSERT));
/* If this name is not in the hash table, add it. */
if (*slot == NULL
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 9c2aea7..f660ce9 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -554,7 +554,9 @@
(objfile)->per_bfd)
extern void symbol_set_names (struct general_symbol_info *symbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd);
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash
+ = gdb::optional<hashval_t> ());
/* Return true if NAME matches the "search" name of SYMBOL, according
to the symbol's language. */
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v5] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (5 preceding siblings ...)
2019-11-26 22:23 ` [review v5] " Christian Biesinger (Code Review)
@ 2019-11-26 22:24 ` Christian Biesinger (Code Review)
2019-11-27 18:03 ` Tom Tromey (Code Review)
` (2 subsequent siblings)
9 siblings, 0 replies; 11+ messages in thread
From: Christian Biesinger (Code Review) @ 2019-11-26 22:24 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey
Christian Biesinger has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Patch Set 5:
(1 comment)
| --- gdb/minsyms.c
| +++ gdb/minsyms.c
| @@ -1252,13 +1252,19 @@ static void
| clear_minimal_symbol_hash_tables (struct objfile *objfile)
| {
| for (size_t i = 0; i < MINIMAL_SYMBOL_HASH_SIZE; i++)
| {
| objfile->per_bfd->msymbol_hash[i] = 0;
| objfile->per_bfd->msymbol_demangled_hash[i] = 0;
| }
| }
|
| +struct computed_hash_values
PS4, Line 1261:
Done
| +{
| + size_t name_length;
| + hashval_t mangled_name_hash;
| +};
| +
| /* Build (or rebuild) the minimal symbol hash tables. This is necessary
| after compacting or sorting the table since the entries move around
| thus causing the internal minimal_symbol pointers to become jumbled. */
|
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Tue, 26 Nov 2019 22:23:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 11+ messages in thread
* [review v5] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (6 preceding siblings ...)
2019-11-26 22:24 ` Christian Biesinger (Code Review)
@ 2019-11-27 18:03 ` Tom Tromey (Code Review)
2019-11-27 21:40 ` [pushed] " Sourceware to Gerrit sync (Code Review)
2019-11-27 21:40 ` Sourceware to Gerrit sync (Code Review)
9 siblings, 0 replies; 11+ messages in thread
From: Tom Tromey (Code Review) @ 2019-11-27 18:03 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches
Tom Tromey has posted comments on this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Patch Set 5: Code-Review+2
Thank you.
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 5
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-Comment-Date: Wed, 27 Nov 2019 18:03:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pushed] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (7 preceding siblings ...)
2019-11-27 18:03 ` Tom Tromey (Code Review)
@ 2019-11-27 21:40 ` Sourceware to Gerrit sync (Code Review)
2019-11-27 21:40 ` Sourceware to Gerrit sync (Code Review)
9 siblings, 0 replies; 11+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-11-27 21:40 UTC (permalink / raw)
To: Christian Biesinger, gdb-patches; +Cc: Tom Tromey
Sourceware to Gerrit sync has submitted this change.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Precompute hash value for symbol_set_names
We can also compute the hash for the mangled name on a background
thread so make this function even faster (about a 7% speedup).
gdb/ChangeLog:
2019-11-27 Christian Biesinger <cbiesinger@google.com>
* minsyms.c (minimal_symbol_reader::install): Also compute the hash
of the mangled name on the background thread.
* symtab.c (symbol_set_names): Allow passing in the hash of the
linkage_name.
* symtab.h (symbol_set_names): Likewise.
Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
---
M gdb/ChangeLog
M gdb/minsyms.c
M gdb/symtab.c
M gdb/symtab.h
4 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0816e73..c278a77 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2019-11-27 Christian Biesinger <cbiesinger@google.com>
+
+ * minsyms.c (minimal_symbol_reader::install): Also compute the hash
+ of the mangled name on the background thread.
+ * symtab.c (symbol_set_names): Allow passing in the hash of the
+ linkage_name.
+ * symtab.h (symbol_set_names): Likewise.
+
2019-11-27 Kevin Buettner <kevinb@redhat.com>
* dwarf2read.c (inherit_abstract_dies): Ensure that delayed
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 03a1932..141c3d2 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1258,6 +1258,16 @@
}
}
+/* This struct is used to store values we compute for msymbols on the
+ background threads but don't need to keep around long term. */
+struct computed_hash_values
+{
+ /* Length of the linkage_name of the symbol. */
+ size_t name_length;
+ /* Hash code (using fast_hash) of the linkage_name. */
+ hashval_t mangled_name_hash;
+};
+
/* Build (or rebuild) the minimal symbol hash tables. This is necessary
after compacting or sorting the table since the entries move around
thus causing the internal minimal_symbol pointers to become jumbled. */
@@ -1370,6 +1380,8 @@
std::mutex demangled_mutex;
#endif
+ std::vector<computed_hash_values> hash_values (mcount);
+
msymbols = m_objfile->per_bfd->msymbols.get ();
gdb::parallel_for_each
(&msymbols[0], &msymbols[mcount],
@@ -1377,6 +1389,8 @@
{
for (minimal_symbol *msym = start; msym < end; ++msym)
{
+ size_t idx = msym - msymbols;
+ hash_values[idx].name_length = strlen (msym->name);
if (!msym->name_set)
{
/* This will be freed later, by symbol_set_names. */
@@ -1386,6 +1400,9 @@
(msym, demangled_name,
&m_objfile->per_bfd->storage_obstack);
msym->name_set = 1;
+
+ hash_values[idx].mangled_name_hash
+ = fast_hash (msym->name, hash_values[idx].name_length);
}
}
{
@@ -1396,8 +1413,14 @@
#endif
for (minimal_symbol *msym = start; msym < end; ++msym)
{
- symbol_set_names (msym, msym->name, false,
- m_objfile->per_bfd);
+ size_t idx = msym - msymbols;
+ symbol_set_names
+ (msym,
+ gdb::string_view(msym->name,
+ hash_values[idx].name_length),
+ false,
+ m_objfile->per_bfd,
+ hash_values[idx].mangled_name_hash);
}
}
});
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 8f46321..894a323 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -836,7 +836,8 @@
void
symbol_set_names (struct general_symbol_info *gsymbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd)
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash)
{
struct demangled_name_entry **slot;
@@ -864,9 +865,11 @@
create_demangled_names_hash (per_bfd);
struct demangled_name_entry entry (linkage_name);
+ if (!hash.has_value ())
+ hash = hash_demangled_name_entry (&entry);
slot = ((struct demangled_name_entry **)
- htab_find_slot (per_bfd->demangled_names_hash.get (),
- &entry, INSERT));
+ htab_find_slot_with_hash (per_bfd->demangled_names_hash.get (),
+ &entry, *hash, INSERT));
/* If this name is not in the hash table, add it. */
if (*slot == NULL
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 7a51456..4cfdf06 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -553,7 +553,9 @@
(objfile)->per_bfd)
extern void symbol_set_names (struct general_symbol_info *symbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd);
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash
+ = gdb::optional<hashval_t> ());
/* Return true if NAME matches the "search" name of SYMBOL, according
to the symbol's language. */
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 6
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: merged
^ permalink raw reply [flat|nested] 11+ messages in thread
* [pushed] Precompute hash value for symbol_set_names
2019-10-25 19:30 [review] Precompute hash value for symbol_set_names Christian Biesinger (Code Review)
` (8 preceding siblings ...)
2019-11-27 21:40 ` [pushed] " Sourceware to Gerrit sync (Code Review)
@ 2019-11-27 21:40 ` Sourceware to Gerrit sync (Code Review)
9 siblings, 0 replies; 11+ messages in thread
From: Sourceware to Gerrit sync (Code Review) @ 2019-11-27 21:40 UTC (permalink / raw)
To: Christian Biesinger, Tom Tromey, gdb-patches
The original change was created by Christian Biesinger.
Change URL: https://gnutoolchain-gerrit.osci.io/r/c/binutils-gdb/+/307
......................................................................
Precompute hash value for symbol_set_names
We can also compute the hash for the mangled name on a background
thread so make this function even faster (about a 7% speedup).
gdb/ChangeLog:
2019-11-27 Christian Biesinger <cbiesinger@google.com>
* minsyms.c (minimal_symbol_reader::install): Also compute the hash
of the mangled name on the background thread.
* symtab.c (symbol_set_names): Allow passing in the hash of the
linkage_name.
* symtab.h (symbol_set_names): Likewise.
Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
---
M gdb/ChangeLog
M gdb/minsyms.c
M gdb/symtab.c
M gdb/symtab.h
4 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 0816e73..c278a77 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2019-11-27 Christian Biesinger <cbiesinger@google.com>
+
+ * minsyms.c (minimal_symbol_reader::install): Also compute the hash
+ of the mangled name on the background thread.
+ * symtab.c (symbol_set_names): Allow passing in the hash of the
+ linkage_name.
+ * symtab.h (symbol_set_names): Likewise.
+
2019-11-27 Kevin Buettner <kevinb@redhat.com>
* dwarf2read.c (inherit_abstract_dies): Ensure that delayed
diff --git a/gdb/minsyms.c b/gdb/minsyms.c
index 03a1932..141c3d2 100644
--- a/gdb/minsyms.c
+++ b/gdb/minsyms.c
@@ -1258,6 +1258,16 @@
}
}
+/* This struct is used to store values we compute for msymbols on the
+ background threads but don't need to keep around long term. */
+struct computed_hash_values
+{
+ /* Length of the linkage_name of the symbol. */
+ size_t name_length;
+ /* Hash code (using fast_hash) of the linkage_name. */
+ hashval_t mangled_name_hash;
+};
+
/* Build (or rebuild) the minimal symbol hash tables. This is necessary
after compacting or sorting the table since the entries move around
thus causing the internal minimal_symbol pointers to become jumbled. */
@@ -1370,6 +1380,8 @@
std::mutex demangled_mutex;
#endif
+ std::vector<computed_hash_values> hash_values (mcount);
+
msymbols = m_objfile->per_bfd->msymbols.get ();
gdb::parallel_for_each
(&msymbols[0], &msymbols[mcount],
@@ -1377,6 +1389,8 @@
{
for (minimal_symbol *msym = start; msym < end; ++msym)
{
+ size_t idx = msym - msymbols;
+ hash_values[idx].name_length = strlen (msym->name);
if (!msym->name_set)
{
/* This will be freed later, by symbol_set_names. */
@@ -1386,6 +1400,9 @@
(msym, demangled_name,
&m_objfile->per_bfd->storage_obstack);
msym->name_set = 1;
+
+ hash_values[idx].mangled_name_hash
+ = fast_hash (msym->name, hash_values[idx].name_length);
}
}
{
@@ -1396,8 +1413,14 @@
#endif
for (minimal_symbol *msym = start; msym < end; ++msym)
{
- symbol_set_names (msym, msym->name, false,
- m_objfile->per_bfd);
+ size_t idx = msym - msymbols;
+ symbol_set_names
+ (msym,
+ gdb::string_view(msym->name,
+ hash_values[idx].name_length),
+ false,
+ m_objfile->per_bfd,
+ hash_values[idx].mangled_name_hash);
}
}
});
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 8f46321..894a323 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -836,7 +836,8 @@
void
symbol_set_names (struct general_symbol_info *gsymbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd)
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash)
{
struct demangled_name_entry **slot;
@@ -864,9 +865,11 @@
create_demangled_names_hash (per_bfd);
struct demangled_name_entry entry (linkage_name);
+ if (!hash.has_value ())
+ hash = hash_demangled_name_entry (&entry);
slot = ((struct demangled_name_entry **)
- htab_find_slot (per_bfd->demangled_names_hash.get (),
- &entry, INSERT));
+ htab_find_slot_with_hash (per_bfd->demangled_names_hash.get (),
+ &entry, *hash, INSERT));
/* If this name is not in the hash table, add it. */
if (*slot == NULL
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 7a51456..4cfdf06 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -553,7 +553,9 @@
(objfile)->per_bfd)
extern void symbol_set_names (struct general_symbol_info *symbol,
gdb::string_view linkage_name, bool copy_name,
- struct objfile_per_bfd_storage *per_bfd);
+ struct objfile_per_bfd_storage *per_bfd,
+ gdb::optional<hashval_t> hash
+ = gdb::optional<hashval_t> ());
/* Return true if NAME matches the "search" name of SYMBOL, according
to the symbol's language. */
--
Gerrit-Project: binutils-gdb
Gerrit-Branch: master
Gerrit-Change-Id: I044449e7eb60cffc1c43efd3412f2b485bd9faac
Gerrit-Change-Number: 307
Gerrit-PatchSet: 6
Gerrit-Owner: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Christian Biesinger <cbiesinger@google.com>
Gerrit-Reviewer: Tom Tromey <tromey@sourceware.org>
Gerrit-MessageType: newpatchset
^ permalink raw reply [flat|nested] 11+ messages in thread