Stefan Fuhrmann wrote on Sun, Jun 26, 2011 at 23:00:24 +0200: > On 11.06.2011 14:53, Daniel Shahaf wrote: > >Stefan Fuhrmann wrote on Sat, Jun 11, 2011 at 10:13:27 +0200: > >>On 10.06.2011 22:28, Daniel Shahaf wrote: > >>>stef...@apache.org wrote on Wed, May 25, 2011 at 22:20:25 -0000: > >>>>Author: stefan2 > >>>>Date: Wed May 25 22:20:25 2011 > >>>>New Revision: 1127709 > >>>> > >>>>URL: http://svn.apache.org/viewvc?rev=1127709&view=rev > >>>>Log: > >>>>Fix a pool usage issue: svn_cache__get_partial() may be called many > >>>>times in a row. Thus, the internal pool used to construct keys should > >>>>be cleared in this function as well from time to time. > >>>> > >>>>* subversion/libsvn_subr/cache-membuffer.c > >>>> (svn_membuffer_cache_get_partial): regularly clear the internal > >>>> scratch pool > >>>> > >>>>Modified: > >>>> subversion/trunk/subversion/libsvn_subr/cache-membuffer.c > >>>> > >>>>Modified: subversion/trunk/subversion/libsvn_subr/cache-membuffer.c > >>>>URL: > >>>>http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c?rev=1127709&r1=1127708&r2=1127709&view=diff > >>>>============================================================================== > >>>>--- subversion/trunk/subversion/libsvn_subr/cache-membuffer.c (original) > >>>>+++ subversion/trunk/subversion/libsvn_subr/cache-membuffer.c Wed May 25 > >>>>22:20:25 2011 > >>>>@@ -1668,6 +1668,12 @@ svn_membuffer_cache_get_partial(void **v > >>>> > >>>> DEBUG_CACHE_MEMBUFFER_INIT_TAG > >>>> > >>>>+ if (++cache->alloc_counter> ALLOCATIONS_PER_POOL_CLEAR) > >>>>+ { > >>>>+ apr_pool_clear(cache->pool); > >>>>+ cache->alloc_counter = 0; > >>>>+ } > >>>>+ > >>>Does this need to be guarded by a cache lock? > >>> > >>No. This happens in the outer / front-end code > >>that merely adds a key prefix (combine_key below) > >>before calling the shared cache object. > >> > >>All front-end operations assume single-threaded > >>access, which should be o.k. for fs_t-local objects. > >Okay, if that function is guaranteed (perhaps by API contract) not to > >run concurrently to any other `front end' function (and in particular to > >itself), then my concerns are resolved. > > > >(I was worried about accessing cache->pool and cache->alloc_counter from > >multiple threads concurrently --- writer-writer or writer-reader --- > >which might to undefined behaviour.) > > > >What guarantees the single-threaded access? I don't see it documented > >in svn_cache.h (on the contrary, that one has explicit 'thread_safe' > >parameters) and the code doesn't take a lock at at that point either. > > > I implemented optional thread-safety some days ago but it > turned out to require / suggest a new API for svn_mutex_t > structures (handles NULL, APR w/o threading etc.) > Otherwise, there would be even more duplicated mutex > handling code. > > However, to keep API churn low for 1.7 stabilization, I like > to suggest the following: simply adding a thread_safe > parameter to the membuffer cache constructor API and > returning "not supported / implemented" if set to TRUE. >
In other words, you're suggesting to have *all* membuffer caches declare themselves as non-thread-safe in 1.7? Assuming the cache users are fine with that, it solves the issue I raised, and all these are private API's, so +1. Also: creating the membuffer singleton respects the public API's config singleton's SINGLE_THREADED parameter... so we'd have to somehow "ignore" that parameter (and rely on FSFS-level mutexes?). Hmm. > -- Stefan^2.
