You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Stefan Fuhrmann <eq...@web.de> on 2011/07/05 00:02:27 UTC
Re: svn commit: r1127709 - /subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
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:
>>>>> stefan2@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.
> 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.
Re: svn commit: r1127709 -
/subversion/trunk/subversion/libsvn_subr/cache-membuffer.c
Posted by Daniel Shahaf <da...@apache.org>.
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:
> >>>>>stefan2@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.