On Mon, Dec 22, 2014 at 2:09 PM, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> On 19 December 2014 at 19:26, Ivan Zhakov <ivan_at_visualsvn.com> wrote:
> >
> > On 19 December 2014 at 18:55, Branko Čibej <brane_at_wandisco.com> wrote:
> > > On 19.12.2014 15:37, Ivan Zhakov wrote:
> > >> Originally FSFS caching implementation was using apr_thread_mutex_t to
> > >> serialize access to shared data. In r1346122 [1] implementation was
> > >> switched to apr_thread_rw_lock_t to improve performance. This change
> > >> was released in Subversion 1.8.0
> > >>
> > >> Unfortunately current apr_thread_rwlock_t implementation is very slow
> > >> on Windows. As quick workaround FSFS code was patched to select
> > >> apr_thread_mutex_t or apr_thread_rwlock_t() depending of platform on
> > >> compile time (r1611380 [2]).
> > >>
> > >> In further investigation Bert found why apr_thread_rwlock_t() so slow
> > >> on Windows: implementation uses kernel level mutex object instead of
> > >> lightweight critical section (critical sections are also used in
> > >> apr_thread_mutex_t in most cases).
> > >>
> > >> The simple patch to switch apr_thread_rwlock_t() implementation to use
> > >> critical sections (through apr_thread_mutex_t) was proposed on APR
> > >> development mailing list [3], but patch was not commited nor reviewed.
> > >>
> > >> So the current situation is:
> > >> 1. APR has performance problem on Windows that hurts Subversion 1.8.x
> and trunk
> > >> 2. Subversion trunk has workaround for this specific problem at
> compile time
> > >> 3. Patch proposed to APR mailing list, without any reaction for three
> months
> > >>
> > >> From my point of view the proposed patch is straightforward and Bert
> > >> stated that it makes apr_thread_rwlock_t 10 to 140 times more
> > >> efficient on Windows.
>
One way to reduce the impact is by increasing
the cache size. Large(r) caches get segmented
with each segment having its private sync object.
IOW, less contention and closer to the 10x case
rather than the 140x.
> So the best option will be to commit Bert's
> > >> patch, but I cannot do this since I'm not APR committer :(
> > >>
> > >> Thoughts?
> > >
> > >
> > >
> > > Thanks for the reminder ... I remember that thread on apr-dev, but it
> > > slipped my mind. If I don't review and commit Bert's patch in the next
> > > week or so, feel free to send another nagging e-mail. :)
> > >
> > Will do!
> >
> > > FWIW, committing to APR trunk isn't enough: should be back-ported to
> 1.6
> > > and 1.5, too.
> > >
> > Yes, you are right. We need this to be backported to 1.6 and 1.5 APR
> branches.
> >
>
> Btw it's not the only problem with APR read/write lock on Windows and
> OS/2: rwlock sometimes allows a writer to take the lock while a reader
> has the lock. The detailed explanation of possible race-condition
> given in issue 45455 [1].
>
> Subversion 1.8.x is affected by this issue and FSFS membuffer cache
> may be corrupted. I'm thinking about backporting r1611380 to 1.8.x or
> switch back to simple mutex for FSFS caches for all platforms.
We should backport r1611380. This probably needs
a backport branch.
> Any thoughts will be very appreciated.
> [1] https://issues.apache.org/bugzilla/show_bug.cgi?id=45455
>
> We could change the compile time condition to
APR_HAVE_PTHREAD_H since for Unix, APR
directly maps to the respective pthread primitives
and is almost guaranteed to behave correctly.
If we do that change, it should be part of the backport.
-- Stefan^2.
Received on 2014-12-22 21:16:35 CET