Re: Request for comments about synchronized queue using boost
On Oct 17, 9:12 am, James Kanze <james.ka...@gmail.com> wrote:
On Oct 15, 6:02 pm, Maxim Yegorushkin
<maxim.yegorush...@gmail.com> wrote:
On Oct 15, 2:36 pm, Nordl=F6w <per.nord...@gmail.com> wrote:
[...]
Apart from that, the mutex is held for too long. You don't
really need to hold the lock when allocating memory for
elements and when invoking the copy constructor of the
elements.
Which sounds a lot like pre-mature optimization to me. First
get the queue working, then see if there is a performance
problem, and only then, do something about it. (Given that
clients will normally only call functions on the queue when they
have nothing to do, waiting a couple of microseconds longer on
the lock won't impact anything.)
Here is an improved version (although a bit simplified):
void push(T const& t)
Actually, I'd name this "send", and not "push". Just because
the standard library uses very poor names doesn't mean we have
to.
{
// this avoids an allocation inside the critical sectio=
n
bellow
queue_type tmp(&t, &t + 1);
{
boost::mutex::scoped_lock l(mtx_);
q_.splice(q_.end(), tmp);
}
cnd_.notify_one();
}
This is unnecessary complexity. And probably looses runtime
efficiency (not that it's important): his initial version uses
std::deque, which doesn't have to allocate at each
insertion---in fact, in all of the uses I've measured, the queue
tends to hit its maximum size pretty quickly, and there are no
more allocations after that.
Yet another case where premature optimization turns out to be
pessimization.
[...]
// this function provides only basic exception safety if T's
// copy ctor can throw or strong exception safety if T's copy
// ctor is nothrow
:-).
In practice, I find that almost all of my inter-thread queues
need to contain polymorphic objects. Which means that the queue
contains pointers, and that all of the objects will in fact be
dynamically allocated. The result is that I use std::auto_ptr
in the interface (so the producer can't access the object once it
has been passed off, and the consumer knows to delete it).
I agree with you that holding work elements by value is not most
practical. boost::function<> and std::list<> were used only for
simplicity here.
As you said, in practice, the work units are dynamically allocated
polymorphic objects. Naturally, the work unit base class is also a
(singly-linked) list node and the queue is implemented as an intrusive
list. This way, once a work unit has been allocated, the operations on
the inter-thread queue do not involve any memory allocations.
Something like this:
struct WorkUnit
{
WorkUnit* next;
WorkUnit()
: next()
{}
virtual ~WorkUnit() = 0;
virtual void execute() = 0;
virtual void release() { delete this; }
};
template<class T>
struct IntrusiveQueue
{
T *head, **tail;
IntrusiveQueue()
: head()
, tail(&head)
{}
void push_back(T* n)
{
*tail = n;
tail = &n->next;
}
T* pop_front()
{
T* n = head;
if(head && !(head = head->next))
tail = &head;
return n;
}
};
--
Max