roc-streaming / roc-toolkit

Real-time audio streaming over the network.
https://roc-streaming.org
Mozilla Public License 2.0
1.09k stars 213 forks source link

Make core::Pool lock-free #602

Open gavv opened 1 year ago

gavv commented 1 year ago

Last revised: Jun 2024

core::SlabPool implements slab pool. (Actual implementation is in core::SlabPoolImpl).

When an object is allocated, there are two main paths:

Currently, both paths runs under a mutex. Hot path uses a doubly linked-list of free slots.

We should update pool so that hot path will not use mutex, and will use lock-free freelist instead.

This is important to prevent priority inversion problems, because shared pools are used by both realtime and non-realtime threads.

Steps:

This excellent article provides ABA-free implementation of lock-free freelist: Solving the ABA Problem for Lock-Free Free Lists

Hassall commented 1 year ago

can you assign it to me?

gavv commented 1 year ago

Sure, you're welcome

Hassall commented 1 year ago

@gavv

because shared pools are used by both realtime and non-realtime threads

are these long lived threads? I was thinking about a different approach where we could use thread_local storage which would avoid dealing with mutexes and atomics.

gavv commented 1 year ago

Hi,

I thought about TLS too, but there are several objections for it here:

Using TLS probably can be additional optimization enabled only for some use-cases / platforms, though I'm not sure we need it at current point.

Hassall commented 1 year ago

I'll try to have a PR out sometime this weekend.

gavv commented 1 year ago

Awesome, thanks

Hassall commented 1 year ago

hey @gavv, sorry for getting to this late.

elements should inherit node class, so that node data is embedded into element instead of being allocated separately.

Question:

The lock-free ABA solution leverages a tag (std::uint32_t). To avoid the FreeList class from making allocations separately, ListNode::ListNodaData would then need a new member (...or perhaps some hack to repurpose ListNode::ListNodeData.prev since it's not used in FreeList).

What are your thoughts?

(Another issue I thought I was going to run into was that none of the ListNode::ListNodeData members are of type std::atomic but it seems by using AtomicOps, I can perform atomic operations on any type)

gavv commented 1 year ago

I think we need a separate class for freelist nodes (FreelistNode), that will have whatever fields we need. Usually each intrusive container has its own base class for nodes.

We don't use std::atomic because we use C++98. We have two classes for atomics: core::AtomicOps and core::Atomic<T>. In this case, I think we need AtomicOps because it allows to control which memory order to use. Yes, it operates on regular types like int or pointer. You can take a look how it's done in core::MpscQueue.

Hassall commented 1 year ago

I think we need a separate class for freelist nodes (FreelistNode), that will have whatever fields we need. Usually each intrusive container has its own base class for nodes.

~I see. In that case we wouldn't be able to do an in-place swap of List with FreeList? Curious what the plan is on how to proceed once FreeList and FreeListNode classes are completed~.

I see we just need to make Slab and Slot inherit from FreeListNode.

gavv commented 1 year ago

I see we just need to make Slab and Slot inherit from FreeListNode.

Exactly

gavv commented 9 months ago

@Hassall Hi, do you still have plans on this?

gavv commented 5 months ago

Unassigning, so that someone could pick this up.