diff mbox

[0/5] cp-namespace.c cleanup pass

Message ID m37fxtihsy.fsf@seba.sebabeach.org
State New, archived
Headers

Commit Message

Doug Evans Dec. 15, 2014, 5:49 a.m. UTC 
  Hi.

This patch set is a cleanup pass over cp-namespace.c.

There are two things I wish to achieve here:

1a) Remove redundancy to make the code more readable.
    There are several redundant lookups here, and it's hard to reason
    about the code because one has to ask if there's something one is
    missing or whether the lookup really is redundant.  Plus it's more
    code to have to follow while studying the flow of any particular
    function.

1b) Performance.
    While redundant lookups don't always hurt performance if a symbol is
    found, efficient handling of the null case (symbol not found) is at
    least as important.

2)  Lay the groundwork so that I can finish this patch, that adds support
    for looking up primitive types as symbols:
    https://sourceware.org/ml/gdb-patches/2014-12/msg00169.html

1/5: whitespace
2/5: simplify cp_lookup_symbol_in_namespace
3/5: introduce cp_lookup_nested_symbol_1
4/5: remove redundant calls to cp_lookup_symbol_in_namespace
5/5: remove redundancies in cp_lookup_symbol_nonlocal

Appended is a patch I wrote to log symbol lookup calls while
running the testsuite, and here are the before/after numbers:

[dje@seba gdb]$ sed -e "s/ *(.*$//" </tmp/gdb-call.log >before.tmp
[dje@seba gdb]$ sort before.tmp | uniq -c
  40230 basic_lookup_symbol_nonlocal
  74647 lookup_global_symbol
  18834 lookup_language_this
  73063 lookup_local_symbol
  30849 lookup_static_symbol
  73063 lookup_symbol_aux
  59411 lookup_symbol_in_block
 656240 lookup_symbol_in_objfile
     23 lookup_symbol_in_objfile_from_linkage_name
 656298 lookup_symbol_in_objfile_symtabs
  74075 lookup_symbol_in_static_block
 614899 lookup_symbol_via_quick_fns

[dje@seba gdb]$ sed -e "s/ *(.*$//" </tmp/gdb-call.log >after.tmp
[dje@seba gdb]$ sort after.tmp | uniq -c
  39019 basic_lookup_symbol_nonlocal
  61959 lookup_global_symbol
   9614 lookup_language_this
  71826 lookup_local_symbol
  29565 lookup_static_symbol
  71826 lookup_symbol_aux
  55214 lookup_symbol_in_block
 504688 lookup_symbol_in_objfile
     23 lookup_symbol_in_objfile_from_linkage_name
 504746 lookup_symbol_in_objfile_symtabs
  63217 lookup_symbol_in_static_block
 463884 lookup_symbol_via_quick_fns

There's a clear reduction in the number of symbol looks being done here.
A later patch will augment our performance testsuite, but my main focus
at the moment is getting to the "use the index better" speedup patch.
[And along the way cleaning things up so that I understand the code
better, and *hopefully* making it so that others will too.]

Regression tested on amd64-linux.

I still need to do a bit more testing,
but it's at the point where it could use another pair of eyes.


---

Comments

Doug Evans Dec. 22, 2014, 5:32 p.m. UTC | #1 
Doug Evans <xdje42@gmail.com> writes:
> Hi.
>
> This patch set is a cleanup pass over cp-namespace.c.
>
> There are two things I wish to achieve here:
>
> 1a) Remove redundancy to make the code more readable.
>     There are several redundant lookups here, and it's hard to reason
>     about the code because one has to ask if there's something one is
>     missing or whether the lookup really is redundant.  Plus it's more
>     code to have to follow while studying the flow of any particular
>     function.
>
> 1b) Performance.
>     While redundant lookups don't always hurt performance if a symbol is
>     found, efficient handling of the null case (symbol not found) is at
>     least as important.
>
> 2)  Lay the groundwork so that I can finish this patch, that adds support
>     for looking up primitive types as symbols:
>     https://sourceware.org/ml/gdb-patches/2014-12/msg00169.html
>
> 1/5: whitespace
> 2/5: simplify cp_lookup_symbol_in_namespace
> 3/5: introduce cp_lookup_nested_symbol_1
> 4/5: remove redundant calls to cp_lookup_symbol_in_namespace
> 5/5: remove redundancies in cp_lookup_symbol_nonlocal
>
> Appended is a patch I wrote to log symbol lookup calls while
> running the testsuite, and here are the before/after numbers:
>
> [dje@seba gdb]$ sed -e "s/ *(.*$//" </tmp/gdb-call.log >before.tmp
> [dje@seba gdb]$ sort before.tmp | uniq -c
>   40230 basic_lookup_symbol_nonlocal
>   74647 lookup_global_symbol
>   18834 lookup_language_this
>   73063 lookup_local_symbol
>   30849 lookup_static_symbol
>   73063 lookup_symbol_aux
>   59411 lookup_symbol_in_block
>  656240 lookup_symbol_in_objfile
>      23 lookup_symbol_in_objfile_from_linkage_name
>  656298 lookup_symbol_in_objfile_symtabs
>   74075 lookup_symbol_in_static_block
>  614899 lookup_symbol_via_quick_fns
>
> [dje@seba gdb]$ sed -e "s/ *(.*$//" </tmp/gdb-call.log >after.tmp
> [dje@seba gdb]$ sort after.tmp | uniq -c
>   39019 basic_lookup_symbol_nonlocal
>   61959 lookup_global_symbol
>    9614 lookup_language_this
>   71826 lookup_local_symbol
>   29565 lookup_static_symbol
>   71826 lookup_symbol_aux
>   55214 lookup_symbol_in_block
>  504688 lookup_symbol_in_objfile
>      23 lookup_symbol_in_objfile_from_linkage_name
>  504746 lookup_symbol_in_objfile_symtabs
>   63217 lookup_symbol_in_static_block
>  463884 lookup_symbol_via_quick_fns
>
> There's a clear reduction in the number of symbol looks being done here.
> A later patch will augment our performance testsuite, but my main focus
> at the moment is getting to the "use the index better" speedup patch.
> [And along the way cleaning things up so that I understand the code
> better, and *hopefully* making it so that others will too.]

Hi.  fyi,
I committed this series.
diff mbox

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index 54e4be4..85eaf10 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -18,6 +18,7 @@ 
    along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
 
 #include "defs.h"
+#include <stdarg.h>
 #include "symtab.h"
 #include "gdbtypes.h"
 #include "gdbcore.h"
@@ -1264,6 +1265,26 @@  demangle_for_lookup (const char *name, enum language lang,
   return cleanup;
 }
 
+extern void log_call (const char *fmt, ...);
+
+void
+log_call (const char *fmt, ...)
+{
+  static FILE *f;
+  va_list args;
+
+  if (f == NULL)
+    f = fopen ("/tmp/gdb-call.log", "a");
+  if (f == NULL)
+    abort ();
+
+  va_start (args, fmt);
+  vfprintf (f, fmt, args);
+  va_end (args);
+  fputc ('\n', f);
+  fflush (f);
+}
+
 /* See symtab.h.
 
    This function (or rather its subordinates) have a bunch of loops and
@@ -1310,6 +1331,8 @@  struct symbol *
 lookup_language_this (const struct language_defn *lang,
 		      const struct block *block)
 {
+  log_call ("lookup_language_this (...)");
+
   if (lang->la_name_of_this == NULL || block == NULL)
     return NULL;
 
@@ -1387,6 +1410,8 @@  lookup_symbol_aux (const char *name, const struct block *block,
   struct symbol *sym;
   const struct language_defn *langdef;
 
+  log_call ("lookup_symbol_aux (%s, ...)", name);
+
   /* Make sure we do something sensible with is_a_field_of_this, since
      the callers that set this parameter to some non-null value will
      certainly use it later.  If we don't set it, the contents of
@@ -1458,6 +1483,8 @@  lookup_local_symbol (const char *name, const struct block *block,
   struct symbol *sym;
   const struct block *static_block = block_static_block (block);
   const char *scope = block_scope (block);
+
+  log_call ("lookup_local_symbol (%s, ...)", name);
   
   /* Check if either no block is specified or it's a global block.  */
 
@@ -1522,6 +1549,8 @@  lookup_symbol_in_block (const char *name, const struct block *block,
 {
   struct symbol *sym;
 
+  log_call ("lookup_symbol_in_block (%s, ...)", name);
+
   sym = block_lookup_symbol (block, name, domain);
   if (sym)
     {
@@ -1541,6 +1570,8 @@  lookup_global_symbol_from_objfile (struct objfile *main_objfile,
 {
   struct objfile *objfile;
 
+  log_call ("lookup_global_symbol_from_objfile (%s, ...)", name);
+
   for (objfile = main_objfile;
        objfile;
        objfile = objfile_separate_debug_iterate (main_objfile, objfile))
@@ -1568,6 +1599,8 @@  lookup_symbol_in_objfile_symtabs (struct objfile *objfile, int block_index,
 
   gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK);
 
+  log_call ("lookup_symbol_in_objfile_symtabs (%s, ...)", name);
+
   ALL_OBJFILE_COMPUNITS (objfile, cust)
     {
       const struct blockvector *bv;
@@ -1607,6 +1640,8 @@  lookup_symbol_in_objfile_from_linkage_name (struct objfile *objfile,
 						 &modified_name);
   struct objfile *main_objfile, *cur_objfile;
 
+  log_call ("lookup_symbol_in_objfile_from_linkage_name (%s, ...)", linkage_name);
+
   if (objfile->separate_debug_objfile_backlink)
     main_objfile = objfile->separate_debug_objfile_backlink;
   else
@@ -1663,6 +1698,8 @@  lookup_symbol_via_quick_fns (struct objfile *objfile, int block_index,
   const struct block *block;
   struct symbol *sym;
 
+  log_call ("lookup_symbol_via_quick_fns (%s, ...)", name);
+
   if (!objfile->sf)
     return NULL;
   cust = objfile->sf->qf->lookup_symbol (objfile, block_index, name, domain);
@@ -1719,6 +1756,8 @@  basic_lookup_symbol_nonlocal (const char *name,
      the current objfile.  Searching the current objfile first is useful
      for both matching user expectations as well as performance.  */
 
+  log_call ("basic_lookup_symbol_nonlocal (%s, ...)", name);
+
   sym = lookup_symbol_in_static_block (name, block, domain);
   if (sym != NULL)
     return sym;
@@ -1735,6 +1774,8 @@  lookup_symbol_in_static_block (const char *name,
 {
   const struct block *static_block = block_static_block (block);
 
+  log_call ("lookup_symbol_in_static_block (%s, ...)", name);
+
   if (static_block != NULL)
     return lookup_symbol_in_block (name, static_block, domain);
   else
@@ -1752,6 +1793,8 @@  lookup_symbol_in_objfile (struct objfile *objfile, int block_index,
 {
   struct symbol *result;
 
+  log_call ("lookup_symbol_in_objfile (%s, ...)", name);
+
   result = lookup_symbol_in_objfile_symtabs (objfile, block_index,
 					     name, domain);
   if (result == NULL)
@@ -1771,6 +1814,8 @@  lookup_static_symbol (const char *name, const domain_enum domain)
   struct objfile *objfile;
   struct symbol *result;
 
+  log_call ("lookup_static_symbol (%s, ...)", name);
+
   ALL_OBJFILES (objfile)
     {
       result = lookup_symbol_in_objfile (objfile, STATIC_BLOCK, name, domain);
@@ -1829,6 +1874,8 @@  lookup_global_symbol (const char *name,
   struct objfile *objfile = NULL;
   struct global_sym_lookup_data lookup_data;
 
+  log_call ("lookup_global_symbol (%s, ...)", name);
+
   /* Call library-specific lookup procedure.  */
   objfile = lookup_objfile_from_block (block);
   if (objfile != NULL)