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_at_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.
-- Stefan^2.
Received on 2011-06-26 23:00:59 CEST