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

Reconsider supported draining behaviour #1611

Closed
Thomasdezeeuw opened this issue Aug 17, 2022 · 4 comments
Closed

Reconsider supported draining behaviour #1611

Thomasdezeeuw opened this issue Aug 17, 2022 · 4 comments
Milestone

Comments

@Thomasdezeeuw
Copy link
Collaborator

For Unix (kqueue and epoll) to drain readiness all you have to do is read/write until you get a "short" read/write, i.e. less bytes are processed than your buffer size. However Mio doesn't guarantee that another event is return until the I/O operations hits a WouldBlock error. This means that to strictly follow Mio's docs you'll have to issue another pointless system call (read/write/etc.).

This is because on Windows Mio could only guarantee the readiness to be drained once a WouldBlock error was returned. However nowadays we control that ourselves in IoSourceState::do_io. So, we could change it to reregister when e.g. the read bytes < buffer size.

Also find out if all of this is true for poll (see #1602).

@Thomasdezeeuw Thomasdezeeuw changed the title Reconsider support draining behaviour Reconsider supported draining behaviour Aug 17, 2022
@Janrupf
Copy link

Janrupf commented Aug 17, 2022

Also find out if all of this is true for poll (see #1602).

If I understand correctly, it is. poll is level-based by default anyway, so we have full control over when we want to re-arm an edge trigger. #1602 currently does not implement this, but since for poll this solves other issues as well, I'm working on implementing edge-based triggers using IoSourceState.

@Noah-Kennedy
Copy link

Noah-Kennedy commented Aug 17, 2022

This can be a pretty impactful optimization in certain workloads.

@Thomasdezeeuw
Copy link
Collaborator Author

One thing we need to consider are the try_io functions on various types, e.g. TcpStream::try_io, which currently don't give us access to the amount of bytes read.

@Thomasdezeeuw
Copy link
Collaborator Author

I don't think we can change the behaviour any more. The public try_io method doesn't have access to the internal success type of the function it's wrapping, nor could it know if e.g. a read was short. Furthermore I don't this working with the poll(2) implementation.

Based on that I'm going to close this and keep the current behaviour.

If someone does have an idea on how to support try_io and the poll(2) we can reopen this.

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

No branches or pull requests

3 participants