[Heap Trace Standalone] improvements to formatting, code, comments (IDFGH-8699)

[chipweinberger]

Original PR: #10127

Explicitly allow SPI ram for standalone heap tracing, and improve its usage.

• adds total_isr_skipped, which gets incremented if psram records buffer is locked
• adds heap_trace_dump_internal_ram(). I usually run out of internal ram, and only want to see internal ram.
• adds heap_trace_dump_psram(), for completeness
• adds heap_trace_summary() so that users can access alll heap tracing information.
• refactor some global variables into a records_t struct, for clarity
• adds high_water_mark to the records buffer, to make it easier for users to choose a buffer size
• improve formatting of heap_trace_dump for clarity, and print "PSRAM" v.s. "Internal" for each allocation
• fix bug where total_frees would not get incremented when records count is 0

[chipweinberger]

@igrr Would love to get all of these changes merged. (pending review)

I have a separate PR to ~1000x the perf of standalone Leaks mode, which I'd like to rebase. With the perf change (doubly linked list & simple hash table), standalone leaks mode is fast enough to be enabled 24/7 during development, so all living memory allocations can be printed when low ram is hit.

Very useful!

[chipweinberger]

@igrr, bump. It's a pretty simple change, despite the largish-diff.

[chipweinberger]

@SoucheSouche, I noticed you are the reviewer on my other Heap Trace PR.

Please let me know your thoughts / if there is anything you'd like me to change.

[SoucheSouche]

Hi @chipweinberger, I will have a look at your PR very shortly. Sorry for not answering earlier.

[SoucheSouche]

Hi @chipweinberger, thank you for this contribution! I had a look at your PR and everything looks great. However, I have a few notes.

1. I saw that you introduced a dependency to spi_flash in the heap component which I have to strongly recommend against as it means that we won't be able to run the heap component on new chips until we support the spi_flash component on said new chip.

2. I locally rebased your branch to be in line with upstream and realized when setting CONFIG_HEAP_TRACING_STANDALONE to true that your code was not compiling. It turns out that spi_flash_cache_enabled() is declared in esp_private/cache_utils.h and can't be accessed by including esp_spi_flash.h (which I assume was your initial intention here). Of course this issue won't exist when the dependency to spi_flash will be removed.

The points above lead to the removal of the total_isr_skipped tracking most likely so let me know what you think about it.

[chipweinberger]

Thanks for the review.

I think we could keep total_isr_skipped, but put it behind a default: true KConfig. This is beneficial to most users. Without spi_flash, ISR allocations cause crashes, especially for 3rd party components.

During development of new chips, breifly disable the KConfig, until spi_flash is supported. I would need to learn how to read KConfig options in CMAKE. Have we done anything like this before?

Or perhaps, we could move heap_trace to its own component?

All of this being said, I will remove total_isr_skipped for now, and open a new commit / PR to address it later.

[SoucheSouche]

Or perhaps, we could move heap_trace to its own component?

All of this being said, I will remove total_isr_skipped for now, and open a new commit / PR to address it later.

I think it is best to discuss this outside of this PR as you mentioned as we should address your other PR before deciding on total_isr_skipped in my opinion.

Your latest changes look good to me. I will create the merge request today and keep you updated.

Thanks again for your contribution!

[igrr]

During development of new chips, breifly disable the KConfig, until spi_flash is supported. I would need to learn how to read KConfig options in CMAKE. Have we done anything like this before?

I think the problem is just generally that a lower-level component (heap) depends on the higher-level component (spi_flash). This is something we are trying to avoid. There might be a way to let spi_flash register some callback in heap component to establish a reverse dependency.

[chipweinberger]

I think the problem is just generally that a lower-level component (heap) depends on the higher-level component (spi_flash).

Ah, got it. Keeping good hierarchy is important. Yes, an optional callback registration seems like a simple solution!

[SoucheSouche]

Hi @chipweinberger,
we found a little discrepancy with the rest of the heap_caps_xx API functions in your PR. All the functions interacting with different memory capabilities are using uint32_t caps parameter so I replaced
heap_trace_dump_internal_ram(void), heap_trace_dump_psram(void) and heap_trace_dump(void) to heap_trace_dump(const uint32_t caps) to remain consistent with the rest of the component API.

[chipweinberger]

nice!

that seems like a good change!

[SoucheSouche]

Hi @chipweinberger, your feature was merged successfully! Sorry it took a little longer than expected but I had to create tests for the added functions.

[chipweinberger]

great! appreciate your work in getting this in!

[chipweinberger]

This looks like a bug to me. What will happen when the count is 0?

count is size_t, unsigned

I suppose it gets casted back to int, but it relies on size_t and int being the same size, i think. Needless complexity imo. If it works, nbd.

c39a9de

[SoucheSouche]

@chipweinberger , this is a good point. In both 32 and 64 bits architectures, records.count has a rank greater than or equal to the rank of (signed) int and therefore the result of records.counter - 1 for records.counter = 0 will always be 0xFFFF... but since this result is casted back to signed integer, this is not really an issue in my opinion.