Attempt to expand listener to multi-threaded #1313
Conversation
This commit resolves issue pistacheio#1312. Listener now supports multiple "acceptor" threads handling new client socket connection. In http this has no benefit, as there is only one `accept()` syscall involved. However, if the server is armed with ssl, this significantly increases the server performance regarding tps. The single server thread impedes the tps in two ways: 1. Regular tcp handshake is "non-blocking" to server, as long as server sends the syn+ack frame, server side is considered established, the `accept()` returns. Whereas with ssl, the server needs to send the its cert and wait for client's response, this blocks the `SSL_accept()` call until the handshake completes. If the client comes from a remote data center with large latency, the latency contributes to the blocking of listener thread, resulting very low tps. 2. The ssl handshake involves cryptographic arithmetic, which is CPU bound. Even if client to server latency can be ignored, one listening thread is only able to handle ~1000 tps on a Intel Xeron core @2.1GHz. In this commit, we add two more options to Endpoint::Options that specifies the number of "acceptor" threads and the thread name, Listener::runThreaded() consumes them and initializes this number of threads. To make every thread has a equal chance to receive epoll events, the lock operation is removed before calling `poller.poll()`, there are only two fds registered to Listener's poller, the listen_fd and shutdownFd, there is no scenario regarding fd being removed while being handled.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1313 +/- ##
==========================================
- Coverage 76.29% 75.90% -0.39%
==========================================
Files 64 63 -1
Lines 11115 9946 -1169
==========================================
- Hits 8480 7550 -930
+ Misses 2635 2396 -239 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Should bump minor version as this PR addes new interfaces.
|
Hi @jiankyu - Couple of things. 1/ Windows compile error The build sees error: This is in the code to set the "thread description" in the Windows case. I think you copied this over from reactor.cc? Which is fine. But you would also need to copy over (i.e. add to listener.cc) the declaration: 2/ Test case I take it from the prior conversation that you are seeing the SIGPIPE failure from your own application. It seems the SIGPIPE is not demonstrated by the existing Pistache unit tests? Presuming not, could you add a test that activates the problem? Thanks in advance. |
|
@jiankyu, I don't know if it's possible through a unit test, but it would be helpful if you are able to benchmark before and after your (finalized) PR when ready. That will help us evaluate whether the underlying resource bottleneck problem has been resolved. |
|
I actually did the benchmarking. Below is the test data. The numbers are generated by a multi-threaded client request generator. As we can see, the maximum ops we can get is 15304.74 at 50 threads, further increasing the threads doesn't increase the ops. |
|
Cool. Out of interest, what kind of CPU was it, with how many parallel execution units? Also, as per @kiplingw's prior comment, it would be awesome to put some form of this - e.g. 1, 2, 25, 50, 75 threads for a few seconds each - into a unit test if practical. |
|
|
Unfortunately the current Http::Experimental::Client() can't be used to benchmark this. There are couple of problems with it. The Client performance issue can be resolved by multiple clients making requests simultaneously, but I got stuck at the connection reuse problem, I tried different fixes but none of them works so far. |
The shutdownFd is used to tell the acceptor worker thread to exit, when there is multiple threads, the current implementation is not enough to ensure every worker has a chance to get the event, if it's busy doing something, it will miss the notification. Without changing the NotifyFd interface, the guaranteed way is to do a loop notification and check the number of alive worker threads, keep notifying them until all the threads are done.
Attempt to resolve #1312. Changes summary can be found in commit message.
This is still working in progress, stability issue present. Bring in early to check if any failed CI job can shed a light.