This is the mail archive of the
gdb-patches@sourceware.cygnus.com
mailing list for the GDB project.
Re: (patch) hpjyg09: bcache optimizations
- To: Jimmy Guo <guo at cup dot hp dot com>
- Subject: Re: (patch) hpjyg09: bcache optimizations
- From: Andrew Cagney <ac131313 at cygnus dot com>
- Date: Fri, 05 Nov 1999 10:01:13 +1100
- CC: gdb-patches at sourceware dot cygnus dot com
- Organization: Cygnus Solutions
- References: <Pine.LNX.4.10.9911041332020.15719-100000@hpcll168.cup.hp.com>
Jimmy Guo wrote:
>
> ***
> Patch dependency: hpjyg05 (config/pa/tm-hppa.h)
> ***
>
> Couple of bcache.c optimizations:
>
> 1) Introduction and use of inlined hash function, and attempt to reduce
> the memcmp call.
Um,
If the bcache_hash code really is a performance hit can a static INLINE
C function be added to the header file instead of the macro? My
experience with simulators taught me that complex macro's should be
avoided like the plague - make it debuggable and then think about making
it fast.
On to the actual patch:
o The move to add a ``bcache_'' prefix (and fix some
name space problems is definitly good.
o Is the change duplicating the hash code -
BCACHE_HASH and bcache.c:hash()? I think
it would be safer if GDB only contained
one implementation of that code (even if it was
in a macro). hash() could use BCACHE_HASH.
o rather than adding:
+ /* CM: Use the inlined hash function if requested. */
+ #ifdef BCACHE_USE_INLINED_HASH
+ BCACHE_HASH (hashval, bytes, count);
+ #else
hashval = hash (bytes, count);
+ #endif
I suspect that it will be easier/cleaner to
just provide a default BCACHE_HASH definition:
(HASHVAL) = hash ((BYTES), (COUNT)).
o I'm in too minds over the first argument
(HASHVAL). I recently spent time eliminating
several really bad macro definitions that
abused the fact that they had direct access to
arguments - changing them to multi-arch functions
was, er, non-trivial.
However, in this case, the rational and use is
clean.
Just 1.26c worth,
Enjoy,
Andrew
> + /* CM: The hash method should exactly match the function defined below. */
> +
> + #define BCACHE_HASH(retval, bytes, temp_count) \
> + { \
> + int bcache_hash_count = (temp_count); \
> + unsigned int bcache_hash_len; \
> + unsigned long bcache_hash_hashval; \
> + unsigned int bcache_hash_c; \
> + const unsigned char *bcache_hash_data = (bytes); \
> + \
> + bcache_hash_hashval = 0; \
> + bcache_hash_len = 0; \
> + while (bcache_hash_count-- > 0) \
> + { \
> + bcache_hash_c = *bcache_hash_data++; \
> + bcache_hash_hashval += bcache_hash_c + (bcache_hash_c << 17); \
> + bcache_hash_hashval ^= bcache_hash_hashval >> 2; \
> + ++bcache_hash_len; \
> + } \
> + bcache_hash_hashval += bcache_hash_len + (bcache_hash_len << 17); \
> + bcache_hash_hashval ^= bcache_hash_hashval >> 2; \
> + (retval) = (bcache_hash_hashval % BCACHE_HASHSIZE); \
> + }