Re: [boost] Request for interest in the new Synchro library

Subject: Re: [boost] Request for interest in the new Synchro library
From: vicente.botet (vicente.botet_at_[hidden])
Date: 2009-03-04 07:18:19

----- Original Message ----- 
From: "Dmitry Goncharov" <dgoncharov_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Wednesday, March 04, 2009 10:06 AM
Subject: Re: [boost] Request for interest in the new Synchro library

> 
> 
> 
> vicente.botet wrote:
>>> What about the following adapter?
>>>
>>>
>>> #ifndef BOOST_BINSEM_HPP
>>> #define BOOST_BINSEM_HPP
>>>
>>> #include <boost/thread.hpp>
>>>
>>> namespace boost
>>> {
>>> template <typename Semaphore>
>>> class binary_semaphore
>>> {
>>> public:
>>>    explicit binary_semaphore(int v)
>>>        : m_sem(v)
>>>    {}
>>>
>>>    void wait()
>>>    {
>>>        m_sem.wait();
>>>    }
>>>
>>>    void post()
>>>    {
>>>        // If cannot lock then some other thread is posting.
>>>        // In this case just return.
>>>        if (m_mutex.try_lock())
>>>        {
>>>            try
>>>            {
>>>                // The result of try_wait() is not important.
>>>                m_sem.try_wait();
>>>                m_sem.post();
>>>            }
>>>            catch (...)
>>>            {
>>>                m_mutex.unlock();
>>>                throw;
>>>            }
>>>            m_mutex.unlock();
>>>        }
>>>    }
>>>
>>>    bool timed_wait(boost::posix_time::ptime const& t)
>>>    {
>>>        return m_sem.timed_wait(t);
>>>    }
>>>
>>>    bool try_wait()
>>>    {
>>>        return m_sem.try_wait();
>>>    }
>>>
>>> private:
>>>    boost::mutex m_mutex;
>>>    Semaphore m_sem;
>>> };
>>> }
>>>
>>> #endif
>>>
>>> This adapter makes a binary semaphore out of a regular one. The post() 
>>> method is not as efficient as it should be. On the other hand the 
>>> adapter is very simple.
>>> There is also a test attached.
>>>     
>>
>> Thanks for the proposal. I have another alternative using inheritance that avoid adding a new mutex.
>>
>> template <typename ScopeTag=multi_threaded_tag>
>> class basic_binary_semaphore: public basic_semaphore<ScopeTag> {
>> public:
>>     BOOST_COPY_CONSTRUCTOR_DELETE(basic_binary_semaphore) /*< disable copy construction >*/
>>     BOOST_COPY_ASSIGNEMENT_DELETE(basic_binary_semaphore) /*< disable copy asignement >*/
>>
>>     inline explict basic_binary_semaphore(int initialCount):basic_semaphore<ScopeTag>(initialCount>0?1:0) {};
>>
>>     inline void post()
>>     {
>>         scoped_lock lock(this->m_mut);
>>         if(this->m_count == 0){
>>             this->m_cond.notify_one();
>>             ++(this->m_count);
>>         }
>>     }
>> };
>> typedef basic_binary_semaphore<> binary_semaphore;
>>
>> Attached the complete file. What do you think?
>>
>> Thanks,
>> Vicente
>>   
>>   
> Could not find basic_semaphore in boost. Is it one from your Synchro 
> library?

Yes. And I use the fields of the base semaphore. I though that you has taken a look, but ... :(
 
> Locking a mutex inside post() is inefficient. If one thread has the 
> mutex locked then there is no need to wait until the mutex is unlocked. 
> Use try_lock() and return if cannot lock.

I could a a try_post with this semantics. BUt what the user will do if fails?

I'll re-post your implementation here

> namespace boost
> {
> template <typename Semaphore>
> class binary_semaphore
> {
> public:
>    explicit binary_semaphore(int v)
>        : m_sem(v)
>    {}
> 
>    void wait()
>    {
>        m_sem.wait();
>    }
> 
>    void post()
>    {
>        // If cannot lock then some other thread is posting.
>        // In this case just return.
>        if (m_mutex.try_lock())
>        {
>            try
>            {
>                // The result of try_wait() is not important.
>                m_sem.try_wait();
>                m_sem.post();
>            }
>            catch (...)
>            {
>                m_mutex.unlock();
>                throw;
>            }
>            m_mutex.unlock();
>        }
>    }
> 
>    bool timed_wait(boost::posix_time::ptime const& t)
>    {
>        return m_sem.timed_wait(t);
>    }
> 
>    bool try_wait()
>    {
>        return m_sem.try_wait();
>    }
> 
> private:
>    boost::mutex m_mutex;
>    Semaphore m_sem;
> };

You implementation has some deficiencies. It add another mutex to the semaphore. My implementation do not add nothing more. Just controls that second posts are ignored as you have requested. Yours add some try_locks and at the end call
 m_sem.post();

which do the following in my implementation

template <typename ScopeTag>
inline void basic_semaphore<ScopeTag>::post()
{
   scoped_lock lock(m_mut);
   if(m_count == 0){
      m_cond.notify_one();
   }
   ++m_count;
}

So you will lock in any case.

> In one of the previous posts i described why a 
> binary_semaphore::timed_wait() was superior in my case than 
> condition_variable::timed_wait(). Using a condition variable defeats the 
> purpose of a binary_semaphore (at least in my usage scenario).

Well all depends, on how the Semaphore::timed_wait is implemented.

>    bool timed_wait(boost::posix_time::ptime const& t)
>    {
>        return m_sem.timed_wait(t);
>    }
> 

In my case it inherits from the basic_semaphore<>::timed_wait.

So in both cases 

yours::binary_semaphore<mine::basic_semaphore<multi_threaded_tag> > will even lock more than

mine::basic_binary_semaphore<multi_threaded_tag> >

Maybe your binary_semaphore class works better with Semaphore classes that do not use conditions. But do you have such a portable Semaphore class? If you have I'm interested also.

Best,
Vicente

Vicente