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

Improve response times #413

Closed
wants to merge 1 commit into from
Closed

Improve response times #413

wants to merge 1 commit into from

Conversation

karlri
Copy link
Contributor

@karlri karlri commented Jan 12, 2024

When the network interface can't make any progress, it's probably a good idea to yeild the task so that the wifi task may run and proceed with actually sending/receiving data.

This seems to greatly improve response times, especially with low tick_rate_hz values. At 100Hz, ping time goes down from 25ms to <2ms. A simple http server test saw a response time reduction from 45ms to 6ms. At 1000Hz tick rate the gains are diminishing but there are some small gains. I ran my tests on esp32s3.

This might also increase throughput. I have not tested that. It also may have some unforseen adverse effects.

Ideally, we would have a more fine grained return value from interface.poll() than a bool so we have a better idea if yeilding is appropriate or not.

Perhaps with many sockets, yeilding like this is not a good idea.

When the network interface can't make any progress, it's probably
a good idea to yeild the task so that the wifi task may run and
proceed with actually sending/receiving data.

This seems to greatly improve response times, especially with low
tick_rate_hz values. At 100Hz, ping time goes down from 25ms to
<2ms. A simple http server test saw a response time reduction from
45ms to 6ms. At 1000Hz tick rate the gains are diminishing but
there are some small gains. I ran my tests on esp32s3.

This might also increase throughput. I have not tested that.
It also may have some unforseen adverse effects.

Ideally, we would have a more fine grained return value from
interface.poll() than a bool so we have a better idea if yeilding
is appropriate or not.
@MabezDev
Copy link
Member

MabezDev commented Jan 13, 2024

Interesting, thanks for the PR! I wonder if this change will also improve the throughput... 🤔. I will test it out with the bench example on Monday :).

edit: hehe I just read your description again and you already mentioned this 🤦

@karlri
Copy link
Contributor Author

karlri commented Jan 13, 2024

Also, I forgot to mention that it seems like the response time "relative variance" is decreased. No scientific measurements done.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 13, 2024

Nice idea! Totally makes sense

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 15, 2024

I don't see such a dramatic effect but it's definitely a nice improvement at least for non-async. (in my setup the pings w/o this are 14-16ms, with the changes they are 6-8ms)

@karlri
Copy link
Contributor Author

karlri commented Jan 15, 2024

@bjoernQ What esp product do you have?

This fix does not cover all cases. Notably async and coex would probably need some work. If results are promising we can look into that aswell.

I will submit my code that i used for testing in the evening. I tested on esp32s3. My test just accepted connection on tcp, sent a short reply, flushed and closed in a loop.

@bjoernQ
Copy link
Contributor

bjoernQ commented Jan 15, 2024

I tested on the original ESP32 since that one was connected to my system when I looked into it - haven't checked on any other chip yet. I tested with the dhcp example in this repo

@bjoernQ bjoernQ mentioned this pull request Feb 6, 2024
@bjoernQ
Copy link
Contributor

bjoernQ commented Feb 6, 2024

Thanks again for the great idea. I will close this in favor of #430 - I included the original commit there to make sure you contribution gets honored

@bjoernQ bjoernQ closed this Feb 6, 2024
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.

3 participants