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.

Sign up for GitHub

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

Jump to bottom

Performance improvement #430

Merged
merged 5 commits into from
Feb 8, 2024
Merged

Performance improvement #430

merged 5 commits into from
Feb 8, 2024

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Feb 6, 2024

This is based on the idea in #413 but this also works for async since the decision to yield from the active task is done at a lower level. To attribute the user who came up with the original idea I included that commit.

Here are some figures using ESP32-C6 with this cfg.toml

[esp-wifi]
rx_queue_size = 32
tx_queue_size = 3
static_rx_buf_num = 16
dynamic_rx_buf_num = 32
ampdu_rx_enable = 0
ampdu_tx_enable = 0
rx_ba_win = 32
max_burst_size = 6
heap_size = 86000

Using embassy_bench.rs I got these results (with a lot of RF noise, connected the ESP32-C6 to my laptop's hotspot)

original
Testing download...
connecting to Address([192, 168, 137, 1]):4321...
connected, testing...
download: 410 kB/s
Testing upload...
connecting to Address([192, 168, 137, 1]):4322...
connected, testing...
upload: 210 kB/s
Testing upload+download...
connecting to Address([192, 168, 137, 1]):4323...
connected, testing...
upload+download: 307 kB/s

Reply from 192.168.137.143: Bytes=32 Time=31ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=33ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=34ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=33ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=54ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=40ms TTL=64


with these changes
connecting to Address([192, 168, 137, 1]):4321...
connected, testing...
download: 1046 kB/s
Testing upload...
connecting to Address([192, 168, 137, 1]):4322...
connected, testing...
upload: 1823 kB/s
Testing upload+download...
connecting to Address([192, 168, 137, 1]):4323...
connected, testing...
upload+download: 1154 kB/s


Reply from 192.168.137.143: Bytes=32 Time=1ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=3ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=2ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=1ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=4ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=2ms TTL=64
Reply from 192.168.137.143: Bytes=32 Time=5ms TTL=64

This also fixes the embassy_bench.rs exampe.

karlri and others added 5 commits February 5, 2024 17:28
@karlri @bjoernQ
Improve response times
77c437a 
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.
@bjoernQ
Revert "Improve response times"
e00d318 
This reverts commit 77c437a.
@bjoernQ
Fix embassy_bench
1084673
@bjoernQ
Yield from tx_token/rx_token if no progress can be made otherwise
db1065b
@bjoernQ
CHANGELOG.md entry
8fc04ad
@bjoernQ bjoernQ mentioned this pull request Feb 6, 2024
Closed
@karlri
Copy link
Contributor

karlri commented Feb 6, 2024

This looks great! Gonna definitely try async when i get home from vacation next week :)

@MabezDev
Copy link
Member

MabezDev commented Feb 6, 2024

Woah! This is a huge improvment!

image

I'm getting 2MB/s upload in also pretty congested network! Thanks for the idea @karlri!

MabezDev
MabezDev approved these changes Feb 7, 2024
View reviewed changes
Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

LGTM!

@karlri
Copy link
Contributor

karlri commented Feb 8, 2024

With these changes, what would happen if the user tried to call this code from the other CPU core on dual-core models? I guess that use case was never officially supported but still:

We would not want to task switch away from the wifi driver and we wouldnt want to interrupt the wrong core or something I guess.

@bjoernQ
Copy link
Contributor Author

bjoernQ commented Feb 8, 2024

With these changes, what would happen if the user tried to call this code from the other CPU core on dual-core models? I guess that use case was never officially supported but still:

We would not want to task switch away from the wifi driver and we wouldnt want to interrupt the wrong core or something I guess.

The scheduler doesn't support running some tasks on different cores currently and most probably never will. In theory we can run everything on the second core (needs some adjustments currently) but nothing changes then - it's just a different core. The interrupt is asserted on the same core in that case

@bjoernQ bjoernQ merged commit e6c58b6 into main Feb 8, 2024
9 checks passed
@bjoernQ bjoernQ deleted the performance-improvement branch February 8, 2024 08:05
@MabezDev MabezDev mentioned this pull request Feb 8, 2024
Open
@bjoernQ bjoernQ mentioned this pull request Sep 16, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MabezDev MabezDev MabezDev approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bjoernQ @karlri @MabezDev