Re: [patch] Do not add partial_symbol again and again to the list

[Aleksandar Ristovski <aristovski@qnx.com>]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

Daniel Jacobowitz wrote:
> On Mon, Feb 11, 2008 at 04:41:08PM -0500, Aleksandar Ristovski wrote:
>> Daniel Jacobowitz wrote:
>>> On Mon, Feb 11, 2008 at 03:51:59PM -0500, Aleksandar Ristovski wrote:
>>>> Daniel Jacobowitz wrote:
>>>>> On Mon, Feb 11, 2008 at 03:23:35PM -0500, Aleksandar Ristovski wrote:
>>>>>> The attached patch checks if partial_symbol has already been added 
>>>>>> to the list instead of adding duplicate records.
>>>>> How does this ever happen?  It seems very wrong.  Also, I am worried
>>>>> that the linear search will be a bottleneck (this is quadratic as each
>>>>> psymtab grows).
>>>> Yes, I understand your concern about the complexity... but...
>>> That's only part of the problem.  You have this huge duplication of
>>> identical partial symbols within the same block.  How did that happen?
>>> It shouldn't.  Maybe we can avoid creating them in the first place.
>>>
>> Daniel, could you clarify: when you say "maybe we can avoid..." who is 
>> "we" - gdb or gcc?
> 
> Probably GDB, but I don't know.  Can you show me an example of these
> unnecessary psymbols?
> 
Some of them:
unsigned int
_GCC_ATTR_ALIGN_64t
long long int
_GCC_ATTR_ALIGN_u64t
long long unsigned int
_Int64t
_Uint64t
_GCC_ATTR_ALIGN_u32t
unsigned int
_GCC_ATTR_ALIGN_32t
int
_Uint32t
_Int32t
_GCC_ATTR_ALIGN_16t
short int
_GCC_ATTR_ALIGN_u16t
short unsigned int
_Int16t
_Uint16t
_GCC_ATTR_ALIGN_8t
signed char
_GCC_ATTR_ALIGN_u8t
unsigned char
_Int8t
_Uint8t
_Intptrt
_Uintptrt
_Longlong
_ULonglong

Additionally, please take a look at the modified patch, I have added 
bcache_added function to return whether it added the data or returned cached 
one. This way linear search is avoided.


[-- Attachment #2: bcache_added_donotduplicatepsyms.diff --]
[-- Type: text/plain, Size: 4007 bytes --]

Index: gdb/bcache.h
===================================================================
RCS file: /cvs/src/src/gdb/bcache.h,v
retrieving revision 1.12
diff -u -p -r1.12 bcache.h
--- gdb/bcache.h	1 Jan 2008 22:53:09 -0000	1.12
+++ gdb/bcache.h	11 Feb 2008 21:55:41 -0000
@@ -150,6 +150,8 @@ extern void *deprecated_bcache (const vo
 extern const void *bcache (const void *addr, int length,
 			   struct bcache *bcache);
 
+extern void *bcache_added (const void *addr, int length,
+				 struct bcache *bcache, int *added);
 /* Free all the storage used by BCACHE.  */
 extern void bcache_xfree (struct bcache *bcache);
 
Index: gdb/bcache.c
===================================================================
RCS file: /cvs/src/src/gdb/bcache.c,v
retrieving revision 1.20
diff -u -p -r1.20 bcache.c
--- gdb/bcache.c	1 Jan 2008 22:53:09 -0000	1.20
+++ gdb/bcache.c	11 Feb 2008 21:55:41 -0000
@@ -197,11 +197,34 @@ expand_hash_table (struct bcache *bcache
 static void *
 bcache_data (const void *addr, int length, struct bcache *bcache)
 {
+  return bcache_added (addr, length, bcache, NULL);
+}
+
+
+void *
+deprecated_bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+const void *
+bcache (const void *addr, int length, struct bcache *bcache)
+{
+  return bcache_data (addr, length, bcache);
+}
+
+void *
+bcache_added (const void *addr, int length, struct bcache *bcache, 
+		   int *added)
+{
   unsigned long full_hash;
   unsigned short half_hash;
   int hash_index;
   struct bstring *s;
 
+  if (added)
+    *added = 0;
+
   /* If our average chain length is too high, expand the hash table.  */
   if (bcache->unique_count >= bcache->num_buckets * CHAIN_LENGTH_THRESHOLD)
     expand_hash_table (bcache);
@@ -242,21 +265,12 @@ bcache_data (const void *addr, int lengt
     bcache->unique_size += length;
     bcache->structure_size += BSTRING_SIZE (length);
 
+    if (added)
+      *added = 1;
+
     return &new->d.data;
   }
 }
-
-void *
-deprecated_bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
-
-const void *
-bcache (const void *addr, int length, struct bcache *bcache)
-{
-  return bcache_data (addr, length, bcache);
-}
 \f
 /* Allocating and freeing bcaches.  */
 
Index: gdb/symfile.c
===================================================================
RCS file: /cvs/src/src/gdb/symfile.c,v
retrieving revision 1.198
diff -u -p -r1.198 symfile.c
--- gdb/symfile.c	29 Jan 2008 22:47:20 -0000	1.198
+++ gdb/symfile.c	11 Feb 2008 21:55:46 -0000
@@ -3102,15 +3103,20 @@ add_psymbol_to_list (char *name, int nam
 		     enum language language, struct objfile *objfile)
 {
   struct partial_symbol *psym;
-  char *buf = alloca (namelength + 1);
+  int added;
+  char *buf = name;  
   /* psymbol is static so that there will be no uninitialized gaps in the
      structure which might contain random data, causing cache misses in
      bcache. */
   static struct partial_symbol psymbol;
-
-  /* Create local copy of the partial symbol */
-  memcpy (buf, name, namelength);
-  buf[namelength] = '\0';
+  
+  if (name[namelength] != '\0')
+    {
+      buf = alloca (namelength + 1);
+      /* Create local copy of the partial symbol */
+      memcpy (buf, name, namelength);
+      buf[namelength] = '\0';
+    }
   /* val and coreaddr are mutually exclusive, one of them *will* be zero */
   if (val != 0)
     {
@@ -3128,8 +3134,11 @@ add_psymbol_to_list (char *name, int nam
   SYMBOL_SET_NAMES (&psymbol, buf, namelength, objfile);
 
   /* Stash the partial symbol away in the cache */
-  psym = deprecated_bcache (&psymbol, sizeof (struct partial_symbol),
-			    objfile->psymbol_cache);
+  psym = bcache_added (&psymbol, sizeof (struct partial_symbol),
+			    objfile->psymbol_cache, &added);
+
+  if (!added)
+    return psym;
 
   /* Save pointer to partial symbol in psymtab, growing symtab if needed. */
   if (list->next >= list->list + list->size)