[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: svn commit: r1333326 - in /subversion/trunk/subversion: include/private/svn_hash_private.h libsvn_fs_fs/temp_serializer.c libsvn_subr/hash.c

From: Julian Foad <julianfoad_at_btopenworld.com>
Date: Thu, 3 May 2012 10:24:54 +0100 (BST)

> URL: http://svn.apache.org/viewvc?rev=1333326&view=rev
> Introduce private API functions that wrap apr_hash_make_custom
> and return hash tables that are 2 to 4 times faster than the APR default.

Would it be sensible to propose these (the hash-functions) for inclusion in APR itself?

> Both yield repeatable results (each instance will store items in the same
> order if the keys are the same). The first, svn_hask__make will return

s/hask/hash/

> a hash table that behaves like pre APR 1.4.6 default hashes.
>
> * subversion/include/private/svn_hash_private.h
>   (svn_hash__make, svn_hash__make_fast): new private API

I suggest changing the names to

  svn_hash__make_compatible

  svn_hash__make  # faster than the 'compatible' one

because being compatible is a specific requirement that some callers have (that might compromise speed or other properties), so we make this property explicit, whereas being "fast" is a vague and relative term that no caller specifically needs or doesn't need, it's just generally good for any function to be as fast as possible within its specific constraints.

If we still have code that depends on the original hash ordering, then we wouldn't be able to drop in 'svn_hash__make' (the non-compatible version) as a direct replacement for apr_hash_make() throughout the code base -- but I don't know that we do have any such dependencies.

> * subversion/libsvn_subr/hash.c
>   (svn_hash_from_cstring_keys): use new API
>   (hashfunc_compatible, LOWER_7BITS_SET, BIT_7_SET, READ_CHUNK,
>    hashfunc_fast): implement efficient hash functions
>   (svn_hash__make, svn_hash__make_fast): implement new private API
> * subversion/libsvn_fs_fs/temp_serializer.c
>   (deserialize_dir, svn_fs_fs__deserialize_properties): use new API
>
>Modified: subversion/trunk/subversion/include/private/svn_hash_private.h
>==============================================================================
>--- subversion/trunk/subversion/include/private/svn_hash_private.h (original)
>+++ subversion/trunk/subversion/include/private/svn_hash_private.h Thu May  3 07:16:11 2012
>@@ -102,6 +102,31 @@ svn_hash__get_bool(apr_hash_t *hash,
>
>/** @} */
>
>+/**
>+ * @defgroup svn_hash_create Create optimized APR hash tables
>+ * @{
>+ */
>+
>+/** Returns a hash table, allocated in @a pool, with the same ordering of
>+ * elements as APR 1.4.5 or earlier (using apr_hashfunc_default) but uses
>+ * a faster hash function implementation.
>+ *
>+ * @since New in 1.8.
>+ */
>+apr_hash_t *
>+svn_hash__make(apr_pool_t *pool);
>+
>+/** Returns a hash table, allocated in @a pool, that is faster to modify
>+ * and access then the ones returned by @ref svn_hash__make. The element

s/faster...then/faster...than/

>+ * order does not match any APR default.
>+ *
>+ * @since New in 1.8.
>+ */
>+apr_hash_t *
>+svn_hash__make_fast(apr_pool_t *pool);
>+

>Modified: subversion/trunk/subversion/libsvn_subr/hash.c
>URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/hash.c?rev=1333326&r1=1333325&r2=1333326&view=diff
>==============================================================================
>--- subversion/trunk/subversion/libsvn_subr/hash.c (original)
>+++ subversion/trunk/subversion/libsvn_subr/hash.c Thu May  3 07:16:11 2012
>+/*** Optimized hash functions ***/
>+
>+/* Optimized version of apr_hashfunc_default. It assumes that the CPU has
>+ * 32-bit multiplications with high throughput of at least 1 operation
>+ * every 3 cycles. Latency is not an issue. Another optimization is a
>+ * mildly unrolled main loop.

Such specific details should at least refer to a specific version of apr_hashfunc_default().  Perhaps also (for the "1 op per 3 cycles" part, in particular) a specific system architecture or compiler.

>+ */
>+static unsigned int
>+hashfunc_compatible(const char *char_key, apr_ssize_t *klen)

- Julian
Received on 2012-05-03 11:25:33 CEST

This is an archived mail posted to the Subversion Dev mailing list.