Re: [boost] Re: Boost.Threads: Do we need all those mutexes?

From: Peter Dimov (pdimov_at_[hidden])
Date: 2004-07-11 07:46:39

Michael Glassford wrote:
> Peter Dimov wrote:
>> After giving it some thought:
>>
>> - one lock class: tie;
>
> This seems both popular and reasonable. I presume it would be
> templated on the mutex type?

Seems entirely reasonable. Perhaps it's time to move to formal wording.

template<class M> class scoped_lock
{
public:

    explicit scoped_lock( M & m, bool l = true );
    // effects: if( l ) m.lock();
    // post: locked() == l && mutex() == &m;

    ~scoped_lock();
    // effects: if( locked() ) mutex()->unlock();

    void lock();
    // throws: lock_aready_locked if locked();
    // effects: mutex()->lock();
    // post: locked();

    bool try_lock();
    // throws: lock_aready_locked if locked();
    // returns: mutex()->try_lock();
    // post: locked() == (return value);

    bool timed_lock( xtime xt );
    // throws: lock_aready_locked if locked();
    // returns: mutex()->timed_lock( xt );
    // post: locked() == (return value);

    void unlock();
    // throws: lock_not_locked when !locked();
    // effects: mutex()->unlock();
    // post: !locked();

    bool locked() const;

    Mutex * mutex() const;
    // returns: the associated mutex;

    operator unspecified-bool-type() const;
    // returns: locked().
};

I've added the mutex() accessor to support Andrei's lock idiom:

void f()
{
    scoped_lock lock( my_mutex );
    f( lock );
}

void f( scoped_lock & lock )
{
    // check for lock validity
    assert( lock.locked() && lock.mutex() == &my_mutex );

    // proceed with operation
}

Now that we got rid of the excess locks, how about doing the same with the
mutexes?

class mutex // DefaultConstructible
{
private:

    void lock();
    // pre: *this is not locked by the current thread
    // effects: blocks until *this is unlocked
    // post: *this is locked by the current thread

    bool try_lock();
    // returns: true iff *this was unlocked
    // post: if *this was unlocked, *this is now locked by the current
thread

    bool timed_lock( xtime xt );
    // returns: true if *this is now locked by the current thread
    // effects: if *this is unlocked, returns immediately, otherwise blocks
    //             until either *this is unlocked or xt has been reached
    // post: if *this was or became unlocked, *this is now locked by the
    //         current thread

    void unlock();

    // pre: *this is locked by the current thread
    // post: *this is unlocked
};

The main potential issue here is mutex::timed_lock; try_lock doesn't seem
controversial. It is true that Windows 95 does not have
TryEnterCriticalSection, but an implementation can use Alexander Terekhov's
alternative:

http://lists.boost.org/MailArchives/boost/msg64648.php

The mutex::timed_lock situation is a bit more complicated. POSIX labels
pthread_mutex_timedlock as part of the Timeouts option, and I don't know
whether this is because many implementations deliberatley do not provide
timed locks, or because this is a relatively new addition to pthreads. Does
someone know?

My not-so-informed opinion at the moment is that we should provide
timed_lock. I see that the current Boost.Threads implementation uses a mutex
and a condition variable to implement a timed_lock-capable mutex, so we have
a proof of concept that it can always be done, however there may be
efficiency concerns. My line of thought is that since a mutex (usually) has
a fast path user space portion and a slow path kernel space portion, with
the timed_lock baggage not affecting the fast path, it seems reasonable to
always require timed_lock (remember we're in "next standard mode" now,
thinking severals years ahead).

Thoughts?

Some more random remarks regarding the state of Boost.Threads. Several files
still seem to have incorrect line endings. This usually happens when
checking in Windows line endings with a Unix CVS client. Please don't do
that.

I was somewhat surprised by the mutex/cv implementation. I expected a thin
wrapper over pthreads, but this is not the case. At first sight it seems
that the complexity is caused by the fact that boost::condition supports
recursive mutexes that are locked more than once, whereas POSIX does not. I
do not recall any discussions about this issue, and I'm not sure why this
decision was made. It seems wrong (and the implementation seems buggy), but
as usual, I may be missing something.