Re: svn commit: r1142130 - in /subversion/branches/svn_mutex/subversion/libsvn_fs_fs: fs.c fs.h fs_fs.c

[Blair Zajac <bl...@orcaware.com>]

On Jul 1, 2011, at 5:27 PM, stefan2@apache.org wrote:

> Author: stefan2
> Date: Sat Jul  2 00:27:28 2011
> New Revision: 1142130
>
> URL: http://svn.apache.org/viewvc?rev=1142130&view=rev
> Log:
> Replace usage of plain APR thread mutex API with the new svn_mutex
> API in FSFS.

> -#if APR_HAS_THREADS
> -      /* We also need a mutex for synchronising access to the active
> -         transaction list and free transaction pointer. */
> -      status = apr_thread_mutex_create(&ffsd->txn_list_lock,
> -                                       APR_THREAD_MUTEX_DEFAULT,  
> common_pool);
> -      if (status)
> -        return svn_error_wrap_apr(status,
> -                                  _("Can't create FSFS txn list  
> mutex"));
> -#endif
> -
> +      SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, TRUE,  
> common_pool));

I don't get why you have enable_mutex.  On a platform without thread  
support, svn_mutex__init with TRUE will always return APR_ENOTIMPL.   
The proper fix is to wrap APR_HAS_THREAD around it:

#if APR_HAS_THREADS
SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, TRUE, common_pool));
#endif

but that seems like unnecessary work.  I would rather see this

SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, common_pool));

and the mutex will behave with or without locking automatically.

Finally, looking at svn_mutex__init's implementation, I would state  
that the wrapped pointer will always either be NULL or a valid  
pointer, since you always set it to 0.

Blair

Re: svn commit: r1142130 - in /subversion/branches/svn_mutex/subversion/libsvn_fs_fs: fs.c fs.h fs_fs.c

[Stefan Fuhrmann <eq...@web.de>]

On 02.07.2011 04:35, Blair Zajac wrote:
>
> On Jul 1, 2011, at 5:27 PM, stefan2@apache.org wrote:
>
>> Author: stefan2
>> Date: Sat Jul  2 00:27:28 2011
>> New Revision: 1142130
>>
>> URL: http://svn.apache.org/viewvc?rev=1142130&view=rev
>> Log:
>> Replace usage of plain APR thread mutex API with the new svn_mutex
>> API in FSFS.
>
>> -#if APR_HAS_THREADS
>> -      /* We also need a mutex for synchronising access to the active
>> -         transaction list and free transaction pointer. */
>> -      status = apr_thread_mutex_create(&ffsd->txn_list_lock,
>> -                                       APR_THREAD_MUTEX_DEFAULT, 
>> common_pool);
>> -      if (status)
>> -        return svn_error_wrap_apr(status,
>> -                                  _("Can't create FSFS txn list 
>> mutex"));
>> -#endif
>> -
>> +      SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, TRUE, 
>> common_pool));
>
> I don't get why you have enable_mutex.  On a platform without thread 
> support, svn_mutex__init with TRUE will always return APR_ENOTIMPL.  
> The proper fix is to wrap APR_HAS_THREAD around it:
>
> #if APR_HAS_THREADS
> SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, TRUE, common_pool));
> #endif
>
> but that seems like unnecessary work.  I would rather see this
>
> SVN_ERR(svn_mutex__init(&ffsd->txn_list_lock, common_pool));
>
> and the mutex will behave with or without locking automatically.
>
> Finally, looking at svn_mutex__init's implementation, I would state 
> that the wrapped pointer will always either be NULL or a valid 
> pointer, since you always set it to 0.

No, the reason behind the flag is its usage in APIs like
svn_cache__create_inprocess() where the caller can
decide whether a synchronization is necessary for a
given instance. If it won't be used from multiple threads,
the implementation will simply avoid the mutex overhead.

-- Stefan^2.