Boost logo

Threads-Devel :

Subject: Re: [Threads-devel] RFC: 1st pthread shared_mutex refactoring patch
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2013-03-17 08:22:17


Le 17/03/13 11:32, Fredrik Orderud a écrit :
> On Fri, Feb 15, 2013 at 11:51 PM, Vicente J. Botet Escriba
> <vicente.botet_at_[hidden] <mailto:vicente.botet_at_[hidden]>> wrote:
>
> Le 15/02/13 15:34, Fredrik Orderud a écrit :
>> On Tue, Feb 12, 2013 at 7:08 PM, Vicente J. Botet Escriba
>> <vicente.botet_at_[hidden] <mailto:vicente.botet_at_[hidden]>> wrote:
>>
>> I will start to analyze your patch this week.
>>
>> I don't know if you have followed this ticket.
>> #7906 <https://svn.boost.org/trac/boost/ticket/7906> Very
>> bad performance of generic implementation of shared_mutex on
>> windows <https://svn.boost.org/trac/boost/ticket/7906>
>>
>>
>> From the performances presented it seems that the windows
>> specific implementation will be needed, so maybe a windows
>> specific implementation for shared priority mutex should also
>> be needed. The problem is that the windows implementation
>> contains some bugs and missing features.
>>
>>
>> Thank you for the update Vicente. I was not aware of the poor
>> win32 performance for generic shared_mutex, so that of course
>> needs to be taken into consideration. I believe it should be
>> feasible to refactor the pthread & win32 shared_mutex
>> implementations to share state_data (locking policy logic) even
>> though the locking "backends" are different.
>>
> yes, but I don't know if a generic backend for priority shared
> mutex will be enough for windows, so if you have some ideas on how
> to implements them on windows it is worth exploring them. Anyway,
> it will be better to have a generic implementation of priority
> shared mutex than none.
>
> I would like to introduce your patch soon, but lastly there has
> been a lot of troubles and I have not be tempted to add more
> disturbance.
>
> I'll let you know when I have some news.
>
>
> Vicente,
> have you had time to examine my patch yet?
>
> Please let me know if there is something I can do to expedite the
> process. If preferable, I can e.g. break it up into a series of
> smaller patches. If accepted, I want to do the same refactoring also
> for the win32 shared_mutex, and finally unify the state_data
> inner-class between the pthread and win32 implementations, so that the
> implementations behave consistently.

I have started today.

I'm not confident with this refactoring

          void unlock_shared()
          {
              boost::unique_lock<boost::mutex> lk(state_change);
- bool const last_reader=!--state.shared_count;

- if(last_reader)
- {
- if(state.upgrade)
- {
- state.upgrade=false;
- state.exclusive=true;
- upgrade_cond.notify_one();
- }
- else
- {
- state.exclusive_waiting_blocked=false;
- }
+ bool const last_reader = !state.unlock_shared();
+
+ if (last_reader) {
+ upgrade_cond.notify_one();

where

             unsigned unlock_shared () {
                 bool const last_reader = !--shared_count;

                 if (last_reader) {
                     if (upgrade) {
                         upgrade=false;
                         exclusive=true;
                     } else {
                         exclusive_waiting_blocked=false;
                     }
                 }

                 return shared_count;
             }

In the current code
                 upgrade_cond.notify_one();

is done if !--state.shared_count and state.upgrade

After the pacth
                 upgrade_cond.notify_one();

is done if !--state.shared_count independently of the state of
state.upgrade.

I guess that unlock_shared () should return whether the mutex is downgraded

             bool unlock_shared () {
                 bool const last_reader = !--shared_count;

                 if (last_reader) {
                     if (upgrade) {
                         upgrade=false;
                         exclusive=true;
                         return true;
                     } else {
                         exclusive_waiting_blocked=false;
                     }
                 }
                 return false;
             }

and the code is replaced by

+ bool const downgraded = state.unlock_shared();
+
+ if (downgraded) {
+ upgrade_cond.notify_one();

What do you think?

I'll post more a I try to merge your patch.

Best,
Vicente



Threads-Devel list run by bdawes at acm.org, david.abrahams at rcn.com, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk