-
Notifications
You must be signed in to change notification settings - Fork 74
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
conduit-lwt-unix: se accept_n on the server #387
Conversation
Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
…n since run_handler and accept_n should not raise Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
@@ -10,6 +10,7 @@ val process_accept : | |||
unit Lwt.t | |||
|
|||
val init : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should probably document this, I was first waiting to see if you are interested at all and what the review says about the implementation
Signed-off-by: Marcello Seri <[email protected]>
Signed-off-by: Marcello Seri <[email protected]>
Adding the yield call greatly improves the latency, but apparently at a cost on lost connections. |
| `Accepted connections -> | ||
Lwt_list.iter_p | ||
(fun v -> | ||
connected (); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may also be a race to update connected here since I am calling iter_p
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite see the race you're referring to here -- where is the iter_p
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I am not mistaken iter_p
runs the iteration in parallel, and connected ()
increments the active connection count without any protection (let connected () = incr active
). My fear was that multiple connections executed in parallel could try to increment the variable simultaneously (and I don't want to put a mutex there).
This reverts commit e490592.
I have updated the test, using the test server suggested in inhabitedtype/httpaf#200 and the wrk2 call used there. In practice, it seems that using I would be curious to see if this makes any difference on linux (I am testing on a macbook pro) |
…s on the increments Signed-off-by: Marcello Seri <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like an improvement to me for sure. I'd like to run tests on Linux before merging it just to be quantify the win.
Signed-off-by: Marcello Seri <[email protected]>
If somebody can benchmark it on linux, can you also check what difference it does with and without cf7c62f ? |
I gave this a run on linux today. I did a set of different tests. I first tried running the tests through cohttp + conduit and took the example server from your gist. I didn't notice a big difference between the current released conduit and the version with accept_n. I then tried to use conduit for my own WIP http 1.1 server implementation and tried the following combinations: All servers run a service of the form
All of the data can be found at https://gist.github.com/anuragsoni/33ae4a750dd7e110cbe94ed69614b7d5 As an aside (and this is most likely off-topic for conduit and maybe more a discussion about how lwt scheduler works?), another potential data point was when i tried running a test that can simulate handlers that do a little bit of work. I added a sleep of 1 millisecond in the handler before it responds. With this change i noticed a big spike in latency and the numbers went up from milliseconds to seconds. https://gist.github.com/anuragsoni/33ae4a750dd7e110cbe94ed69614b7d5#file-h1_conduit_accept_n_1ms_delay-txt I've been running some benchmarks against servers in other languages and i don't see a similar spike when i add a millisecond delay in a handler for a tokio based rust web server, and I don't see a similar spike when I run my |
Dear @anuragsoni thanks a lot for this! The idea of adding the millisecond pause is brilliant. There are a number of tickets on this latency issue, if would be great if we can figure out how to improve/fix it. |
For sure! I remember seeing some tickets on cohttp and httpaf, are those the tickets you are referring to? I'm wondering if some of the latency related investigation/work will be needed at a layer below that? I've only recently started using Lwt seriously, but the fact that I'm seeing the large latency spike only on lwt and not on async leads me to believe that it'd be worthwhile to see if we can come up with a small reproduction case that's independent of cohttp/conduit etc to see if the issue lies somewhere in Lwt. When i tested the 1ms delay on async with my h1 library I saw a similar spike in latency when i was using the higher level Reader/Writer modules that do their own buffering and scheduling for writes. I noticed pretty significant improvements when i switched to using async's file_descr/socket apis directly to orchestrate my own read/writes and do my own buffering. I was hoping to see similar improvements on my lwt backend and I do see improvements when the handler doesn't do any work, but with the 1ms delay going down to read/write calls with Lwt_unix.file_descr's still shows spikes in latency that are similar to when I was testing with the Lwt_io managed buffered channels. Do note that everything I said might very well be wrong as I'm still exploring the lwt api and learning more about it :D |
What you see is reasonable and in line with some of the comments in the tickets I am referring to. Give me a day and I will link them here |
Here we go, the latency issue is discussed in
as well as httpaf#200 and eliom#270 -- ping also @smorimoto that made some tests there -- It can be mitigated a bit manipulating Conduit_lwt_unix.set_max_active 100;
Lwt_io.set_default_buffer_size 0x10000 and, as you observed, by using |
Signed-off-by: Marcello Seri <[email protected]>
There is a new benchmark showing how httpaf crushes cohttp on unilernels: robur-coop/unipi#4 (comment) |
Some more benchmarks comparing various http server implementations at https://github.com/ocaml-multicore/retro-httpaf-bench/ |
In benchmarks for cohttp-lwt-unix' latency we see evidence of connections starvation.
Using
accept_n
seems to improve the situation. You can see here an example of the artificial benchmarks that I used to check thisPing @avsm