[I] FeedSlaveThread uses blocking connection in libevent [kvrocks]

["zhixinwen (via GitHub)" <gi...@apache.org>]

zhixinwen opened a new issue, #3113:
URL: https://github.com/apache/kvrocks/issues/3113

   ### Search before asking
   
   - [x] I had searched in the [issues](https://github.com/apache/kvrocks/issues) and found no similar issues.
   
   
   ### Motivation
   
   Let's created a separate issue to track: https://github.com/apache/kvrocks/issues/3097#issuecomment-3153004778
   
   In theory, we should only use non-blocking socket in bufferevent. However, we are now essentially using a blocking socket in the read callback
   
   
   ### Solution
   
   _No response_
   
   ### Are you willing to submit a PR?
   
   - [ ] I'm willing to submit a PR!


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] FeedSlaveThread uses blocking connection in libevent [kvrocks]

["zhixinwen (via GitHub)" <gi...@apache.org>]

zhixinwen commented on issue #3113:
URL: https://github.com/apache/kvrocks/issues/3113#issuecomment-3181681580

   I tried a naïve way to use libevent and non-block connection to handle `FeedSlaveThread`,but I did not see much performance difference. It may not worth the effort to optimize this, will try some other ways.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [I] FeedSlaveThread uses blocking connection in libevent improperly [kvrocks]

["weim0000 (via GitHub)" <gi...@apache.org>]

weim0000 commented on issue #3113:
URL: https://github.com/apache/kvrocks/issues/3113#issuecomment-5643056508

   I took a closer look at the current implementation, and I’m not sure using a blocking socket in FeedSlaveThread is actually a performance issue here.
   
   As I understand the current design, FeedSlaveThread is only responsible for sending replication data to the replica. It does not synchronously wait for data or ACKs from the replica. The read events are handled asynchronously by the worker’s event_base through bufferevent.
   
   In the normal case, as long as there is enough space in the kernel TCP send buffer, write() just copies/enqueues the replication data into the kernel send buffer and returns. From this perspective, I don’t think the current synchronous write path is inherently slower than sending the same data asynchronously through libevent/bufferevent. The asynchronous approach still eventually needs to submit the data to the socket; it mainly changes when and where the write is performed rather than making the actual socket transmission faster.
   
   If the replica is slow or the TCP send buffer becomes full, FeedSlaveThread may indeed block on write(). However, I think this behavior may actually be reasonable here. Each replica has its own FeedSlaveThread, so blocking only applies backpressure to that particular replica and does not block the worker’s event loop or other replicas. It also naturally prevents replication data from accumulating indefinitely in a user-space output buffer.
   
   So the current model seems to be intentionally mixing two I/O models:
   
   * Read path: asynchronous, handled by bufferevent and the worker’s event_base.
   * Write path: synchronous/blocking, handled by the dedicated FeedSlaveThread.
   
   Given this architecture, I’m not convinced that changing the write path to non-blocking I/O through libevent/bufferevent would provide a performance benefit. In the common case where the socket is writable, I would expect the current direct write approach to have comparable performance, while the asynchronous approach may introduce additional output buffering and state management without reducing the actual amount of socket I/O.
   
   One thing that may still be worth verifying is whether the fd managed by bufferevent is actually kept in blocking mode when FeedSlaveThread calls SockSend(), since SockSend() appears to assume a blocking fd. If the fd is non-blocking, handling of EAGAIN/EWOULDBLOCK would be a separate correctness concern.
   
   Please let me know if I missed anything about the intended threading or I/O model here.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org