Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a lock free implementation of ConnectionBase write message #4809

Merged
merged 16 commits into from
Sep 28, 2023

Conversation

vietj
Copy link
Member

@vietj vietj commented Aug 11, 2023

The ConnectionBase class implementation has a few drawbacks

  • ConnectionBase#writeToChannel currently uses an implicit queue (writeInProgress field + event-loop scheduling)
  • use locks assuming biased locking
    -back-pressure is not correctly handled by usage outside of its event-loop (e.g a pooled HTTP client connection) requiring work around to handle back-pressure

This PR use a new queue to solve these problems:

  • reduce the number of scheduled tasks when writing from another thread than the event-loop (producer thread)
  • use lock free structure
  • connection back-pressure is now based on channel writability and the number of inflight messages

In addition it aims to improve producer message flush consolidation thanks to the queue: when a producer writes several messages and then schedules a drain, the drain consolidates the writes.

The introduced OutboundWriteQueue is a multi producer queue with a single consumer event-loop thread (MPSC) optimised for the ideal case (event-loop thread). The internal is inspired from https://akarnokd.blogspot.com/2015/05/operator-concurrency-primitives_11.html and reuse the MPSC queue + WIP atomic.

Write operations now returns a boolean indicating the writability, which should be used in the future for stream pipe like operations to replace the write then writeQueueFull which can be racy when used outside of the event-loop. It is not yet used but is reserved for a future usage.

Note to reviewers: this PR is structured as is

  • OutboundWriteQueue is a lock free queue that can be reviewed independently
  • ConnectionBase changes replace the use of internal state + message rescheduling by the OutboundWriteQueue
  • the other commits are update the various usage of ConnectionBase
    • HTTP/1.1 server schedules HTTP messages to be written and update Http1xConnectionBase state using those messages
    • HTTP/1.1 client schedule custom messages to update the state of the connection and perform a direct message write
  • There is still some cleanup to do in OutboundWriteQueue tests (try avoiding hooks if possible + commented/ignored tests), this will be done of course

@vietj vietj added this to the 5.0.0 milestone Aug 11, 2023
@vietj vietj force-pushed the write-message-lock-free branch 3 times, most recently from 4df2117 to 92240d2 Compare August 20, 2023 09:15
@vietj vietj force-pushed the write-message-lock-free branch 18 times, most recently from 6e38f1f to b5de493 Compare August 26, 2023 16:34
@vietj vietj force-pushed the write-message-lock-free branch 5 times, most recently from c4b6184 to be6aaf9 Compare August 29, 2023 07:35
…current multi producers back-pressured queue fronting a single back-pressured consumer.

Producers emission flow cannot reliably be controlled by the consumer back-pressure probe since producers emissions are transferred to the consumer thread: the consumer back-pressure controller does not take in account the inflight elements between producers and consumer. This queue is designed to provide reliable signals to control producers emission based on the consumer back-pressure probe and the number of inflight elements between producers and the consumer.
…nt-loop and takes in account such writes (called write in progress) when they happen to preserve order. The current implementation 3 drawbacks

- the implementation uses Java locks and assumes biased locking to erase those locks when use from the event-loop thread
- as soon as a message is rescheduled on the event-loop thread, any following message will follow the same path, creating an implicit message queue on top of the event-loop queue
- the back-pressure probe does not account the infight messages, e.g Http1xClientConnection has to use its own back-pressure controller to work around it and avoid the producer to overflow the connection, this happens because we use a connection pool and we cannot guarantee to use the same event-loop.

The new MPSC write queue (OutboundWriteQueue) rewrites some parts of ConnectionBase to use an explicit MPSC lock free queue instead, as consequence and solve the above issues.
@vietj vietj marked this pull request as ready for review August 29, 2023 08:13
@vietj vietj self-assigned this Aug 29, 2023
@vietj vietj requested a review from jponge August 29, 2023 08:16
@vietj
Copy link
Member Author

vietj commented Aug 29, 2023

@franz1981 can you have look ?

Copy link
Member

@jponge jponge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a first pass focusing on OutboundWriteQueue, see my remarks and suggested edits.

The queue logic looks ok to me, although I will do another pass.

/**
* @return the queue high-water mark
*/
public int highWaterMark() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this and lowWaterMark aren't in use

Copy link
Contributor

@franz1981 franz1981 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently in PTO till 4 sept, so just few comments

@vietj vietj merged commit 0ae0844 into master Sep 28, 2023
6 checks passed
@vietj vietj deleted the write-message-lock-free branch September 28, 2023 08:10
@vietj
Copy link
Member Author

vietj commented Sep 28, 2023

thanks @jponge @franz1981 for the review, it has been really helpful and improved the quality of this PR, I believe those changes will have a great impact on the internals of Vert.x 5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants