Stefan Fuhrmann wrote on Tue, Jul 05, 2011 at 00:02:27 +0200: > On 27.06.2011 13:54, Daniel Shahaf wrote: > >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? > Since this is a completely private API, I decided to not > change the definition for 1.7. Currently, all these "frontend" > objects (forwarding to the already thread-safe singleton) > are used from a single thread. > > We just apply the same guarantees that we already gave > to the inprocess_cache in 1.6. The latter provides optional > synchronization but that is never activated.
Are you saying "the code is safe because svnserve and mod_dav won't access a given svn_fs_t object (and, by extension, the caches therein) from more than one thread"? If yes, that's IMO irrelevant since the svn_fs API may allow that even if our code doesn't use that. If not, perhaps it's best that I catch you on IRC to hash this point out. Daniel (who recalls seeing other new code in FSFS that also optimizes for the "one thread" case) > >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. > No, the singleton already uses a thread-safe API. > This is not going to change soon. > > -- Stefan^2.
