Comments on the thread library

From: Jens Maurer (Jens.Maurer_at_[hidden])
Date: 2001-03-25 04:01:31

I've had a look at the boost thread library.
All in all, I believe this is a solidish foundation to build on.

Here are a few detailed remarks:

 - Some parts of the documentation are hard to understand and/or seem
to require additional commas.  Some rephrasing may help.
In particular, the semaphore docs talk about "decrement in a single
atomic operation".  On first sight, decrement is only a single
operation, so atomicity is for free (of course, it isn't necessarily,
because it involves read-modify-write bus cycles).

 - The documentation is missing some statements about assumptions
of instruction reordering by the compiler.  This could mean
to put the thread sync library under the "observable behaviour"
clause from the C++ standard.  We don't want to have instructions
reordered across lock/unlock calls, possibly moving access to shared
data out of the critical section :-)  This needs some formalism
similar to "volatile" handling, also taking into account actual
memory bus transactions in SMP systems.  Basically, we must be
sure that all effects are "done" when the unlock() returns.

 - fast_mutex strikes me as a bad name.  We just killed fast_crc.
What about "unchecked"?

 - The "mutex" introduction should state at the very beginning that
a mutex is always associated with some shared data.   Otherwise the
"shared data" falls out of nowhere.

 - I've had some difficulties understanding the difference between
"undefined" and "unspecified".  This could be clarified.

 - I believe we need the possibility to specify the desired features
of a mutex in a much more fine-grained manner.  For example, on my
platform (Linux 2.4.2, pthreads from glibc 2.2.2) I have the following
set of features available natively:
  - recursive (without timedlock only)
  - checked with and without timedlock
  - unchecked with and without timedlock
(trylock is available in all cases)
I'd like to make Boost.Threads use the native implementation whenever
possible (see also next item)

 - While we're focusing on interfaces, I'd like to point out that
performance seems to be important for threading.  After all, one
class of problems is turned into a threads-based implementation
for the sole purpose of getting speed improvements on SMP
platforms.  I've run a few measurements with the current code
(this is microseconds per lock/unlock pair (no contention) in a
tight loop, see the attached code)

boost::fast_mutex: 0.255257
boost::mutex: 0.669022
pthreads.fast: 0.21111
pthreads.normal: 0.198289
pthreads.recursive: 0.228514
pthreads.errorcheck: 0.21327

Note how boost::mutex is 3x slower than the native implementation,
because it's emulating recursive mutexes with a condition variable.
Also note that the pimpl-wrapping in boost::fast_mutex costs about
10% performance.
Summary: I want to use the native implementation whenever possible.
That means the user must be able to help me and specify what kinds
of locks he really needs on the mutex.  Currently, he cannot do
so.

 - Minor sub-item of the above: timedlock + unchecked is not available
currently. 

 - The above sounds like we need a feature model diagram.

 - The automatic conversion to bool got enough disreputation here
that I don't need to add anything.

 - One other feature the user may want to state is whether he
needs an explicit unlock().  If he doesn't need it, the boolean
variable in the basic_*lock classes can be removed in these cases.
(At least the interface should allow that to be implemented later.)

 - The timedlock interface (milliseconds) should be changed to
the equivalent of POSIX timespec, i.e. seconds and nanoseconds to
be future-safe.

 - The overall introduction in the docs misses a link to condition
variables.

 - The example for the condition variable references a "finished"
variable which appears not to be defined.

 - atomic_t should say that value_type must be at least "int".
Otherwise, it's of not much use in the real world.

 - lock_error should get a string why it was thrown.

 - How do I put a variable number of mutexes in a dynamic data
structure? Say, I'd like to implement the "dining philosophers"
problem with a runtime configurable number of philosophers?

 - Regarding config.hpp:  We've got a new dimension of configuration
information here, namely operating system and hardware information.
Up to now, we only had information regarding the deviation of some
compilers and libraries from the true and only ISO C++.  There
is not such thing as "true and only" for the threading configuration.
Also, putting BOOST_HAS_WINTHREADS under the MSVC section is,
technically speaking, incorrect, because there may be an
implementation of MSVC on non-WinThreads platforms.

I think we should leave out the threads configuration from config.hpp
and put it somewhere else.  Are we going to have more interfaces that
need OS/hardware specific information? I believe, yes.  Besides,
the OS/hardware configuration should be (nearly) entirely orthogonal
to the compiler/library configuration currently in config.hpp

So let's have some os.hpp and hardware.hpp.  Better names, in particular
for "os.hpp", appreciated.  "platform.hpp" sounds too general, though.

Jens Maurer


#include <boost/thread/mutex.hpp>
#include <boost/thread/fast_mutex.hpp>
#include <boost/thread/thread.hpp>
#include <boost/timer.hpp>

#include <pthread.h>

#include <sys/time.h>
#include <unistd.h>

template<class M>
void run(const char * name)
{
  M mtx;

  timeval tv;
  gettimeofday(&tv, 0);
  for(int i = 0; i < 1000000; ++i) {
    typename M::lock lk(mtx);
  }
  timeval tv2;
  gettimeofday(&tv2, 0);
  std::cout << name << ": "
            << ((tv2.tv_sec - tv.tv_sec) + (tv2.tv_usec - tv.tv_usec)/1e6)
            << std::endl;
}

void run2(int kind, const char *name)
{
  pthread_mutex_t pmtx;
  pthread_mutexattr_t mtxattr;
  pthread_mutexattr_init(&mtxattr);
  pthread_mutexattr_settype(&mtxattr, kind);
  pthread_mutex_init(&pmtx, &mtxattr);
  
  timeval tv;
  gettimeofday(&tv, 0);
  for(int i = 0; i < 1000000; ++i) {
    pthread_mutex_lock(&pmtx);
    pthread_mutex_unlock(&pmtx);
  }
  timeval tv2;
  gettimeofday(&tv2, 0);
  std::cout << name << ": "
            << ((tv2.tv_sec - tv.tv_sec) + (tv2.tv_usec - tv.tv_usec)/1e6)
            << std::endl;
  pthread_mutex_destroy(&pmtx);
}

int main()
{
  run<boost::fast_mutex>("boost::fast_mutex");
  run<boost::mutex>("boost::mutex");
  run2(PTHREAD_MUTEX_FAST_NP, "pthreads.fast");
  run2(PTHREAD_MUTEX_NORMAL, "pthreads.normal");
  run2(PTHREAD_MUTEX_RECURSIVE, "pthreads.recursive");
  run2(PTHREAD_MUTEX_ERRORCHECK, "pthreads.errorcheck");
}