Nah. "thread_safe = TRUE" means "use mutexes if necessary";
"thread_safe = FALSE" means "this object will always be kept within
one thread". It's a function of whether or not the cache could
theoretically be shared between threads. If APR doesn't have threads,
then there's n problem. I don't think there's any need to have the
ugliness of an API whose argument count depends on the existence of
APR_HAS_THREADS.
--dave
On Tue, Mar 11, 2008 at 5:36 PM, Daniel L. Rall <dlr_at_finemaltcoding.com> wrote:
> We should assert() or something if thread_safe is true, but APR_HAS_THREADS
> is false.
>
>
>
> On Tue, 11 Mar 2008, glasser_at_tigris.org wrote:
>
> > Author: glasser
> > Date: Tue Mar 11 17:11:32 2008
> > New Revision: 29866
> >
> > Log:
> > On the in-memory-cache branch:
> >
> > Support thread_safe argument.
> >
> > * subversion/include/svn_cache.h
> > (svn_cache_create): Document that dup_func can't interact safely
> > with the cache.
> >
> > * subversion/libsvn_subr/cache.c
> > (struct svn_cache_t): Add mutex field.
> > (svn_cache_create): Create mutex.
> > (lock_cache, unlock_cache): New.
> > (svn_cache_get, svn_cache_set): Lock and unlock cache.
> >
> >
> > Modified:
> > branches/in-memory-cache/subversion/include/svn_cache.h
> > branches/in-memory-cache/subversion/libsvn_subr/cache.c
> >
> > Modified: branches/in-memory-cache/subversion/include/svn_cache.h
> > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/include/svn_cache.h?pathrev=29866&r1=29865&r2=29866
> > ==============================================================================
> > --- branches/in-memory-cache/subversion/include/svn_cache.h (original)
> > +++ branches/in-memory-cache/subversion/include/svn_cache.h Tue Mar 11 17:11:32 2008
> > @@ -70,6 +70,8 @@ typedef struct svn_cache_t svn_cache_t;
> > *
> > * Note that NULL is a legitimate value for cache entries (and @a dup_func
> > * will not be called on it).
> > + *
> > + * It is not safe for @a dup_func to interact with the cache itself.
> > */
> > svn_error_t *
> > svn_cache_create(svn_cache_t **cache_p,
> >
> > Modified: branches/in-memory-cache/subversion/libsvn_subr/cache.c
> > URL: http://svn.collab.net/viewvc/svn/branches/in-memory-cache/subversion/libsvn_subr/cache.c?pathrev=29866&r1=29865&r2=29866
> > ==============================================================================
> > --- branches/in-memory-cache/subversion/libsvn_subr/cache.c (original)
> > +++ branches/in-memory-cache/subversion/libsvn_subr/cache.c Tue Mar 11 17:11:32 2008
> > @@ -18,9 +18,13 @@
> >
> > #include <assert.h>
> >
> > +#include <apr_thread_mutex.h>
> > +
> > #include "svn_cache.h"
> > #include "svn_pools.h"
> >
> > +#include "svn_private_config.h"
> > +
> > /* The cache object. */
> > struct svn_cache_t {
> > /* Maps from a key (of size CACHE->KLEN) to a struct cache_entry. */
> > @@ -55,6 +59,13 @@ struct svn_cache_t {
> > * structs, as well as the dup'd values and hash keys.
> > */
> > apr_pool_t *cache_pool;
> > +
> > +#if APR_HAS_THREADS
> > + /* A lock for intra-process synchronization to the cache, or NULL if
> > + * the cache's creator doesn't feel the cache needs to be
> > + * thread-safe. */
> > + apr_thread_mutex_t *mutex;
> > +#endif
> > };
> >
> > /* A cache page; all items on the page are allocated from the same
> > @@ -115,7 +126,17 @@ svn_cache_create(svn_cache_t **cache_p,
> > /* The sentinel doesn't need a pool. (We're happy to crash if we
> > * accidentally try to treat it like a real page.) */
> >
> > - /* ### TODO: mutex */
> > +#if APR_HAS_THREADS
> > + if (thread_safe)
> > + {
> > + apr_status_t status = apr_thread_mutex_create(&(cache->mutex),
> > + APR_THREAD_MUTEX_DEFAULT,
> > + pool);
> > + if (status)
> > + return svn_error_wrap_apr(status,
> > + _("Can't create cache mutex"));
> > + }
> > +#endif
> >
> > cache->cache_pool = pool;
> >
> > @@ -188,6 +209,41 @@ duplicate_key(svn_cache_t *cache,
> > return apr_pmemdup(pool, key, cache->klen);
> > }
> >
> > +/* If applicable, locks CACHE's mutex. */
> > +static svn_error_t *
> > +lock_cache(svn_cache_t *cache)
> > +{
> > +#if APR_HAS_THREADS
> > + apr_status_t status;
> > + if (! cache->mutex)
> > + return SVN_NO_ERROR;
> > +
> > + status = apr_thread_mutex_lock(cache->mutex);
> > + if (status)
> > + return svn_error_wrap_apr(status, _("Can't lock cache mutex"));
> > +#endif
> > +
> > + return SVN_NO_ERROR;
> > +}
> > +
> > +/* If applicable, unlocks CACHE's mutex, then returns ERR. */
> > +static svn_error_t *
> > +unlock_cache(svn_cache_t *cache,
> > + svn_error_t *err)
> > +{
> > +#if APR_HAS_THREADS
> > + apr_status_t status;
> > + if (! cache->mutex)
> > + return err;
> > +
> > + status = apr_thread_mutex_unlock(cache->mutex);
> > + if (status && !err)
> > + return svn_error_wrap_apr(status, _("Can't unlock cache mutex"));
> > +#endif
> > +
> > + return err;
> > +}
> > +
> > svn_error_t *
> > svn_cache_get(void **value_p,
> > svn_boolean_t *found,
> > @@ -195,25 +251,26 @@ svn_cache_get(void **value_p,
> > const void *key,
> > apr_pool_t *pool)
> > {
> > - /* ### TODO: mutex */
> > -
> > - void *entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > + void *entry_void;
> > struct cache_entry *entry;
> > + svn_error_t *err;
> > +
> > + SVN_ERR(lock_cache(cache));
> >
> > + entry_void = apr_hash_get(cache->hash, key, cache->klen);
> > if (! entry_void)
> > {
> > *found = FALSE;
> > - return SVN_NO_ERROR;
> > + return unlock_cache(cache, SVN_NO_ERROR);
> > }
> >
> > entry = entry_void;
> >
> > move_page_to_front(cache, entry->page);
> >
> > - SVN_ERR(duplicate_value(value_p, cache, entry->value, pool));
> > *found = TRUE;
> > -
> > - return SVN_NO_ERROR;
> > + err = duplicate_value(value_p, cache, entry->value, pool);
> > + return unlock_cache(cache, err);
> > }
> >
> > /* Removes PAGE from the LRU list, removes all of its entries from
> > @@ -252,7 +309,12 @@ svn_cache_set(svn_cache_t *cache,
> > void *value,
> > apr_pool_t *pool)
> > {
> > - void *existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> > + void *existing_entry;
> > + svn_error_t *err = SVN_NO_ERROR;
> > +
> > + SVN_ERR(lock_cache(cache));
> > +
> > + existing_entry = apr_hash_get(cache->hash, key, cache->klen);
> >
> > /* Is it already here, but we can do the one-item-per-page
> > * optimization? */
> > @@ -281,8 +343,8 @@ svn_cache_set(svn_cache_t *cache,
> > struct cache_page *page = entry->page;
> >
> > move_page_to_front(cache, page);
> > - SVN_ERR(duplicate_value(&(entry->value), cache, value, page->page_pool));
> > - return SVN_NO_ERROR;
> > + err = duplicate_value(&(entry->value), cache, value, page->page_pool);
> > + goto cleanup;
> > }
> >
> > /* Do we not have a partial page to put it on, but we are allowed to
> > @@ -318,8 +380,10 @@ svn_cache_set(svn_cache_t *cache,
> >
> > /* Copy the key and value into the page's pool. */
> > new_entry->key = duplicate_key(cache, key, page->page_pool);
> > - SVN_ERR(duplicate_value(&(new_entry->value), cache, value,
> > - page->page_pool));
> > + err = duplicate_value(&(new_entry->value), cache, value,
> > + page->page_pool);
> > + if (err)
> > + goto cleanup;
> >
> > /* Add the entry to the page's list. */
> > new_entry->page = page;
> > @@ -341,6 +405,6 @@ svn_cache_set(svn_cache_t *cache,
> > }
> > }
> >
> > - /* ### TODO: mutex */
> > - return SVN_NO_ERROR;
> > + cleanup:
> > + return unlock_cache(cache, err);
> > }
>
--
David Glasser | glasser@davidglasser.net | http://www.davidglasser.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-03-12 02:21:48 CET