search

Azure / azure-amqp

AMQP C# library
Other
94 stars 72 forks source link

Reduce locking on IssueCredit path #180

Closed danielmarbach closed 3 years ago

danielmarbach commented 3 years ago

TODO

danielmarbach commented 3 years ago

Closing for now since the field access patterns used in these classes truly requires atomic modifications and I cannot say I fully understand it yet.

xinchen10 commented 3 years ago

Really appreciate your work to improve the library. AMQP semantically is single threaded at the connection level. Ordering is critical to maintain protocol state. This is the main reason the current implementation uses an object level lock to protect protocol state and any operations that could change that state (e.g. SendFlow). Some places could use a smaller coped lock (e.g. message queue and waiters in receiving link) but that does not change much because the lock contention just goes to a different place where things need to be synchronized. Without locking the commands would go on wire out of order. For example, the app issues credit 1, 3, 5 in that order and the flow to the peer could be 1, 5, 3.

This implementation is optimized for the message transfer scenarios, where you set up a link with some reasonable credits and handle messages from the OnMessage handler. The receive APIs have too much overhead, especially when they are called concurrently.

Long term solution to this problem would be to maintain a queue for operations that change protocol state and a single thread to process the work from that queue. The queue could be at the session level or the connection level. Links require delivery ID and state that are managed by session so we cannot implement the operation queue at the link level efficiently.

Right now there are some optimizations we could do. For example, IssueCredit could be called out side of the lock if we add extra logic to maintain calling order.

danielmarbach commented 3 years ago

I've done a few local spikes to replace for example the buffer management with ArrayPool which is doable but then I stumbled over some edge cases I wasn't sure about and stopped.

I think so many of the synchronisation here could be replaced with System.Threading.Channel allowing multiple writers but a single reader. Maybe I'll find some time. Would you be interested to see those spikes here as a means of seeing the thoughts which might trigger other ideas?