-
Notifications
You must be signed in to change notification settings - Fork 174
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
Use new request parser #819
Conversation
If I understand correctly, the slow down is because Lwt_io allocates an additional bigarray? |
So what was the limit for Httpaf here for reference? In other words, how much further can it push past 100k with decent latency |
http-parser/src/http_parser.mli
Outdated
?pos:int -> ?len:int -> string -> (Http.Request.t * int, error) result | ||
|
||
val parse_chunk_length : | ||
?pos:int -> ?len:int -> string -> (int * int, error) result |
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 have used int
for Chunk lengths as the initial work that led to this parser made an assumption that we'll always run on 64bit systems. With that assumption the int
would be enough to represent the largest chunk size that's allowed by the parser.
http-parser/src/http_parser.mli
Outdated
val parse_chunk : | ||
?pos:int -> | ||
?len:int -> | ||
string -> | ||
chunk_kind -> | ||
(chunk_parser_result * int, error) result |
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.
My intention was to not have to use parse_chunk_length
manually, and instead go via the parse_chunk
which helps orchestrate threading through the chunk body state
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'll also note that I don't make use of any of the chunked parsing code in this PR and I'm limiting this diff to just the request (upto the headers) parsing for now
@@ -0,0 +1,24 @@ | |||
open Lwt.Infix | |||
|
|||
type t = { buf : Bytebuffer.t; chan : Lwt_io.input_channel } |
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.
Adding a buffer layer alongside Lwt_io
as it helps in driving the core IO layer that needs access to the internal view within the buffer to feed data to the new parser.
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.
Before this is considered ready, we might want a way to grow the buffer too I suppose (with an upper bound beyond which its considered an error maybe?)
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.
Could you expand on this a bit more. When does the buffer need to grow?
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 think it needs to grow as such as long we pick a value that we feel is reasonable to parse the request + headers. But I noticed that the body decoding implementation in cohttp references that it'd like to read up to a max of 32k bytes at a time when reading chunked bodies
ocaml-cohttp/cohttp/src/transfer_io.ml
Line 52 in efe0599
(* read between 0 and 32Kbytes of a chunk *) |
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.
32K sounds like a reasonable maximum. I suppose we should determine the default buffer size experimentally and let the user customize it if need be.
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.
For reference, I went back through the history to find the PR that first introduced the limit: #330 Looks like at that stage it was arbitrary. I think it would be nice to make it configurable, something similar to Lwt_io.set_default_buffer_size io_buf
Is the intention to make http-parser a public API? If that's the case, a package is appropriate. But if it's not, we need to find a new place for it to live (either in http or cohttp). |
On my laptop its able to go up to 125k with decent latency. With a run of 150k rps it degraded into average latency 2.4 seconds |
I am not fully sure about this. I think I'd be fine with both a public package, or something that lives within either |
examples/bench/lwt_unix_server.ml
Outdated
@@ -0,0 +1,43 @@ | |||
open Cohttp_lwt_unix | |||
|
|||
let text = |
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 is the lwt server I'm using to benchmark cohttp
Do you think the current API is stable? If not, then we should probably include it in Http under a Private module. Once it stabilizes, we can promote it into a separate package, library, etc. |
cohttp-async/src/io.ml
Outdated
@@ -102,6 +102,8 @@ module IO = struct | |||
Writer.write oc buf; | |||
return ()) | |||
|
|||
let refill _ = failwith "Not implemented yet" | |||
let with_input_buffer _ = failwith "not implemented yet" |
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'm thinking about how to implement this, and I think it would actually make things easier if Byte_buffer
was Bigstring based or at least functorized on the buffer type. Reason being is that I don't see how to implement these functions in an efficient manner for Async's Reader for example. I'd imagine we want to use read_one_chunk_at_a_time
to implement with_input_buffer
. I understand how bigstrings don't are a wash performance wise, but it helps keep compatibility with existing implementations. Perhaps we could functorize Byte_buffer to make it possible to continue experimenting in Shuttle/Fiber without normal Bytes.
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 agree but if we want to keep it simple we could check how big of a performace hit would be to just use bigstring
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.
Moving to bigstrings will also mean moving the parser to bigstrings. Otherwise we pay the price of allocating more strings?
I was thinking of implementing the async backend in a similar manner as the lwt where there is a byte buffer paired with the reader.
That said, now that I think about it there is going to be some breaking changes in the sense that people using the expert response in servers will need to adapt to the switch to the new input type in the IO backends.
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.
Moving to bigstrings will also mean moving the parser to bigstrings. Otherwise we pay the price of allocating more strings?
Yes but that is very localized to the Source
module, no? In any case it was just a curiosity, and as you mentioned you also already have an idea to make this a non-issue
That said, now that I think about it there is going to be some breaking changes in the sense that people using the expert response in servers will need to adapt to the switch to the new input type in the IO backends.
That is the last of the problems. We are allowed to break things in cohttp now, as long as we can justify it. Imho the speedup and latency numbers shown here would justify the breakage
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.
string
is the way to go in OCaml 5.0, since it's both non-moving and allocated via malloc (all large ocaml objects are mallocated in multicore)
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.
We can probably deal with this in another manner by pushing the buffer to the core cohttp
library, and then the refill becomes a way for the IO layers to fill the buffer managed by cohttp. That will let us keep the IO types for the IO layers the same as they are today, while still benefiting from the buffering we need to drive the parser.
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'm not a huge fan of moving a buffer to the core library as that removes one benefit for libraries like shuttle that could otherwise drive the whole cohttp server machinery with just the 1 buffer that's used by its channel implementation.
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.
That will let us keep the IO types for the IO layers the same as they are today, while still benefiting from the buffering we need to drive the parser.
Note that we don't necessarily need to retain the same IO types. For example, if we could just create Reader.t/Writer.t on the fly when the user provides Expert that would be good enough. Though I'm not sure if it's at all possible.
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'm not a huge fan of moving a buffer to the core library as that removes one benefit for libraries like shuttle that could otherwise drive the whole cohttp server machinery with just the 1 buffer that's used by its channel implementation.
Yeah, it's not worth it. If we must break the current api and make Reader/Writer impossible to use, then so be it.
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 we could just create Reader.t/Writer.t on the fly when the user provides Expert that would be good enough.
This should be doable. There will be some awkwardness for sure, but we can definitely get something working. We only need to recreate the reader since I haven't switched the writer in the existing implementation. pseudo code for how I'd do this in async
let reader_of_in_channel t =
let unconsumed_payload = Bytebuffer.unconsumed_data t.buf in
let reader_pipe = Reader.pipe t.reader in
Unix.pipe ~info:(Info.create ...)
>>| fun (`Reader rd, `Writer wr) ->
let new_reader = Reader.create rd in
let writer = Writer.create wr in
Writer.write writer unconsumed_payload;
don't_wait_for (
Writer.transfer writer reader_pipe ~stop:(Reader.close_finished new_reader) (fun s -> Writer.write writer s)
>>= fun () -> Writer.close writer );
new_reader
cohttp-async/src/bytebuffer.ml
Outdated
@@ -0,0 +1,87 @@ | |||
open Core |
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 need to think about how to remove some of this duplication as this is very similar to the module proposed for the lwt backend
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.
What are the differences preventing you from making this generic? It seems like you just need a function over a monad + reader.
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.
no difference. This can be moved to something that's instantiated using the IO functor (like we do for other modules like request/response)
I pushed an async update as well. But unlike the lwt backend I see no difference in perf between the old implementation and the new implementation 😞 I'll spend more time on this to try and understand what's happening. |
I reworked the async server loop and now I can see improvement when compared to the existing cohttp implementation. With the older cohttp-async server when I attempt to target 80_000 RPS with 1000 active clients I have an average latency of 8 seconds with tails of 12 seconds. With the new implementation the average latency drops to 2.3 seconds with tails of 3.5 seconds. This is still quite a way behind the lwt implementation. That said, I think on the async backend the current limiting factor might be the conduit setup. I wrote another async backend for cohttp using shuttle https://github.com/anuragsoni/shuttle/blob/b296ac77f590735ea56d94ef19ae503251dcd317/shuttle_cohttp/shuttle_cohttp.ml and with that backend the average latency when serving 80_000 RPS is 6.9ms with tails of 16ms . These numbers are closer to what the new lwt backend gets here. All of the async tests were run on macOS where it uses the select backend (the lwt tests were with libev so it probably picks up kqueue on macOS?), and the async backend might perform a little better when run on linux where it has access to epoll. |
I think you can use the libuv backend now on macos. It is experimental but in my tests it has been working pretty reliably |
I will try that! That said the libev backend for lwt seems to be working well so I'm not expecting much difference between that and libuv since the current state of |
http/src/http.ml
Outdated
Source.advance source 1; | ||
stop := true) | ||
else ( | ||
stop := true; |
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 think we can make stop := true
outside of the if
.
@rgrinberg @mseri I should've asked this before I rebased. How would you like me to sync with new changes from the main branch? I'm happy to rebase but if merge commits will make it easier to review new changes I can switch to that for new updates. |
Rebase as much as you want.
Rudi.
…On Jan 7, 2022, 2:10 PM -0700, Anurag Soni ***@***.***>, wrote:
@rgrinberg @mseri I should've asked this before I rebased. How would you like me to sync with new changes from the main branch? I'm happy to rebase but if merge commits will make it easier to review new changes I can switch to that for new updates.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
I agree, go wild |
Is this expected?
I think it happens when a consumer reads from a channel that has already been exhausted or that has been closed for other reasons (timeouts?) |
When expert mode was introduced this was handled by fee808a (See the commentary on #647 (comment)) |
The CI failure on |
Do you think it's acceptable to change to int64? |
Yes. Using int64 for chunk length should be fine. |
I factored out the buffer implementation from cohttp-lwt-unix, and I now have the mirage backend compiling with the new parser implementation. I believe that with this last commit, unless I've missed it, I don't have anything else that's still commented/left unfinished, so I guess this PR can be considered not a draft. The mirage backend is the piece I'm least confident about as I don't have much experience with it, and I don't see any existing tests for it within the repository. For the Async and lwt-unix backend I think I have an idea about how we can still keep the same public api by ensuring that the callback needed for the |
* Hand written http request/response parser * Far more efficient IO layer
Thanks for rewriting half of cohttp! I'll merge it now and we can solve the remaining issues in separate PR's. @samoht @dinosaure @hannesm could you have a look/optimize the mirage backend? The mirage should also be able to reap the performance benefits. |
I did a quick test with cohttp mirage today. I used the macOS backend as I don't have an easy access to a linux machine at the moment.
wrk bench against cohttp-mirage 5.0.0
wrk bench against cohttp-mirage pinned to commit 06d2127 (the new parser)
|
* We only have a new parser for requests for now * We only touched the read path with mirage#819
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)
…p-mirage, cohttp-lwt, cohttp-lwt-unix, cohttp-lwt-jsoo, cohttp-eio, cohttp-curl, cohttp-curl-lwt, cohttp-curl-async, cohttp-bench and cohttp-async (6.0.0~alpha0) CHANGES: - cohttp-eio: ensure "Host" header is the first header in http client requests (bikallem mirage/ocaml-cohttp#939) - cohttp-eio: add TE header in client. Check TE header is server (bikallem mirage/ocaml-cohttp#941) - cohttp-eio: add User-Agent header to request from Client (bikallem mirage/ocaml-cohttp#940) - cohttp-eio: add Content-Length header to request/response (bikallem mirage/ocaml-cohttp#929) - cohttp-eio: add cohttp-eio client api - Cohttp_eio.Client (bikallem mirage/ocaml-cohttp#879) - http: add requires_content_length function for requests and responses (bikallem mirage/ocaml-cohttp#879) - cohttp-eio: use Eio.Buf_write and improve server API (talex5 mirage/ocaml-cohttp#887) - cohttp-eio: update to Eio 0.3 (talex5 mirage/ocaml-cohttp#886) - cohttp-eio: convert to Eio.Buf_read (talex5 mirage/ocaml-cohttp#882) - cohttp lwt client: Connection cache and explicit pipelining (madroach mirage/ocaml-cohttp#853) - http: add Http.Request.make and simplify Http.Response.make (bikallem mseri mirage/ocaml-cohttp#878) - http: add pretty printer functions (bikallem mirage/ocaml-cohttp#880) - New eio based client and server on top of the http library (bikallem mirage/ocaml-cohttp#857) - New curl based clients (rgrinberg mirage/ocaml-cohttp#813) + cohttp-curl-lwt for an Lwt backend + cohttp-curl-async for an Async backend - Completely new Parsing layers for servers (anuragsoni mirage/ocaml-cohttp#819) + Cohttp now uses an optimized parser for requests. + The new parser produces much less temporary buffers during read operations in servers. - Faster header comparison (gasche mirage/ocaml-cohttp#818) - Introduce http package containing common signatures and structures useful for compatibility with cohttp - and no dependencies (rgrinberg mirage/ocaml-cohttp#812) - async(server): allow reading number of active connections (anuragsoni mirage/ocaml-cohttp#809) - Various internal refactors (rgrinberg, mseri, mirage/ocaml-cohttp#802, mirage/ocaml-cohttp#812, mirage/ocaml-cohttp#820, mirage/ocaml-cohttp#800, mirage/ocaml-cohttp#799, mirage/ocaml-cohttp#797) - http (all cohttp server backends): Consider the connection header in response in addition to the request when deciding on whether to keep a connection alive (anuragsoni, mirage/ocaml-cohttp#843) + The user provided Response can contain a connection header. That header will also be considered in addition to the connection header in requests when deciding whether to use keep-alive. This allows a handler to decide to close a connection even if the client requested a keep-alive in the request. - async(server): allow creating a server without using conduit (anuragsoni mirage/ocaml-cohttp#839) + Add `Cohttp_async.Server.Expert.create` and `Cohttp_async.Server.Expert.create_with_response_action`that can be used to create a server without going through Conduit. This allows creating an async TCP server using the Tcp module from `Async_unix` and lets the user have more control over how the `Reader.t` and `Writer.t` are created. - http(header): faster `to_lines` and `to_frames` implementation (mseri mirage/ocaml-cohttp#847) - cohttp(cookies): use case-insensitive comparison to check for `set-cookies` (mseri mirage/ocaml-cohttp#858) - New lwt based server implementation: cohttp-server-lwt-unix + This new implementation does not depend on conduit and has a simpler and more flexible API - async: Adapt cohttp-curl-async to work with core_unix. - *Breaking changes* + refactor: move opam metadata to dune-project (rgrinberg mirage/ocaml-cohttp#811) + refactor: deprecate Cohttp_async.Io (rgrinberg mirage/ocaml-cohttp#807) + fix: move more internals to Private (rgrinberg mirage/ocaml-cohttp#806) + fix: deprecate transfer encoding field (rgrinberg mirage/ocaml-cohttp#805) + refactor: deprecate Cohttp_async.Body_raw (rgrinberg mirage/ocaml-cohttp#804) + fix: deprecate more aliases (rgrinberg mirage/ocaml-cohttp#803) + refactor: deprecate connection value(rgrinberg mirage/ocaml-cohttp#798) + refactor: deprecate using attributes (rgrinberg mirage/ocaml-cohttp#796) + cleanup: remove cohttp-{curl,server}-async (rgrinberg mirage/ocaml-cohttp#904) + cleanup: remove cohttp-{curl,server,proxy}-lwt (rgrinberg mirage/ocaml-cohttp#904) + fix: all parsers now follow the spec and require `\r\n` endings. Previously, the `\r` was optional. (rgrinberg, mirage/ocaml-cohttp#921) - `cohttp-lwt-jsoo`: do not instantiate `XMLHttpRequest` object on boot (mefyl mirage/ocaml-cohttp#922)
Hello,
I'll preface this by saying that this is far from being ready, and I've had to comment/break a lot of things to get a minimal server example working.
Background
I've been working on a HTTP request (and chunked body) parser and prototype for a HTTP 1.1 server and I decided to use the Cohttp types as the base layer for my explorations. The server experiments in
shuttle
have shown promise, but instead of adding yet another server library I'd like to explore if it's possible to integrate some of that work directly into Cohttp. I'm opening this really early in the process to see if an approach similar to what I'm proposing here will fit well with the Cohttp maintainer's vision (and to hear if there are suggestions on improving the IO layer even further).I am starting with
cohttp-lwt-unix
first as that's the cohttp backend I use at work, and I wanted to try my work on a non async backend to validate that the work inshuttle
can be ported to non async libraries as well. All of the new code in this diff is a direct port of implementations from theshuttle
library.I am using the test payload referenced in #328 to measure progress.
Disclaimer: All of these test runs were made on my laptop, and if someone has a better hardware setup for such benchmarking then I'd appreciate getting some more measurements from there.
Library Versions: Cohttp (the version in the current trunk as of January 6 2022), LWT (5.5.0), OCaml (4.13.1)
I ran an initial set of tests using
wrk2
using 1000 active connections and two different request rates (80_000, and 100_000). The results from the runs on my laptop can be seen at https://gist.github.com/anuragsoni/eb095a7f4c3aec1f4ba65b9866d9f9d3 . The summary from that gist is:Lwt_unix.file_descr
paired with the buffer that's part of this PR. With that the server was able to handle 100_000. RPS with average latencies of 2ms with tail latencies of 7.6ms - link hereApologies for wall of text, but I wanted to start a discussion even if I only have a partially working solution so far, as I'd like to get some insights into whether the proposed implementation here makes sense as a starting point, and I'm also hoping to learn how best to test/benchmark such work so we can have an accurate picture of any potential improvements.
Thanks,
Anurag