esp-hal icon indicating copy to clipboard operation
esp-hal copied to clipboard

feat(esp-alloc): Add heap usage stats

Open AnthonyGrondin opened this issue 1 year ago • 13 comments

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 📝

  • [x] I have updated existing examples or added new ones (if applicable).
  • [x] I have used cargo xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • [x] My changes were added to the CHANGELOG.md in the proper section.
  • [x] I have added necessary changes to user code to the Migration Guide.
  • [x] 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.

AnthonyGrondin avatar Sep 10 '24 21:09 AnthonyGrondin

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

bugadani avatar Sep 11 '24 06:09 bugadani

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?

bjoernQ avatar Sep 11 '24 07:09 bjoernQ

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

okhsunrog avatar Sep 11 '24 19:09 okhsunrog

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 avatar Sep 12 '24 16:09 AnthonyGrondin

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 https://github.com/esp-rs/esp-hal/pull/2137#issuecomment-2344506476: 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.

AnthonyGrondin avatar Sep 12 '24 17:09 AnthonyGrondin

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.

bugadani avatar Sep 12 '24 17:09 bugadani

@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

okhsunrog avatar Sep 12 '24 18:09 okhsunrog

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

okhsunrog avatar Sep 19 '24 10:09 okhsunrog

@okhsunrog Could you prepare the PR based on Anthony's branch? Once both are ready I'll merge them.

MabezDev avatar Sep 20 '24 12:09 MabezDev

@MabezDev Sure, I'm create the PR tomorrow based on this branch

okhsunrog avatar Sep 20 '24 13:09 okhsunrog

I'm converting this to a draft for now, no rush with this of course. Please let us know if you'd like any help wrapping this up.

jessebraham avatar Oct 07 '24 11:10 jessebraham

I got a trauma in the end of September and had a surgery on my elbow. I still intend to work on this as soon as I recover

okhsunrog avatar Oct 07 '24 11:10 okhsunrog

Sorry to hear, get well soon :)

jessebraham avatar Oct 07 '24 13:10 jessebraham

Any update here? It would be nice to get this merged :).

MabezDev avatar Nov 01 '24 10:11 MabezDev

@MabezDev I'll create a new PR this weekend, Saturday or Sunday and finish the defmt part

okhsunrog avatar Nov 01 '24 11:11 okhsunrog

Sorry to keep pinging here, but I'd love to see this finished up :)

MabezDev avatar Nov 23 '24 14:11 MabezDev

Sorry, was busy at work. I've started working on this, I'll upload the PR tomorrow. I started with the fork of the current main branch, cherry-picked some commits, resolved a minor merge conflict (it was a comment for example :D), and now I'm adding the Format implementation on top of it. I've moved the formatting logic to a separate method and I'm using it in both Display and Format implementations. @MabezDev is it okay if I add a heapless dependency for heapless String?

okhsunrog avatar Nov 23 '24 15:11 okhsunrog

@MabezDev is it okay if I add a heapless dependency for heapless String?

I don't think we should be building strings for formatting. I think we can resolve this with a small amount of duplicate code which does the formatting in the same was a fmt::Debug impl, but with the defmt macros instead.

MabezDev avatar Nov 25 '24 15:11 MabezDev

Superceded by https://github.com/esp-rs/esp-hal/pull/2619, thanks @AnthonyGrondin and thanks @okhsunrog!

MabezDev avatar Nov 28 '24 13:11 MabezDev