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

fix very slow poll_writable() #156

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

davibe
Copy link

@davibe davibe commented Oct 2, 2023

For context, I tried using async-io in a tokio application because I wanted to leverage udp_socket functionalities. My application suffered very poor performances sending data, so I investigated this.

I have found poll_writable() to be slow, from 0 to 1200ms, very randomly.

The problem is that at each poll_iteration the ticks are saved again. At the time of saving them the reactor has already updated the tick value. The next time the function is runs, the new tick is almost always within the last two saved. This is visible in the following log generated with these changes

...
delivering event dir: 1, tick: 88726
checking ticks dir: 1, cur: 88726, saved: 88726, 88725
saving ticks dir: 1, ticks: Some((88727, 88726))
delivering event dir: 1, tick: 88727
checking ticks dir: 1, cur: 88727, saved: 88727, 88726
saving ticks dir: 1, ticks: Some((88728, 88727))
delivering event dir: 1, tick: 88728
checking ticks dir: 1, cur: 88728, saved: 88728, 88727
saving ticks dir: 1, ticks: Some((88729, 88728))
delivering event dir: 1, tick: 88729
checking ticks dir: 1, cur: 88729, saved: 88729, 88728
saving ticks dir: 1, ticks: Some((88729, 88729))
delivering event dir: 1, tick: 88730
checking ticks dir: 1, cur: 88730, saved: 88729, 88729

15:13:31.966 INFO   socket send time 86 ms
...

The test I added would previously tick 1400 times on my machine before the future became Ready. With the fix It takes 2 or 3 ticks.

This logic has been re-implemented for async fn writable(), which is where I took the idea of the fix from. In that case infact, the ticks are not updated if they are already saved.

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me, but I'm afraid that this might break cases where two separate poll_read() calls are being made on the same Async<T>. Can you add some test cases where futures_lite::future::read is called twice in parallel on the same Async<T>?

@notgull
Copy link
Member

notgull commented Nov 18, 2023

@davibe Are you still interested in working on this PR?

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

Successfully merging this pull request may close these issues.

2 participants