[boost] Re: Lock unification [move]

From: Howard Hinnant (hinnant_at_[hidden])
Date: 2004-07-21 17:05:15

Thanks everyone for your comments and patience.  There's a new spec up 
at:

http://home.twcny.rr.com/hinnant/cpp_extensions/threads.html

I've tried to incorporate those comments I've agreed with, and explain 
my disagreements below.  Further discussion would be much appreciated.  
I feel there has been dramatic improvements in this first iteration of 
the spec.  Thanks again!

On Jul 18, 2004, at 6:41 PM, Batov, Vladimir wrote:

> 1.    explicit scoped_lock(mutex_type& m);
> 2.    scoped_lock(mutex_type& m, locking_enum lock_it);
> 3.    scoped_lock(mutex_type& m, blocking_enum block_it);
> 4.    scoped_lock(mutex_type& m, const elapsed_time& elps_time);
>
> Do we need (3)? Isn't is covered by (4) with elps_time = INFINITE
> (blocking) and - 0 (non blocking)? If it is intended as a "convenience"
> interface, my expectation would be that average users would use thin 
> and
> more explicit wrappers around the general lock. Like try_lock and
> timed_lock. Then, those thin wrappers would address the "convenience"
> issue better.

I agree that a timed lock collapses to non-locking and non-blocking on 
the extremes, but I don't think it is a good idea to collapse the 
implementations or the signatures.  Separate constructors allows a 
clean separation of ideas, and also allows for the fact that the 
underlying mutex may well support try_lock (for example), but not 
timed_lock.

However, I've modified the signatures of the enum-based constructors 
(read on).

> (1) and (2) overlap as (1) is (2) with lock_it = true. Does not it mean
> that some functionality will have to be duplicated in (1) and (2)? How
> about avoiding the overlap with
>
> 1.    explicit scoped_lock(mutex_type& m);
> 2.    scoped_lock(mutex_type& m, deferred_t);
>
> Then, every constructor would do just one thing.

I agree 100%, though I've spelled deferred_t as defer_lock_type and 
added try_lock_type.  Thanks.  You can now:

scoped_lock lk1(m, defer_lock);  // not locked
scoped_lock lk2(m, try_lock);    // tries to lock

On Jul 19, 2004, at 6:55 AM, Bronek Kozicki wrote:

> * do we need distinction between "write_lock" and "scoped_lock"? After
> all, both lock are:
>   1. exclusive (in regard to ownership of "mutex" object)
>   2. moveable (ie. have move semantics instead of copy semantics)
>
> write_lock delivers some superset of functionality that scoped_lock 
> does
> not have, but do we need separate class for this functionality? After
> all "if the client of scoped_lock<Mutex> does not use funtionality 
> which
> the Mutex does not supply, no harm is done". I understand that under
> current proposal mutex class may deliver three separate families of 
> lock
> functions : lock, read_lock and write_lock, but I want you to consider
> just two: lock (== exclusive == write) and shared_lock (== read). I
> suggest that scoped_lock (another possible name "exclusive_lock") would
> be move-constructible also from upgradable_shared_lock. If Mutex class
> does not deliver member function
> unlock_upgradable_shared_lock_exclusive , we would have compilation
> error. Otherwise, no harm is done. The same applies to move-assignment
> from upgradable_shared_lock (hm, some shorter name please?)

Good comments.  Here's what I've done in response:

scoped_lock  |----------------> scoped_lock
write_lock   |
read_lock  -------------------> sharable_lock
upgradable_read_lock ---------> upgradable_lock

> * I do not quite like
> "write_lock(try_rvalue_ref<upgradable_read_lock<mutex_type> > r);"
> (neither
> "exclusive_lock(try_rvalue_ref<upgradable_shared_lock<mutex_type> >
> r)" ). It seems inconsistent with rest of interface. What about
> "write_lock (rvalue_ref<upgradable_read_lock<mutex_type> > r,
> blocking_enum block_it);" ?

My problem is that this syntax does not generalize to the 
try-move-from-upgradable_lock-assignment:

upgradable_lock ul(m);

scoped_lock sl  (try_move(ul));
             sl = try_move(ul);

I really want the try-to-upgrade syntax to be consistent between copy 
ctor and assignment forms.  The consistency makes the interface easier 
to learn.

> * mutex() memeber function may return void* instead of "const
> mutex_type*". This will make it suitable just to check for equality of
> owned mutex, while preventing any kind of mutex manipulation. Just an
> idea, because "const" (which is already there) in most cases should
> suffice.

I went with Peter's non-const pointer.

> * I'd still like to have number of free functions:
>
> template <typename Mutex>
> scoped_lock<Mutex> lock_mutex(Mutex); // or "acquire"
>
> template <typename Mutex>
> scoped_lock<Mutex> try_lock_mutex(Mutex); // nonblocking; or
> "try_acquire"
>
> template <typename Mutex>
> scoped_lock<Mutex> try_lock_mutex(Mutex, const elapsed_time&); // or
> "try_acquire"
>
> These functions might look little different (eg. use enumerations
> locking_enum  and blocking_enum). The idea is that function will create
> lock appropriate for given Mutex type - assuming that there migh be
> Mutex-es delivered with their own lock class, or that.

I'm not completely against the free functions.  But I do feel that they 
are syntax sugar for the constructors that must be there anyway 
(discussed more below).  And if you have them for one lock, you need 
them for all 3 locks for consistency.

> * I'd like to have common base class over all lock classes. I do not
> want polymorphic behaviour (all member functions might be protected,
> including destructor, ctor and cctor), but to allow following very
> simple and I believe typical usage scenarios:

The common base class stuff makes me nervous.  I don't want polymorphic 
behavior either.  But if it isn't polymorphic behavior what would 
~lock() do?  Choices are m_.unlock(), m_.unlock_sharable() and 
m_.unlock_upgradable().

On Jul 19, 2004, at 8:47 AM, Peter Dimov wrote:

> I mostly agree with Bronek Kozicki. Given a movable lock, Eric 
> Niebler's
> proposal:
>
>     scoped_lock try_lock( Mutex & m );
>     scoped_lock timed_lock( Mutex & m );
>
> is a better try/timed interface. Heisenberg constructors must die.

Sorry, I don't know what a Heisenberg constructor is.

I don't dislike these helper functions.  But I do dislike the idea that 
they should be the only way to construct-and-try or 
construct-with-time.  The problem is that we have 3 different types of 
locks, and I want to be able to construct-and-try in a generic-lock 
function:

template <class Lock>
void
foo()
{
     Lock lk(m, try_lock);
     ...
}

The above code should work for scoped_lock, sharable_lock and 
upgradable_lock.  I don't see how to make that work with the helper 
function syntax, at least not without getting a lot uglier (like using 
explicit template arguments).

> Also, I still think that the bool parameter is better than a "locking" 
> enum,
> as evidenced by the use case shown in the specification:
>
>     scoped_lock lock( m, want_to_lock? locking: non_locking );
>
> I find this an unnecessarily verbose and contrived way to express
>
>     scoped_lock lock( m, want_to_lock );
>
> which is the original intent.

So how do you feel about:

scoped_lock lk1(m, defer_lock);  // not locked
scoped_lock lk2(m, try_lock);    // tries to lock

As others have shown, you can still decide at runtime with something 
like:

scoped_lock lk(want_to_lock ? scoped_lock(m) : scoped_lock(defer_lock));

or whatever.  As Vladimir pointed out, these new constructors are very 
cheap and fast since they don't have to test anything to find out what 
they should do.  The defer_lock constructor is especially fast.  It 
just sets the internal mutex reference to m, and the locked_ to false 
(two, maybe 3 assembly instructions).

> I disagree that
>
>     void * mutex() const;
>
> is a better mutex() accessor. On the contrary, I'd argue that mutex() 
> should
> return a non-const mutex_type. A const return doesn't make a lot of 
> sense
> given that our mutexes have no const member functions; the private 
> interface
> is a better protection than the const. On the other hand, a non-const 
> return
> allows the client to use scoped_lock<> on user-defined mutex types (in
> implementing the user-defined condition::wait, for example).

I've gone with:

mutex_type* mutex() const;

I don't have very strong feelings on this one, except that the current 
boost design does not hide the mutex interface as we thought it should. 
  You just get to the mutex interface via the lock instead of directly.  
So I'm not that anxious to make it difficult to access the mutex and 
operate on it directly.  Sometimes there's good reason to do that.

> I also like the proposed scoped_lock == write_lock unification.

<nod>

On Jul 19, 2004, at 2:05 PM, Eric Niebler wrote:

> Movable locks open up all sorts of interesting interface possibilities.

<nods so agreeably that head almost falls off!>  :-)

On Jul 19, 2004, at 6:28 PM, David Abrahams wrote:

> "non_locking" is a terribly obscure name to associate with a lock.  I
> really think "deferred" works well.

<nod>  How's:

scoped_lock lk1(m, defer_lock);  // not locked

-Howard