Skip to content
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

add function read_blocking/write_blocking and alike (fixes also a dep… #101

Closed
wants to merge 4 commits into from

Conversation

craff
Copy link
Contributor

@craff craff commented Dec 19, 2022

I wanted to test with these as my project Simple_httpd uses non blocking sockets. I observed a 2-3% amelioration in very stressing condition (100 downloads simultaneous, using only 6 domains/cores)

So it may be worth a PR?

(fixes also a deprecation in a tests)

If you wish to accept this PR, I will add the necessary tests.

@craff
Copy link
Contributor Author

craff commented Dec 20, 2022

My latest commit fixes #102 : a bug in Ssl.flush with non blocking socket.
The return case of -1 which means retry should be handled.

@craff
Copy link
Contributor Author

craff commented Feb 14, 2023

I did some timing. On medium size (50Mb) size transfer, no releasing the lock is new to a factor 2 gain, because it saves releasing the lock. On simple_httpd, using domains, we go from
37 request per seconds to 57. This is for the whole process. So just timing Ssl.write and Ssl.read should indeed give a factor around 2.

@craff
Copy link
Contributor Author

craff commented Apr 6, 2023

Sorry, but I am about to release simple_httpd. I do not want to fork ocaml-ssl (I think this is rude and bad for the comunity). In the meantime, I add ocaml-ssl with this PR and version updated to 0.6 as a sub module. If we converge to an accepted PR, I will remove this submodule.

…recation in a tests). This function

are grouped in two module: Release with the original behavious and the new ones in NoRelease
@craff
Copy link
Contributor Author

craff commented Apr 6, 2023

I used rebase to cleanup the commit list for easier review.

@toots
Copy link
Member

toots commented Apr 8, 2023

This PR looks fine to me on principle and first pass and, if the performances you mention are real, would be warranted. We passed control of this module over to @anmonteiro and co. so I believe it would be up to them to accept the patch if they want to. Consider this as my emeritus maintainer approval

@anmonteiro
Copy link
Collaborator

anmonteiro commented Apr 8, 2023

This PR has seemingly grown in scope since it was first opened.

@craff would you mind reverting the opam file change, and submitting the deprecations in a separate PR?

@craff
Copy link
Contributor Author

craff commented Apr 9, 2023 via email

@craff craff closed this Apr 9, 2023
@craff
Copy link
Contributor Author

craff commented Apr 9, 2023

I close this PR as it is replaced by the two others.

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.

None yet

3 participants