Re: thread lib review

From: williamkempf_at_[hidden]
Date: 2001-09-07 08:24:56

--- In boost_at_y..., John Maddock <John_Maddock_at_c...> wrote:
> First of all, let me say that I like the thread lib, and I think 
that it
> should be accepted into boost.  I think that the docs are 
excellent; more
> specific comments follow (based on threadcorrect1.zip) :
> 
> Semaphores:
> ~~~~~~~~~
> 
> I think that the semaphore type should be in the lib, However I 
have one
> query about the interface - you have included in the constructor an 
option
> to set the maximum value of the semaphore - as far as I know this 
is a
> Win32 specific feature.  Neither POSIX nor BeOS semaphores support 
this. 
> While it is true that you can emulate this behaviour (probably 
using two
> native semaphores), generally speaking a semaphore should be the 
most
> lightweight synchronisation primitive available, adding this 
feature makes
> it impossible to implement boost::semaphore in terms of a single
> lightweight native primitive.

It's not truly a MS thing.  There's a lot of documentation out there 
that describes both binary semaphores (the classic) and counting 
semaphores (what Boost.Threads has), and a lot of this was published 
before Win32 existed.  There are a lot of benefits to a counting 
semaphore and native implementations should be doable on any platform 
(though you're correct that I've emulated them for POSIX today).

> In Semaphore::up the assert:
> 
>     assert(ret || GetLastError() == ERROR_TOO_MANY_POSTS);
> 
> is failing for the semaphore test in thread_test.cpp, it seems as 
though
> GetLastError is returning ERROR_INVALID_PARAMETER rather than
> ERROR_TOO_MANY_POSTS on my Win98 box.

Hmmm... I get no failures on Win2k.  I'll have to research.
 
> Mutex:
> ~~~~
> 
> Ideally I would like each mutex type to be as efficient as 
possible: on
> win32 that means using critical sections and not mutexes where 
possible
> (mutex and recursive_mutex).  Last I heard, a win32 mutex is about 
4x as
> slow as a critical section, and given that the simpler primitives 
here are
> likely to be used in "contentious" situations, I would like them to 
be as
> efficient as possible.

I've talked about this elsewhere.
 
> Should I be concerned that you cast a HMUTEX to an unsigned long?  

Only mildly.  This is precisely what MS themselves did in the C RTL 
(look at the docu for _beginthreadex) to avoid inclusion of Windows 
specific headers.

>Is this
> portable to all windows versions (32 and 64 bits, WinCE etc?).  I 
think I
> would be inclined to use a void* throughout, and remove the casts - 
that
> way you'll get an error if its wrong, alternatively add a:
> BOOST_STATIC_ASSERT(sizeof(unsigned int) >= sizeof(HANDLE))
> to one of the cpp files.

I'm holding off on such changes to see precisely how MS themselves 
address this in 64 bit.
 
> The Win32 mutex constructors look wrong: you have both an assert 
and a
> (currently redundant) throw.

The assert allows for debugging while the throw insures an error is 
raised in release.  Can't really do with out either one.  I will 
however look to make sure there's not something wrong.
 
> Condition:
> ~~~~~~
> Is it correct that you can use a condition variable with any mutex 
type? 
> I'm concerned that this places an unnecessary restriction on how 
the mutex
> types are implemented, and in particular that it may prevent use of 
the
> most efficient implementation for the simpler mutex types. I'm 
mainly
> concerned about porting to new platforms/api's here.

It is correct, and it's not caused any problems so far.  Can the 
concern be made more real?
 
>  What happens if the mutex is a recursive mutex, and has been locked
> multiple times - does it deadlock?  If so that would be a good 
reason not
> to use recursive mutexes with condition variables?  

No, recursive mutexes have been specifically coded to work 
correctly. :)
 
> If the condition variable supports timed waits, then does the mutex 
also
> have be timed waitable? (I guess not?)

No, it doesn't.
 
> Once.cpp:
> ~~~~~~~
> 
> You seem to be using swprintf without including the necessary header
> (wchar.h), you are also using a non-standard (MS specific) version 
of that
> function, here's what C99 says about it:
> 
> 7.24.2.3 The swprintf function
> Synopsis
> 1 #include <wchar.h>
> int swprintf(wchar_t * restrict s,
> size_t n,
> const wchar_t * restrict format, ...);
> 
> Note the extra size_t parameter - we have a macro for this BTW (or 
will
> have when the new config gets merged in): BOOST_NO_STD_SWPRINTF.

This is used only on MS platforms so I don't think it's an issue.  
However, it will be going away since UNICODE doesn't work on 9x.
 
> You also use the wide character version of CreateMutex here: but 
that isn't
> supported on Win9x (and fails).

Pointed out, and will be changed.
 
> xtime.cpp:
> ~~~~~~
> 
> I would prefer the section:
> 
> #else
>         return 0;
> #endif
> 
> to be replaced by
> 
> #else
> #  error
> #endif
> 
> It makes it easier to catch a mis-configured set of headers.

But won't be correct for the final "full" implementation, since not 
all platforms will support all clock types.
 
> Headers:
> ~~~~~~
> 
> Can we have a section that produces a #error if BOOST_HAS_THREADS is
> defined, but no supported threading API is configured?

With boost/thread/config.hpp this problem isn't possible.  
BOOST_HAS_THREADS is conditionally defined dependent on those other 
macros as well as detection of whether or not threading support 
is "on" by the compiler.  I'm not sure how to work this out with the 
new config system.  I'll have to get some help integrating this 
portion if accepted.
 
> Config:
> ~~~~
> 
> We need some test code for each of the macros (to slot into the
> split-config's regression tests, and configure script).
> We also need to figure out what the appropriate POSIX feature tests 
are for
> some of these API's, and add them to the new config.

Some don't have feature tests, unfortunately.  Getting the config 
stuff correct is going to take some doing.
 
Bill Kempf