This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

PATCH: performance improvement in lookup_symtab()


A customer of ours observed a noticable first-time 
performance penalty setting breakpoints.

Since we are using absolute file paths to specify the 
breakpoint, GDB was following a code path in lookup_symtab() 
which was expensive because it was computing the fullpaths 
and realpaths for every item in the symtabs and partial 
symtabs as it scanned linearly for the matching file.

The customer had obvserved startup delays of up to 30 
seconds on account of this issue.  They report that there is no
delay when NOT sending absolute paths for breakpoint
file specs.  However, sending unqualified file names is not a 
general solution, it is really not a solution at all.
 
The attached patch optimizes the absolute path case by first 
qualifying the symbol table entry's base file name against the 
name we are searching for.  If the base file name does not match, 
then there is no point in calculating fullpaths or realpaths 
for the symtab in question.
 
The only caveat to the changes is that it will potentially 
fail to find a file that is symlinked to a symlink to a file 
with a different name.  That's not a typo -- a single symlink 
will work because I test for the base file name both before
and after xfullpath() -- the file name has to be symlinked 
twice to cause the search to fail.  This is the case where:
 
    FILENAME_CMP( lbasename( name            ), lbasename( s->filename ) ) != 0
         and
    FILENAME_CMP( lbasename( xfullpath(name) ), lbasename( s->filename ) ) != 0
        and
    FILENAME_CMP( xfullpath(name), symtab_to_fullname(s) ) == 0
 
'name' basically has to be the middle symlink for this to fail.
 
In my books, this is an acceptable issue, considering that the 
mainstream code path when the path was not absolute could not 
deal with symlinks at all, and also considering that I'm making 
the assumption that symtab_to_fullname(s->filename) would resolve 
a symlink if there was one.  Also, keep in mind, these are file
name symlinks, not directory symlinks, so this is not even
a remotely common use case, much less a chain of two such links.

I will have no objection if the extra comparison is
removed entirely and we just don't worry about symlinks at all.

Similar performance improvements to lookup_symtab() have been
suggested before, but never integrated.  Knowing how expensive
symtab_to_fullname() can be, especially the cost of doing it for
EVERY single symbol table, I think this fix is pretty much a no-brainer.
GDB needs this, especially for the MI controlled case, where apps
are forced to send absolute file paths as that is the logical way
to handle setting breakpoints in same-named files in different
directories (like the example below):

       /src/electronics/controller.cpp
       /src/patterns/mvc/controller.cpp
       /src/wife/controller.cpp

 
ChangeLog:
 
2008-06-03  Dennis Brueni  <dbrueni@nc.rr.com>

* symtab.c (lookup_symtab and lookup_partial_symtab): 
  Optimized case where file name is an absolute path.


Index: symtab.c
===================================================================
RCS file: /cvs/src/src/gdb/symtab.c,v
retrieving revision 1.189
diff -c -p -r1.189 symtab.c
*** symtab.c	27 May 2008 19:29:51 -0000	1.189
--- symtab.c	3 Jun 2008 21:43:33 -0000
*************** struct symtab *
*** 160,169 ****
--- 160,174 ----
  lookup_symtab (const char *name)
  {
    struct symtab *s;
+   struct symtab *s_base = NULL;
    struct partial_symtab *ps;
    struct objfile *objfile;
    char *real_path = NULL;
    char *full_path = NULL;
+   const char *real_path_base = NULL;
+   const char *full_path_base = NULL;
+   const char *s_file_base = NULL;
+   const char *name_file_base = lbasename(name);
  
    /* Here we are interested in canonicalizing an absolute path, not
       absolutizing a relative path.  */
*************** lookup_symtab (const char *name)
*** 173,178 ****
--- 178,185 ----
        make_cleanup (xfree, full_path);
        real_path = gdb_realpath (name);
        make_cleanup (xfree, real_path);
+       real_path_base = lbasename(real_path);
+       full_path_base = lbasename(full_path);
      }
  
  got_symtab:
*************** got_symtab:
*** 185,225 ****
        {
  	return s;
        }
-       
-     /* If the user gave us an absolute path, try to find the file in
-        this symtab and use its absolute path.  */
      
!     if (full_path != NULL)
!       {
!         const char *fp = symtab_to_fullname (s);
!         if (fp != NULL && FILENAME_CMP (full_path, fp) == 0)
!           {
!             return s;
!           }
        }
  
!     if (real_path != NULL)
        {
!         char *fullname = symtab_to_fullname (s);
!         if (fullname != NULL)
!           {
!             char *rp = gdb_realpath (fullname);
!             make_cleanup (xfree, rp);
!             if (FILENAME_CMP (real_path, rp) == 0)
!               {
!                 return s;
!               }
!           }
        }
    }
  
    /* Now, search for a matching tail (only if name doesn't have any dirs) */
  
!   if (lbasename (name) == name)
!     ALL_SYMTABS (objfile, s)
      {
!       if (FILENAME_CMP (lbasename (s->filename), name) == 0)
! 	return s;
      }
  
    /* Same search rules as above apply here, but now we look thru the
--- 192,251 ----
        {
  	return s;
        }
      
!     /* If the user gave us an absolute path, try to find the file in
!        this symtab and use its absolute path.
!        Note that if the base file name doesn't match, then there's
!        no point in checking absolute or real paths
!        */
!     s_file_base = lbasename(s->filename);
! 
!     if (full_path != NULL) 
!       { 
! 	/* compare both, in case if the file is symlink'd */
! 	if (FILENAME_CMP(full_path_base, s_file_base) == 0 ||
! 	    FILENAME_CMP(name_file_base, s_file_base) == 0)
! 	  {
! 	    const char *fp = symtab_to_fullname (s);
! 	    if (fp != NULL && FILENAME_CMP (full_path, fp) == 0)
! 	      {
! 		return s;
! 	      }
! 	  }
!       }
! 
!     if (real_path != NULL) 
!       { 
! 	/* compare both, in case if the file is symlink'd */
! 	if (FILENAME_CMP(real_path_base, s_file_base) == 0 ||
! 	    FILENAME_CMP(name_file_base, s_file_base) == 0)
! 	  {
! 	    char *fullname = symtab_to_fullname (s);
! 	    if (fullname != NULL)
! 	      {
! 		char *rp = gdb_realpath (fullname);
! 		make_cleanup (xfree, rp);
! 		if (FILENAME_CMP (real_path, rp) == 0)
! 		  {
! 		    return s;
! 		  }
! 	      }
! 	  }
        }
  
!     /* Check if we might have an unqualified match */
!     if (name_file_base==name && s_base==NULL)
        {
!         if (FILENAME_CMP (s_file_base, name) == 0)
!           s_base = s;
        }
    }
  
    /* Now, search for a matching tail (only if name doesn't have any dirs) */
  
!   if (name_file_base==name && s_base!=NULL)
      {
!       return s_base;
      }
  
    /* Same search rules as above apply here, but now we look thru the
*************** struct partial_symtab *
*** 257,265 ****
--- 283,296 ----
  lookup_partial_symtab (const char *name)
  {
    struct partial_symtab *pst;
+   struct partial_symtab *pst_base = NULL;
    struct objfile *objfile;
    char *full_path = NULL;
    char *real_path = NULL;
+   const char *full_path_base = NULL;
+   const char *real_path_base = NULL;
+   const char *pst_file_base = NULL;
+   const char *name_file_base = lbasename(name);
  
    /* Here we are interested in canonicalizing an absolute path, not
       absolutizing a relative path.  */
*************** lookup_partial_symtab (const char *name)
*** 269,274 ****
--- 300,307 ----
        make_cleanup (xfree, full_path);
        real_path = gdb_realpath (name);
        make_cleanup (xfree, real_path);
+       full_path_base = lbasename(full_path);
+       real_path_base = lbasename(real_path);
      }
  
    ALL_PSYMTABS (objfile, pst)
*************** lookup_partial_symtab (const char *name)
*** 279,318 ****
        }
  
      /* If the user gave us an absolute path, try to find the file in
!        this symtab and use its absolute path.  */
!     if (full_path != NULL)
!       {
! 	psymtab_to_fullname (pst);
! 	if (pst->fullname != NULL
! 	    && FILENAME_CMP (full_path, pst->fullname) == 0)
  	  {
! 	    return pst;
  	  }
        }
  
!     if (real_path != NULL)
!       {
!         char *rp = NULL;
! 	psymtab_to_fullname (pst);
!         if (pst->fullname != NULL)
!           {
!             rp = gdb_realpath (pst->fullname);
!             make_cleanup (xfree, rp);
!           }
! 	if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
  	  {
! 	    return pst;
  	  }
        }
    }
  
    /* Now, search for a matching tail (only if name doesn't have any dirs) */
! 
!   if (lbasename (name) == name)
!     ALL_PSYMTABS (objfile, pst)
      {
!       if (FILENAME_CMP (lbasename (pst->filename), name) == 0)
! 	return (pst);
      }
  
    return (NULL);
--- 312,370 ----
        }
  
      /* If the user gave us an absolute path, try to find the file in
!        this symtab and use its absolute path.
!        Note that if the base file name doesn't match, then there's
!        no point in checking absolute or real paths
!        */
!     pst_file_base = lbasename(pst->filename);
! 
!     if (full_path != NULL) 
!       { 
! 	/* compare both, in case if the file is symlink'd */
! 	if (FILENAME_CMP(full_path_base, pst_file_base) == 0 ||
! 	    FILENAME_CMP(name_file_base, pst_file_base) == 0)
  	  {
! 	    psymtab_to_fullname (pst);
! 	    if (pst->fullname != NULL
! 		&& FILENAME_CMP (full_path, pst->fullname) == 0)
! 	      {
! 		return pst;
! 	      }
  	  }
        }
  
!     if (real_path != NULL) 
!       { 
! 	/* compare both, in case if the file is symlink'd */
! 	if (FILENAME_CMP(real_path_base, pst_file_base) == 0 ||
! 	    FILENAME_CMP(name_file_base, pst_file_base) == 0)
  	  {
! 	    char *rp = NULL;
! 	    psymtab_to_fullname (pst);
! 	    if (pst->fullname != NULL)
! 	      {
! 		rp = gdb_realpath (pst->fullname);
! 		make_cleanup (xfree, rp);
! 	      }
! 	    if (rp != NULL && FILENAME_CMP (real_path, rp) == 0)
! 	      {
! 		return pst;
! 	      }
  	  }
        }
+ 
+     /* Now, check for a matching tail (only if name doesn't have any dirs) */
+     if (name_file_base==name && pst_base==NULL)
+       {
+         if (FILENAME_CMP (pst_file_base, name) == 0)
+           return (pst);
+       }
    }
  
    /* Now, search for a matching tail (only if name doesn't have any dirs) */
!   if (name_file_base==name && pst_base!=NULL)
      {
!       return pst_base;
      }
  
    return (NULL);
 


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]