Re: [Threads-devel] RFC: 1st pthread shared_mutex refactoring patch

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