Created attachment 66415 [details] [review]
AutoLocker patch as described above

re #1, the "using namespace Birnet;" has to be done for any birnet code use, and needs to be present regardless of the autolocker implementation. 
so this can't seriously be considered when doing a complexity comparison.

wrg to:
  Mutex mutex;
  AutoLocker<Mutex> locker (mutex);
that is not quite what i suggested.
instead, you should be doing:
  AutoLocker<Mutex> locker;
i.e. the template argument should be default constructed and be part of the auto locker class implementation. in contrast to the Qt pendant, this also allowes recursive mutextes or other objects that support lock() and unlock().

But

  AutoLocker<Mutex> locker;

in itself is useless. The important part here is that you can use the AutoLocker to lock a mutex you have previously constructed. I.e.:

Mutex foo_mutex;
Foo   foo;

void EngineThread::some_function ()
{
  AutoLocker<Mutex> locker (foo_mutex);
  if (foo.bar > 3)
    return;

  do_something_else();
}

void BseThread::another_function ()
{
  AutoLocker<Mutex> locker (foo_mutex);
  foo.bar = 4;
}

That way, the Foo structure is protected by the foo_mutex; the locker objects in each function ensure that the foo_mutex is locked as long as the function runs, and are a shorthand for writing:

Mutex foo_mutex;
Foo   foo;

void EngineThread::some_function ()
{
  foo_mutex.lock();
  if (foo.bar > 3)
    {
      foo_mutex.unlock();
      return;
    }
  do_something_else();
  foo_mutex.unlock();
}

void BseThread::another_function ()
{
  foo_mutex.lock();
  foo.bar = 4;
  foo_mutex.unlock();
}

So they basically ensure that you can't forget the unlock(). Note that if do_something_else() throws an exception, the code of some_function without the locker is even incorrect because you need a try block around do_something_else() to ensure that it gets unlocked in any case.

Stroustrup discusses the basic idea of acquiring any kind of resources by helper objects in his C++ book (I can't quote it exactly because I don't have the book - but its called  "resource acquisition is initialization"). Its also mentioned here:

http://www.research.att.com/~bs/bs_faq2.html#finally


If now - as you suggested - each AutoLocker would construct its own Mutex object, the foo datastructure would no longer be protected (that is, both would access bar at the same time).

Also note that in Qt4 recursive and normal mutexes are also supported by the QMutexLocker. The reason is that the QMutex class handles both, recursive and non-recursive mutexes.

Here is another useful link, to the Qt4.1 QMutexLocker documentation:

http://doc.trolltech.com/4.1/qmutexlocker.html

I looked at the magic code you sent me by email (which gets rid of the extra template argument). However, it does so at the cost of introducing an extra new/delete. The compiler can optimize away lots of stuff, but I don't believe it can optimize away a new/delete call.

So basically it could be - depending on the new/delete implementation, that you have introduced an extra lock/unlock (for the allocator) for each lock/unlock done via the AutoLocker. I don't think thats too good, and I don't want to find myself arguing "can I use the auto locker here? or will that introduce priority inversion with even the lowest priority thread, because of the delete/new in there?"...

And even if it is not priority inversion that is to be feared: if its slower, I'll find myself not using AutoLocker in performance critical code. Thats what I want to avoid.

So I am suggesting a simpler method here, which of course restricts the possibilities for using it: it explicitely only works for Mutex and RecMutex. However, it does so without using new/delete, and without the extra template argument.

Here are benchmarks to back my performance concerns:

[BB] stefan@lotrien:~/prog/autolocker$ time autolocker lock_unlock

real    0m1.879s
user    0m1.855s
sys     0m0.015s
[BB] stefan@lotrien:~/prog/autolocker$ time autolocker simple_locker

real    0m1.756s
user    0m1.728s
sys     0m0.016s
[BB] stefan@lotrien:~/prog/autolocker$ time autolocker magic_locker

real    0m4.357s
user    0m4.303s
sys     0m0.036s

The variants:
lock_unlock) doesn't use auto locking at all, so its the performance we want to reach

simple_locker) is my implementation - only works for mutexes and rec_mutexes; however it seems that the compiler can optimize away all the bits and pieces so that only the lock() and unlock() calls remain

magic_locker) is your implementation - generic but it shows that it is a lot slower


[ benchmarks done on AMD64 with g++-4.1 and -O2 ]

Created attachment 66443 [details]
Benchmark code for locker variants

The C++ code I used to create the benchmarks in comment #5. It contains two approaches for getting rid of the extra (redundant) template argument.

are you going to supply a new patch that suports both mutexes?

Created attachment 66469 [details] [review]
AutoLocker patch simplified (without template)

Here is a simplified version of the patch that supports recursive and non-recursive mutexes and gets rid of the extra template argument.

your comment: /** * Locks a mutex on construction, ...
doesn't form a valid sentence, you're lacking the subject.
you will have to add "This class"/"The AutoLocker"/etc.
and:
  explicit	AutoLocker    (Mutex& m)
		  : recursive (false)		    { mtx.mutex = &m; relock(); }
is invalid indentation (and hardly to be supported by any automated indentation tool).
if you really had to single-line the code here, it could be:
  explicit   AutoLocker    (Mutex& m)                                                                                                                         
    : recursive (false)                                                                                                                                       
  { mtx.mutex = &m; relock(); }                                                                                                                               

other than that, the patch looks good.
provided the above issues are fixed and an AutoLocker test is added to the threading test in birnet, the patch can be applied.

(In reply to comment #9)
> your comment: /** * Locks a mutex on construction, ...
> doesn't form a valid sentence, you're lacking the subject.

Fixed.

> you will have to add "This class"/"The AutoLocker"/etc.
> and:
>   explicit      AutoLocker    (Mutex& m)
>                   : recursive (false)               { mtx.mutex = &m; relock();
> }
> is invalid indentation (and hardly to be supported by any automated indentation tool).
> if you really had to single-line the code here, it could be:
>   explicit   AutoLocker    (Mutex& m)                                           
>     : recursive (false)                                                         
>   { mtx.mutex = &m; relock(); }                                                 

I didn't squeeze it in one line now. I am not a big fan of that anyway, but its done for all other methods in the file, so I supposed better be compatible with that.

> other than that, the patch looks good.
> provided the above issues are fixed and an AutoLocker test is added to the
> threading test in birnet, the patch can be applied.

Actually for non-recursive mutexes I've got a pretty good idea how to test them. For recursive mutexes I don't - I committed the code with some kind of test now, but I don't know of an elegant TASSERT () that could reasonably be used to validate that recursive mutexes are locked and unlocked correctly. If we want to test it to that degree, that is.

fixed up formatting in CVS and extended the recursive mutext test a bit now.

i've come across the need of an auto-locker for other objects than Mutex and RecMutex. so i've reimplemented my original idea, just without the new/delete now.
the result is in birnet now, along with proper benchmarking code.
the results are pretty encouraging:
  Birnet-AutoLocker: 0.000328 msecs
  Direct-AutoLocker: 0.000537 msecs
  Heap-AutoLocker:   0.000994 msecs
here, "Birnet-AutoLocker" is the newest generic version, "Direct-AutoLocker" is the former implementation that directly hard-codes the Mutex and RecMutex types, and "Heap-AutoLocker" is my original implementation, mentioned in comment #5.

the timings in comment #12 were buggy.
here're fixed timings, also the auto-locker implementation was refined once more (from Pointer-AutoLocker to Birnet-AutoLocker). also included are timings for manual locking without usage of an auto-locker:

Manual-Locker:       82.390 nsecs
Direct-AutoLocker:   82.750 nsecs
Birnet-AutoLocker:   97.184 nsecs
Pointer-AutoLocker:  98.276 nsecs
Heap-AutoLocker:    341.574 nsecs