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_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.
>>
> 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.
Received on 2011-07-05 00:03:05 CEST