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

Reduce stack-allocations #243

Merged
merged 3 commits into from
Aug 22, 2023
Merged

Reduce stack-allocations #243

merged 3 commits into from
Aug 22, 2023

Conversation

bjoernQ
Copy link
Contributor

@bjoernQ bjoernQ commented Aug 21, 2023

Currently there are a lot of stack allocations especially here:

Code  Stack Name
 3980  6992 main
  904  4608 <esp_wifi::wifi::WifiTxToken as smoltcp::phy::TxToken>::consume::h03e28a51459eb5a7
  612  4592 esp_wifi::wifi::recv_cb::h76250891538a1884
  700  4592 <esp_wifi::wifi::WifiTxToken as smoltcp::phy::TxToken>::consume::h07b10c4b31c2e16a
  752  4592 <esp_wifi::wifi::WifiTxToken as smoltcp::phy::TxToken>::consume::h1693af2449f9090a
  368  3232 <esp_wifi::wifi::WifiRxToken as smoltcp::phy::RxToken>::consume::hf90923587131582a
  784  3152 esp_wifi::wifi::send_data_if_needed::h08e5ef596584e917
  196  1552 heapless::spsc::Queue<T,_>::inner_dequeue::h6ca259dae19e1234
  196  1552 heapless::spsc::Queue<T,_>::inner_dequeue::h7ab73b821e96b945

I first tried to use heapless::Pool - that helped a lot but not as much as I wanted.

This PR now uses statically allocated memory backing the DataFrame

With that we get to e.g.

  598    64 esp_wifi::wifi::recv_cb::h76250891538a1884

Which looks quite okay to me.

MabezDev
MabezDev requested changes Aug 21, 2023
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.

Weird that heapless::Pool didn't help that much 🤔. This impl looks good to me though, just a few comments :)

@AnthonyGrondin
Copy link
Contributor

Tested locally in a current project. The highlighted code section isn't present after 6b53683 -> 04e9d2c So that's a very optimized

Code  Stack Name
12746 70736 embassy_executor::raw::TaskStorage<F>::poll::hb6d055c6609618a1
//// START This section gets optimized
  730  4608 esp_wifi::wifi::embassy::<impl embassy_net_driver::TxToken for esp_wifi::wifi::WifiTxToken>::consume::ha2f8c883678dc32d 
  735  4608 <esp_wifi::wifi::WifiTxToken as smoltcp::phy::TxToken>::consume::h6503c28ba30dd642 
  513  4592 esp_wifi::wifi::embassy::<impl embassy_net_driver::TxToken for esp_wifi::wifi::WifiTxToken>::consume::h17cae9bf793f0a83 
  512  4592 esp_wifi::wifi::embassy::<impl embassy_net_driver::TxToken for esp_wifi::wifi::WifiTxToken>::consume::hacd316ed4935b9d3 
  439  4592 esp_wifi::wifi::recv_cb::h0b1c0a30febc48dc 
  517  4592 <esp_wifi::wifi::WifiTxToken as smoltcp::phy::TxToken>::consume::h008af2dce9b78f58 
  518  4592 <esp_wifi::wifi::WifiTxToken as smoltcp::phy::TxToken>::consume::h5ca1eccd1b3d4e47 
  720  3264 esp_wifi::wifi::embassy::<impl embassy_net_driver::RxToken for esp_wifi::wifi::WifiRxToken>::consume::h5746f09a478ceae8 
  708  3264 <esp_wifi::wifi::WifiRxToken as smoltcp::phy::RxToken>::consume::h1d3e212500ef1969 
  588  3136 esp_wifi::wifi::send_data_if_needed::h58c85effdf579114 
/// END

MabezDev
MabezDev approved these changes Aug 22, 2023
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!

@bjoernQ bjoernQ merged commit b8bddff into main Aug 22, 2023
@bjoernQ bjoernQ deleted the use-pool branch August 22, 2023 09:44
bugadani
bugadani reviewed Sep 2, 2023
View reviewed changes
esp-wifi/src/wifi/mod.rs
@@ -97,31 +98,72 @@ impl WifiMode {
}
}

const DATA_FRAMES_MAX_COUNT: usize = RX_QUEUE_SIZE + RX_QUEUE_SIZE;
Copy link
Contributor

Choose a reason for hiding this comment

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

@bjoernQ Is this a typo (RX_QUEUE_SIZE + TX_QUEUE_SIZE) or is this intentionally 2x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks like a typo, true. should be RX + TX queue size 👍

bugadani
bugadani reviewed Sep 4, 2023
View reviewed changes
esp-wifi/src/wifi/mod.rs
let packet = if let Some(packet) = DataFrame::from_bytes(src) {
packet
} else {
return esp_wifi_sys::include::ESP_ERR_NO_MEM as esp_err_t;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this return may leak memory because esp_wifi_internal_free_rx_buffer(eb); is not called.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true - we should call esp_wifi_internal_free_rx_buffer(eb); here 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

before you do anything, I made a PR this morning that includes this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bugadani bugadani bugadani left review comments

@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.
4 participants
@bjoernQ @AnthonyGrondin @bugadani @MabezDev