Skip to content

Switch to Cohttp#670

Merged
jrochel merged 5 commits into
masterfrom
cohttp-jv
May 17, 2021
Merged

Switch to Cohttp#670
jrochel merged 5 commits into
masterfrom
cohttp-jv

Conversation

@vouillon
Copy link
Copy Markdown
Member

No description provided.

@smorimoto
Copy link
Copy Markdown
Member

Issue mirage/ocaml-cohttp#328 still remains in cohttp, and unless we fix this first, I think there is a problem as a framework used in enterprise applications. There is also httpaf as an alternative, but it still doesn't support SSL/TLS, so fixing cohttp seems easier and smarter.

@mseri
Copy link
Copy Markdown

mseri commented Apr 19, 2021

Do you know what is the latency difference between the version pre and post port to cohttp? If you have time, can you check if tweaking the values below before creating the server improves the situation?

Conduit_lwt_unix.set_max_active 256;
Lwt_io.set_default_buffer_size 0x10000;

@smorimoto
Copy link
Copy Markdown
Member

@mseri I will try it later. However, it's already confirmed that the problem can be reproduced with the latest cohttp, so the comparison between the patched version and the original version will be still meaningful.

@mseri
Copy link
Copy Markdown

mseri commented Apr 20, 2021

it's already confirmed that the problem can be reproduced with the latest cohttp

Oh sure, my question was about comparing the current eliom, eliom with this port to cohttp, and eliom with this port to cohttp with the additional limits on max active connections (256 is quite low, probably 1024 is more realistic) and a different Lwt_io buffer size (the default is 0x1000, just 4k).

@mseri
Copy link
Copy Markdown

mseri commented Apr 20, 2021

I will try it later.

Thanks a lot!

@smorimoto
Copy link
Copy Markdown
Member

smorimoto commented Apr 20, 2021

Oh, I see. Then it seems better to compare the old ocsigenserver and cohttp.

@jrochel
Copy link
Copy Markdown

jrochel commented May 17, 2021

@smorimoto According to @vouillon the performance issues as per mirage/ocaml-cohttp#328 are not a blocking issue, so I'm merging.

@jrochel jrochel merged commit 41be5a6 into master May 17, 2021
@jrochel jrochel deleted the cohttp-jv branch May 17, 2021 14:42
@smorimoto
Copy link
Copy Markdown
Member

I see!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants