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

feat(esp-alloc): Add heap usage stats #2137

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

AnthonyGrondin
Copy link
Contributor

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

This PR provides a way to easily see the current heap usage of esp_alloc. This adds HeapRegion::stats() and EspHeap::stats(), to return RegionStats and HeapStats respectively.

Both also implement core::fmt::Display to easily pretty-print the heap usage as following:

println!("{}", esp_alloc::get_info!());
HEAP INFO
Size: 117760
Current usage: 47280
Max usage: 70060
Total freed: 103012
Total allocated: 150292
Memory Layout: 
Internal | ██████████████░░░░░░░░░░░░░░░░░░░░░ | Used: 47280 / Total: 117760 (Free: 70480)
Unused   | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |
Unused   | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |

Testing

I have tested with a few examples locally and with esp-mbedtls, and everything works fine.
I haven't tested with PSRAM as I don't have any.

@bugadani
Copy link
Contributor

bugadani commented Sep 11, 2024

Fancy, but it should also support defmt, not everybody uses esp-println and making the user add Display2Format makes this somewhat less elegant.

@bjoernQ
Copy link
Contributor

bjoernQ commented Sep 11, 2024

Looks nice.

Given this runs a small amount of code every time allloc/dealloc is called would it make sense to have that feature gated?

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.

This looks lovely! I agree with @bjoernQ though that this should be an opt-in feature.

@okhsunrog
Copy link
Contributor

@bugadani won't this work with defmt?
defmt::!println!("{}", esp_alloc::get_info!());

@AnthonyGrondin
Copy link
Contributor Author

I was thinking about feature gating the whole get_info!() + HeapStats feature, but I realized that the extra computation in alloc/dealloc is only to keep track of internal heap stats. I added a new feature named internal-heap-stats (for the lack of better name), to cover this.

The trade-off is that we have a feature that will change the amount of fields returned by the struct, if a user calls stats(), but I think we do that in a few places already.

I also found a chip that has PSRAM, so I tested with using an internal heap and a PSRAM heap, and the output looks like the following:

HEAP INFO
Size: 2148352
Current usage: 512028
Max usage: 512028
Total freed: 0
Total allocated: 512028
Memory Layout: 
Internal | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ | Used: 28 / Total: 51200 (Free: 51172)
External | ████████░░░░░░░░░░░░░░░░░░░░░░░░░░░ | Used: 512000 / Total: 2097152 (Free: 1585152)
Unused   | ░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░░ |

@AnthonyGrondin
Copy link
Contributor Author

Fancy, but it should also support defmt, not everybody uses esp-println and making the user add Display2Format makes this somewhat less elegant.

I'm not used to defmt, can you tell what changes are required to make it work with defmt? The following was suggested by @okhsunrog in #2137 (comment):
defmt::!println!("{}", esp_alloc::get_info!());

The examples aren't set up to use defmt so I'll have to create a project that uses defmt.

… extra computation.

- Introduce the `internal-heap-stats` feature for `esp-alloc`.
- Add `esp_alloc::get_info!()` to `psram_quad` example to show usage and
ensure coverage of the feature in tests.
@bugadani
Copy link
Contributor

With the obvious syntax error, I believe the compiler would just complain about "HeapStats does not implement Format".

How you may do it.

This would also need to be feature gated, similar to how the rest of the crates have a demft feature. We can also do it later if you don't feel comfortable experimenting just for a different printing framework.

@okhsunrog
Copy link
Contributor

@bugadani yeah, that was a typo, sorry. For some reason I also thought that core Debug can also be used for defmt's println. Seems like it's not the case.. After stydying the docs a bit I think now I get it right

esp-alloc/src/macros.rs Outdated Show resolved Hide resolved
esp-alloc/src/lib.rs Show resolved Hide resolved
@AnthonyGrondin AnthonyGrondin changed the title feat(esp-alloc): Add heap usage stats and provide esp_alloc::get_info!() macro feat(esp-alloc): Add heap usage stats Sep 17, 2024
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@jessebraham jessebraham dismissed MabezDev’s stale review September 18, 2024 12:59

Comments have been addressed

#[cfg(feature = "defmt")]
impl defmt::Format for HeapStats {
fn format(&self, fmt: defmt::Formatter) {
defmt::write!(fmt, "{}", defmt::Display2Format(self))
Copy link
Member

Choose a reason for hiding this comment

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

Whilst convenient, this will end up pulling in core::fmt for defmt users, could you just duplicate the fmt impl using the defmt::write! macro instead?

@jessebraham jessebraham added this pull request to the merge queue Sep 18, 2024
@MabezDev MabezDev removed this pull request from the merge queue due to a manual request Sep 18, 2024
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.

Sorry, not quite, we should manually impl defmt to avoid pulling in core::fmt machinery for end users

@okhsunrog
Copy link
Contributor

@MabezDev maybe we can merge it as it is and I'll implement defmt correctly in a follow up PR?

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.

6 participants