Add a multiple-producer single-consumer (MPSC) queue implementation #753
Conversation
LaurentiuCristofor
requested review from
senderista,
tengizk,
daxhaw,
mihirj1993,
yiwen-wong and
simone-gaia
| protected: | ||
| void enqueue_internal(mpsc_queue_node_t<T>* node); | ||
| mpsc_queue_node_t<T>* dequeue_internal(); | ||
|
|
||
| protected: | ||
| std::atomic<mpsc_queue_node_t<T>*> volatile m_head; |
There was a problem hiding this comment.
Why is everything protected? This class does not seem to have subclasses.
There was a problem hiding this comment.
Going with protected indicates that I don't mind having derived classes access this information. Going with private means that I really want to keep the implementation internal, which is not the case here.
When you don't know what you want, protected makes more sense. Private should be used when you really don't want the variables/methods to be used outside a class.
There was a problem hiding this comment.
I could agree if this was a public-facing API that and we did not have control over the sources (even in that case, you should design for "extensions" instead of giving access to the internal state of the class).
Since we do have control over the sources, I think is better to make everything private UNTIL there is a reason not to do so. When that happens, only the fields that need to be exposed to the subclass will be put in the protected section. This helps to understand the scope of a given field.
There was a problem hiding this comment.
One more thing is: if I see protected I assume there is a subclass, or I assume this class is designed to be subclassed. Weh I've seen protected here I started looking for the subclass. I think putting protected without a strong reason causes more harm than good.
There was a problem hiding this comment.
Actually, if it were public API, private would be a safer choice than protected, because protected would still expose the method/variable as part of the interface (for deriving classes), so you'd be forced to continue exposing what you've offered as protected.
But internally, I don't need to protect the code from its developers. And too often, when you derive a class, you want to access its internals directly, instead of exposing all access methods through public methods.
If the class has no derived classes, protected works just like private. And if it gets derived, it doesn't require updating to turn private fields into protected.
There was a problem hiding this comment.
there is no point in using protected id there are no subclasses
The point is that you allow any subclasses to use these fields.
It does matter because the subclass may change the value of the fields hence I need to be extra carful when using them from upper classes.
A subclass could also change the value of private fields through the public interface. I think you want to say something different, but I'm not sure exactly what that is.
Because seeing those protected fields I must assume some subclasses may use them and I must check what they are used for. To me it's scope-creep.
You only need to do this if you intend to change the class. In which case, you'd do enough look-around to figure out how it works that you couldn't avoid finding out if it's sub-classed or not.
There was a problem hiding this comment.
@senderista : Don't mind us. :)
I hope that the comments I added help explain the algorithm and my modifications to it.
There was a problem hiding this comment.
I didn't have time to review this, but I did notice that "head" denotes the end of the queue used to enqueue items, and "tail" denotes the end of the queue used to dequeue items. This is the reverse of the normal convention and I find it very confusing when reading the code.
There was a problem hiding this comment.
Never mind, I see the original code uses the same convention. It's fine.
There was a problem hiding this comment.
Right, I kept the naming close to the original to make it easier to do a side-by-side comparison.
@senderista suggested using an MPSC queue implementation from 1024cores.net:
https://www.1024cores.net/home/lock-free-algorithms/queues/intrusive-mpsc-node-based-queue
This PR includes a C++ implementation that is derived from that 1024cores.net C implementation.
I changed the current use of a different queue implementation to this new one.
The code requires a specific license notice to be distributed, so I added that notice under a new
licensesfolder underproduction. If we have any other bits of code that require specific licenses to be distributed, please add them under this folder and ensure that the name of their containing folder describes the code to which they apply.