Re: [Boost-users] [Review] Lockfree review starts today, July 18th

Subject: Re: [Boost-users] [Review] Lockfree review starts today, July 18th
From: John Bytheway (jbytheway+boost_at_[hidden])
Date: 2011-07-19 16:52:57

On 19/07/11 00:08, Tim Blechmann wrote:
>> - You describe it as a 'limitation' that "The class T is required to
>> have a trivial assignment operator.".  I think this is better described
>> as a 'requirement'.  Are there any other requirements?  I'm guessing it
>> must be CopyConstructible, or at least MoveConstructible?  Are you sure
>> you don't need a trivial destructor and/or copy constructor in addition
>> to the trivial assignment?
> 
> hm, you are right. it both needs to have a trivial destructor and a copy 
> constructor. i don't want to provide any move semantic, enqueue/push may fail if 
> no nodes can be allocated.

Well, indeed it seemed unlikely that move semantics could usefully be
supported.

>> - fifo::empty is described as non-thread-safe.  To what extent?  Might
>> it crash the program or corrupt the container, or is it simply that
>> other threads can change the content of the container at a whim, and so
>> the result of calling empty might not persist until the next operation?
> 
> it won't crash, but the answer will may be incorrect.

The docs should clarify exactly what can go wrong.

>> - Does the specialization fifo<T *, freelist_t, Alloc> also require that
>> T is trivially assignable, or even that it is assignable at all?  It's
>> not clear.  If it is actually storing pointers is there a risk of memory
>> leaks if the container is destroyed while non-empty?  If it's not
>> actually storing pointers, then what am I to do if I genuinely want a
>> fifo of pointers?  Generally I find this specialization a bit worrying
>> since its semantics differ so markedly from the unspecialized equivalent.
> 
> i actually like matthew's proposal of templating the dequeue/pop argument. that 
> would make the specialization obsolete.
> currently, destroying a non-empty fifo<T*> would leak memory.

I don't like this.  Fundamentally, you must decide whether a container
of pointers is responsible for the memory management of the pointers in
it.  Compare, for example, std::vector<T*> and boost::ptr_vector<T>.
Your interface is much more akin to the std:: containers, and I think
it's wiser to stick to that model.  If your users store pointers in it
they should concern themselves with the corresponding memory management
elsewhere.  In particular, the enqueue function should take a T*, not a
T as it presently does.  Adding half-hearted support to help them will
only lead to confusion, obscure memory leaks and problems in the long run.

If you want to have a container that does manage the memory of the
pointers in it, then you should create an entirely separate class for
that, rethink the interface from scratch, and take care of all the
corner cases like strong exception safety and freeing memory on
destruction.  This would certainly be a nice thing to have, but I don't
suggest you try to squeeze it into your first version of Boost.Lockfree
at this point.

>> - The fifo and stack insertion functions both return false if "if the
>> freelist is not able to allocate a new fifo node.".  Is this only when
>> using a fixed-sized freelist which has used all it's size, or might this
>> also happen with a caching freelist when allocation fails (e.g. due to
>> running out of memory).  To put it another way: how are you handling
>> exceptions thrown by the allocator?
> 
> no, but i guess for consistency reasons i should do that!

It's not clear that you want to catch the exceptions.  Personally I
would expect you not to, but the docs should certainly clarify either
way.  And you should check what guarantees your methods offer in the
face of such exceptions, and document them (I imagine the strong
guarantee should be no trouble here).

John