Common: Add thread-safe CircularQueue template and make Mixer use it. #13635
Conversation
jordan-woyak
force-pushed
the
circular-queue
branch
3 times, most recently
from
ba5723d to
dd84380
Compare
jordan-woyak
force-pushed
the
circular-queue
branch
from
dd84380 to
e04e98f
Compare
| // | ||
| // Capacity() gives size of the underlying buffer. | ||
| // | ||
| // Push'ing Size() beyond Capacity() is unchecked and legal. |
There was a problem hiding this comment.
This is only true if the queue is used by a single thread; with separate threads if Size() > Capacity() then you can have a data race.
There was a problem hiding this comment.
Yes. I will make that more clear in the comment.
| // Push/Pop increases/decrease the Size() | ||
| // | ||
| // Capacity() gives size of the underlying buffer. |
There was a problem hiding this comment.
| // Push/Pop increases/decrease the Size() | |
| // | |
| // Capacity() gives size of the underlying buffer. | |
| // Push/Pop increases/decreases the Size() | |
| // | |
| // Capacity() gives the size of the underlying buffer. |
| static_assert(std::is_trivially_copyable_v<ElementType>, | ||
| "Really only sane for trivially copyable types."); |
There was a problem hiding this comment.
What goes wrong if it's not trivially copyable?
There was a problem hiding this comment.
My concern is the Size() > Capacity() race that you've noticed. Restricting the types to trivially copyable ones makes the race "less terrible". I suppose I could just state that Size exceeding Capacity isn't strictly legal for the SPSC versions. Users should "know what they are doing".
| { | ||
| } | ||
|
|
||
| // "Push" functions are only safe from the "producer" thread. |
There was a problem hiding this comment.
| // "Push" functions are only safe from the "producer" thread. | |
| // "Write" and "Push" functions are only safe from the "producer" thread. |
| AdjustSize(count, order); | ||
| } | ||
|
|
||
| // "Pop" functions are only safe from the "consumer" thread. |
There was a problem hiding this comment.
| // "Pop" functions are only safe from the "consumer" thread. | |
| // "Read" and "Pop" functions are only safe from the "consumer" thread. |
| void AdvanceReadHead(std::size_t count) | ||
| { | ||
| m_read_index = (m_read_index + count) % Capacity(); | ||
| AdjustSize(0 - count, std::memory_order_relaxed); |
There was a problem hiding this comment.
0 - count underflows here, which is cancelled out by m_size += count (or fetch_add) overflowing in AdjustSize. The math works, but I feel like it'd be cleaner to have a bit of code duplication with separate IncreaseSize and DecreaseSize functions.
There was a problem hiding this comment.
I dunno, that's 10 lines of duplicate code just for being afraid of some unsigned integer underflow. :D
There was a problem hiding this comment.
Unsigned underflow is well defined, so I'd say take advantage of it.
| // Used by the "consumer" to move the read position backwards. | ||
| // Also increases the size by the given amount. | ||
| void RewindReadHead(std::size_t count) |
There was a problem hiding this comment.
If the consumer thread uses this at the same time as the producer thread uses AdvanceWriteHead that could cause Size to exceed Capacity even if both threads check if there's enough space beforehand, potentially causing a data race.
I'd suggest disabling this when UseAtomicSize is true to maintain the invariant of Only the producer thread can increase the Size, and only the consumer thread can decrease it.
There was a problem hiding this comment.
Yes. I would also rather this function didn't exist at all for all the reasons you mention.
Unfortunately "Mixer.cpp" is already using this racey technique, and I've begrudgingly used it there. The changes are in this PR: e04e98f
Maybe I can add a comment about the consequences of RewindReadHead and to not use it and that it is pending removal? I don't know. Fixing Mixer to not do this doesn't look straightforward.
I really don't like looking at the manual circular buffer atomic head/tail math in Mixer.
I've created a CircularQueue class template with some unit tests.
There's an atomic-using thread-safe
SPSCCircularQueueversion and aWaitableSPSCCircularQueueversion that can block for data or room for pushing data.I plan to also use this for Wii Speak Microphone after the merge of #12769 which currently has a mutex around a manual circular buffer.
I have slightly altered the logic of fill audio gaps (off by one stuff).
It seems somewhat cleaner this way, but hopefully @Sam-Belliveau can tell me if that should be changed back.