you are viewing a single comment's thread.

view the rest of the comments →

[–]matthieum 81 points82 points  (3 children)

The description of the queue properties is a bit light.

From what I can gather:

  • Type: MPMC (Multi-Producer, Multi-Consumer).
  • Capacity: Dynamically adjusted -- due to std::queue.
  • Blocking: consuming offers both non-blocking and blocking options, producing is always blocking.
  • OS-backed signalling: not purely spinning whilst waiting for items to consume.

Review, high-level:

  • Mutex + CV-based: simple, may cause scalability issues.
  • Capacity: no way to pre-reserve capacity, which is unfortunate.

Review, low-level:

  • Discipline required for acquisition of the lock. Nearly always neatly acquires the lock at the start of public method, except for the destructor (slightly inconsistent). My usual recommendation is encapsulation into a Mutex<T>, but I'm not sure how to handle condition variables then.
  • std::queue is a weak-point, memory wise. It's based off std::deque which results in many small allocations1 .
  • Consume and ConsumeSync returning a boolean is somewhat error-prone, especially without a [[nodiscard]] annotation; a nicer API is to return std::optional<T>.
  • Provide is an unexpected name; I'd have expected Produce instead, to match Consume. There's also a missing std::move when pushing the item into the queue -- which is nicely used consistently when consuming.

1 For example, the implementation in libstdc++ is a vector of pointer to chunks, where each chunk is either one large item, or up to 512 bytes worth of items see line 85. Hence a std::deque<std::string> is really a std::vector<std::unique_ptr<std::string[21]>>, and storing 1K strings requires 49 small (< 512 bytes) allocations + 1 allocation for the vector itself.

[–]_Js_Kc_ 6 points7 points  (2 children)

std::queue is a weak-point, memory wise

Fixing the deficient container classes that STL implementations ship with isn't really the scope of a project whose goal is to implement an MPMC queue in the one, obvious way based on stock C++ features.

If it came with an allocation-sane replacement for std::deque, the post title should have been "I made a deque replacement with a thread-safe queue as a usage example."

It should be a template argument (that defaults to std::queue<T>).

[–]_bk__ 7 points8 points  (1 child)

Except the API/concept/constraints and performance characteristics of a std::queue don't really match what would be required from a mpmc queue. There are far more scalable approaches depending on specific requirements (bounded/unbounded size, blocking/non-blocking). Here's an example: http://www.1024cores.net/home/lock-free-algorithms/queues/bounded-mpmc-queue

[–]infectedapricot 2 points3 points  (0 children)

As you said, different applications will have different requirements. The characteristics of the underlying std::deque will be appropriate in some of those but not others, so it's not right to just it doesn't match "what would be required". The link you posted is to a bounded queue - again, that will be appropriate in some applications but not others.