Re: [rfa] Symbol hashing (for the last time?)

[Daniel Jacobowitz <drow at mvista dot com>]

On Thu, Jul 11, 2002 at 04:00:02PM -0400, Elena Zannoni wrote:
> Daniel Jacobowitz writes:
>  > Here's a patch from last October, dusted off and merged to the current
>  > sources.  The only substantial changes were some fixes for ada-lang.c,
>  > merged after I wrote the original patch.  I've verified no regressions
>  > on i386-linux for GCC (2.95,3.0.4,3.1)/(stabs,dwarf2).
>  > 
>  > This converts the normal symbol table lookups into hash tables.  A few
>  > sorts of symbol tables aren't hashed: those produced by mdebugread.c
>  > and dstread.c, because they build symbol tables in lots of ad-hoc code,
>  > and symbol tables which are actually the arguments to a function
>  > (because order matters, or at least comments suggest so). 
> 
> Would you mind adding the paragraph above, to a comment in the block
> structure, maybe above the hashtable flag?

Sure.

> >  A next step
>  > will be to convert mdebugread.c, delete dstread.c (it's marked for an
>  > upcoming obsoletion, isn't it?), and then delete all the complicated
>  > binary search code since the only remaining unhashed symtabs will be
>  > argument lists, which are small.
>  > 
> 
> how about os9kread? Anything needs to be done there?

Nope (though that one's scheduled to go away too, I think?).  Any
reader which uses finish_block is fine.

>  > 	(ALL_BLOCK_SYMBOLS): New macro.
> 
> This is not really true. The macro was there already.

Yup, just noticed that.  Thanks.  I added it in an earlier patch and
never updated my changelog.

>  > 	(BLOCK_SHOULD_SORT): Do not sort hashed blocks.
>  > 	(struct symbol): Add `hash_next' pointer.
>  > 	* symtab.c (lookup_block_symbol): Search using the hash table when
>  > 	possible.
>  > 	(find_pc_sect_symtab): Use ALL_BLOCK_SYMBOLS.
>  > 	(search_symbols, find_addr_symbol): Likewise.
>  > 
>  > 	* dstread.c (process_dst_block): Clear hashtable bit for new block.
>  > 	(read_dst_symtab): Likewise.
>  > 	* jv-lang.c (get_java_class_symtab): Likewise.
>  > 	* mdebugread.c: Include "gdb_assert.h".
>  > 	(shrink_block): Assert that the block being modified is not hashed.
>  > 	* coffread.c (patch_opaque_types): Use ALL_BLOCK_SYMBOLS.
>  > 	* symmisc.c (free_symtab_block): Walk the hash table when freeing
>  > 	symbols.
>  > 	(dump_symtab): Recognize hashed blocks.
>  > 	* printcmd.c (print_frame_args):  Assert that function blocks do not
>  > 	have hashed symbol tables.
>  > 	* ada-lang.c (symtab_for_sym): Use ALL_BLOCK_SYMBOLS.
>  > 	(fill_in_ada_prototype, debug_print_block): Likewise.
>  > 	(ada_add_block_symbols): Use ALL_BLOCK_SYMBOLS.  Handle hash tables.
>  > 
> 
> I now wonder if it would be better to define another 'for' macro for this:
> for (sym = BLOCK_BUCKET (block, hash_index); sym; sym = sym->hash_next)
> it seems to reoccur a few times. Even though it's not that important.

I think I'll avoid introducing any more of those if I possibly can....

> I don't like too much the use of the hardcoded '5' maybe define a variable?

I thought about this for a little bit and settled on:

/* Macro used to set the size of a hashtable for N symbols.  */
#define BLOCK_HASHTABLE_SIZE(n) ((N)/5 + 1)

I don't really see the point in making it user tunable; if we ever find
a case that this one is bad for, I'll gladly change my mind, though.

> Otherwise it's ok.
> (modulus the problem you found).

Thanks!  Committed.

-- 
Daniel Jacobowitz                           Carnegie Mellon University
MontaVista Software                         Debian GNU/Linux Developer