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

Reduce the number of system calls for accept #769

Merged
merged 2 commits into from
Mar 5, 2020

Conversation

chambart
Copy link
Contributor

@chambart chambart commented Mar 5, 2020

Use accept4 instead of accept to avoid 2 fcntl system calls

accept4 takes flags to tell to create the file descriptor in non blocking mode. This way we don't need to set it later with Unix.set_nonblock.

Also, since we know that all the calls to accept will be on non blocking sockets we can avoid releasing the runtime lock.

accept4 accepts flags to tell to create the file descriptor in
non blocking mode. This way we don't need to set it later with
Unix.set_nonblock.

Also, since we know that all the calls to accept will be on
non blocking sockets we can avoid releasing the runtime lock.
@aantron
Copy link
Collaborator

aantron commented Mar 5, 2020

Thanks! I'll merge this shortly.

Just a comment:

Also, since we know that all the calls to accept will be on non blocking sockets we can avoid releasing the runtime lock.

The fd might be blocking, as a user can call Lwt_unix.set_blocking listen_fd true (though I know of no good reason to do so).

However, the current implementation in Lwt seems to assume that Unix.accept on a blocking listening socket will not block anyway, as Lwt will only call Unix.accept if the listening socket becomes readable (i.e., has a connection pending). I don't know if that's correct — perhaps the listening socket can become readable and then Unix.accept still blocks. However, if that's possible, this PR will be about equally as buggy as Lwt currently is here, so I think we can ignore this for now :)

@aantron aantron added this to the 5.2.0 milestone Mar 5, 2020
@aantron aantron merged commit f4557e8 into ocsigen:master Mar 5, 2020
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

2 participants