Re: My -short- lock-free sequencer class, I want to see your comments
K?r?at wrote:
Hi,
Recently I needed a sequencer for my IOCP based socket server and
developed one. I try to implement it in lock-free manner. Your comments
will bee appreciated.
The class is very small and all it do is to maintain two numbers in thread
safe manner. Before every WSARecv call I get next available sequence
number from sequencer and put that number into my PerIoContext object.
When a recv completion occured then I check the sequence number to decide
if the completion occured in order. So one of the two numbers
(m_lCurrentSequence) represents call sequence and the other
(m_lRunningOrder) represents completion sequence. Here is the class :
class Sequencer
{
public:
Sequencer (LONG lMaxSequence) : m_lCurrentSequence (0),
m_lRunningOrder
(0), m_lMaxSequence (lMaxSequence)
{
}
Single-argument constructor that is not declared 'explicit' allows implicit
conversions, which are usually bad. Further, your class is still
default-constructible, copyable and assignable.
LONG getNextSequence ()
{
LONG lCurrentSequence, lNextSequence;
while (true)
{
InterlockedExchange (&lCurrentSequence,
m_lCurrentSequence); lNextSequence = (lCurrentSequence ==
m_lMaxSequence ? 0 :
lCurrentSequence + 1);
if (lCurrentSequence == InterlockedCompareExchange
(&m_lCurrentSequence, lNextSequence, lCurrentSequence))
{
break;
}
}
return lNextSequence;
}
Inconsistent formatting, making it harder to understand what's going on.
Further, the whole code is undocumented. Documenting the design of code
often leads to the discovery of logic errors and allows maintenance by
others.
bool isInOrder (const LONG lSequence)
{
return (lSequence == m_lRunningOrder);
}
Should be const.
bool updateRunningOrder (const LONG lSequence)
{
if (isInOrder (lSequence))
{
// Safe region...
LONG lNewRunningOrder = (lSequence + 1) > m_lMaxSequence ? 0 :
(lSequence + 1);
InterlockedExchange (&m_lRunningOrder, lNewRunningOrder);
return true;
}
return false;
}
private:
LONG m_lMaxSequence;
LONG m_lCurrentSequence;
LONG m_lRunningOrder;
};
InterlockedExchange() takes a pointer to a volatile LONG. While this is
automatically added in a function call, it is still necessary in
isInOrder(). Why? Simple reason: the compiler implements read-accesses to a
volatile in a way similar to the Interlocked*() functions. In fact, a
simple InterlockedRead() function is missing from that API.
Uli
--
C++ FAQ: http://parashift.com/c++-faq-lite
Sator Laser GmbH
Gesch??ftsf??hrer: Thorsten F??cking, Amtsgericht Hamburg HR B62 932