Re: Boost.Threads draft library submission

From: williamkempf_at_[hidden]
Date: 2001-03-15 10:02:19

--- In boost_at_y..., "Dan Nuffer" <dnuffer_at_c...> wrote:
> 
> William Kempf wrote:
> > Despite the delay I've uploaded a _DRAFT_ library submission for
> > Boost.Threads.  It's not 100% ready for prime time, but it's 
ready to
> > share.  Below is a list of things I'd appreciate others on this 
list
> > helping out with:
> >
> > 1) Evaluate the code and documentation much as you would for a 
formal
> > submission.  This particular library is going to need a bit more
> > scrutiny than normal, so I'd like to do some of this prior to
> > submission.
> >
> 
> I have a vested interest in Boost.Threads, as almost every project 
I've
> worked on for the past 3 years has had it's own implementation of 
C++
> threads, and I think it sure would be nice to have something to 
standardize
> on.
> Anyway, I looked at all the code, and documentation, and here are 
some of my
> thoughts.

Thanks very much for your interest and comments.

> One of the most commonly used syncronization primitives in the 
projects I
> have worked on has been a read-write lock.  I was surprised that it 
wasn't
> implemented, and I think it should be part of the library.  They 
are trivial
> to implement for pthreads, and if you want to see an example of how 
to do
> them on windows here is a link to some code of CDSA that does it:
> http://cvs.sourceforge.net/cgi-
bin/cvsweb.cgi/cdsa_dev/cdsa/src/fwk/port/por
> tswmr.c?rev=1.2&content-type=text/x-cvsweb-markup&cvsroot=cdsa

Read/write locks are trivial to implement on any platform.  They 
weren't forgotten here, they were just considered to be of a higher 
level of complexity with lesser actual need and so were left out in 
Phase 1.  I expect to add them in Phase 3.  The introduction in the 
documentation will help you to understand why, though this type was 
not specifically mentioned.

> Another issue is that of how the code is organized.  This appears 
in most of
> the source files:
> #if defined(BOOST_HAS_WINTHREADS)
> all the windows code
> #elif BOOST_HAS_PTHREADS
> all the pthread code
> #endif
> 
> I think it greatly helps readability of the source to separate the 
windows
> code into one file and the pthread code into another file.  Then 
when
> compiling the library, the appropriate file is compiled.  You can 
even help
> out organization by creating separate directories such as 
src/windows and
> src/pthread and putting the respective files under each directory.

I originally did this, and refactored the code right before 
submitting this draft.  I actually found the file seperation to be 
more difficult to maintain and it did cause a (very minor) reduction 
in compile time.  I'm not against this seperation, but before going 
back to it I'd appreciate comments from others.

> You also have a lot of places where you either include windows.h or
> pthread.h.  The code would be cleaner if you created a separate 
file maybe
> called platforminc.hpp which has the #ifdef to include either 
windows.h or
> pthread.h and then in all your code you just include 
platforminc.hpp.  The
> goal is to clarify the code by minimizing the number of spots of 
conditional
> compilation.

I'm less convinced that this would be beneficial.  The includes are 
all implementation details and don't appear in the header files at 
all.  So the only people that will see this at all are those 
developing/maintaining the library.  If the structure is left as is, 
with the conditional compilation in one file for platform specific 
code as you pointed out above, this extra conditional detracts 
nothing to the over all readability of the source.  If the 
implementations are seperated into platform specific source files the 
conditional naturally disappears since the file inclusion is moved to 
the platform specific file.

> In the semaphore docs you mention that the semaphore is the simplest
> primitive.  It seems to me that a mutex is simpler than a semaphore,
> although I guess in reality it's the same thing, a mutex is just a 
semaphore
> with limits of 0 and 1.

Mutexes are also, at least conceptually, restricted to lock/unlock 
pairs within a single thread.  Many libraries enforce this with it 
being an error to unlock a mutex in a thread that did not lock it.  A 
semaphore, on the other hand, is not restricted in this manner at all.

That said, if the wording here still seems misleading or inacurate 
I'm willing to change it.

> You also mention that the semaphore is dangerous, but don't explain 
why.

The semaphore isn't, only the interface given for it is.  I thought 
this was appearant, but I'm too close to this stuff now to be 
objective.  I'll try and rework this section of the documentation.

> The docs don't explain the arguments to the semaphore constructor.  
I
> understood max, but count wasn't too obvious, is it the initial 
value of the
> semaphore?

Yes.  Another oversight.  I'll rework this section as well.

> I don't like the names up and down for the semaphore member 
functions.  I
> haven't ever seen this terminology used.  In college, I learned 
about wait
> and signal operations on semaphores, and I believe that is the most
> prevalent usage (other than p and v)  Even the windows API uses the 
terms
> wait and signal.

Actually, the Windows API does not.  Waiting is done through the 
generic WaitFor* APIs, which could be taken to be the same as "wait" 
since the word is found in the longer name, but the "signal" 
functionality is named ReleaseSemaphore which has no relation at 
all ;).

The point is, the only "common" names for these methods are P and V, 
which make little sense to someone unfamiliar with the concept.  It 
was decided (and not by me) early on during the design of this 
library that up and down convey the actual functionality better than 
any other names.  I'm not stuck with this, but because it's already 
been discussed I think you'll find few people who want to change the 
names here.  So, with out compelling reasons why, I think the names 
will stay.

> The example you provided for the semaphore is more appropriate for 
a mutex.
> I think that a bounded buffer would show the semaphore concept 
better.

I very much agree that the example is contrived and does not show the 
benefits of the semaphore at all.  I don't think a bounded buffer is 
a better example, however, as that example is better suited for (and 
given in the documentation for) a condition.  I'll think about a 
better example for this one.

As I think you can tell, I put little effort into the documentation 
for the semaphore, being more concerned with the mutexes and 
condition.  I wrote the docs for the semaphore near the end, when I 
was sick of writing documentation ;).

> In mutex_concept.html under the Locking Strategies heading, the 
first
> sentence of each strategy is hard to understand.  Maybe there is a 
better
> wording?

I'll look at this as well.
 
> You mention that the user shouldn't include <boost/xlock.hpp> to 
further
> enforce the idea, I think you should move it into boost/detail

I did not say they "shouldn't", just that they normally "won't".  The 
types are useful for developing other mutex concepts, and as such I 
don't want to place them in a detail namespace.  Maybe I can convey 
this better in the documentation... I'll give that some thought.
 
> I think you should move a lot of the simple 1-3 line functions from 
the .cpp
> files into the .hpp files such as:
> 
>     semaphore::semaphore(unsigned count, unsigned max)
>         : pimpl(new impl(count, max))
>     {
>     }
> 
>     semaphore::~semaphore()
>     {
>         delete pimpl;
>     }
> 
>     bool semaphore::up(unsigned count, unsigned* prev)
>     {
>         return pimpl->up(count, prev);
>     }
> 
>     bool semaphore::down(unsigned milliseconds)
>     {
>         return pimpl->down(milliseconds);
>     }

This would require the "impl" class to be visable to client code, 
which is exactly the opposite of the purpose.  The implementation is 
supposed to be hidden completely from client code.  Specifically, I 
don't want client code to include files such as Windows.h, even 
indirectly, since this will polute the namespace and slow 
compilation.  All refactoring that will place these functions in the 
header and yet insure my goal of hiding the implementation will 
result in no speed increase from inline optimizations.
 
> Also, I wonder if the advantages of the pimpl idiom are greater 
than the
> disadvantages?  I definitely think it will cause a performance hit.
> Everytime you lock or unlock a mutex, you've got to dereference the 
pimpl
> pointer.  And for every primitive you create you've got to call new 
and then
> delete when it goes away.  Is there any advantage other than a 
slightly
> faster compile?

It's more than "slightly" on many platforms, Windows being one of 
them.  Further, I don't want to pollute the namespace.  The only 
overhead occurs within construction/destruction, which are not likely 
to occur in time critical locations and so shouldn't be noticed in 
most applications.
 
> > 3) I need serious help in creating some sort of build system.  I
> > realize this is a touchy subject since Boost doesn't yet have a
> > standard way of handling this, but I'll settle for multiple 
platform
> > specific makefiles much as Regex does.  I'm just not qualified to
> > handle this part.
> >
> 
> I can probably help you out on that.  Let me see what I can come up 
with.

I'd appreciate the help.  We need to coordinate things like this so 
that multiple people aren't working on the same thing.  Are you 
volunteering for specific platforms?

Thanks for the comments.

Bill Kempf