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

[Heap Trace Standalone] Support storing records in SPI RAM (IDFGH-8683) #10127

Conversation

chipweinberger
Copy link
Contributor

@chipweinberger chipweinberger commented Nov 7, 2022

EDIT: WONT MERGE, due to new PR with simpler approch: #10141

Add support for storing records in SPI RAM. Internal RAM is just used as a temporary buffer.

With this change, amazingly a small internal ram record buffer size of 13 was enough to handle all records! And this is for a very complex app with bluetooth, wifi, console. etc.

With this change, I am able to store all allocations since boot. During development one can leave this enabled, & If they unexpectedly hit a Low Ram condition, they can immediately print all allocations and analyze the problem without needing to reproduce with host mode enabled.

This makes standalone mode 10x more useful, IMO.

Also adds heap_trace_dump_internal_ram_only(), which is often all we care about.

And adds heap_trace_summary() so that users can access more useful heap tracing information.

Code:

In this PR, Internal RAM is just used as a temporary buffer. We stream records to SPI ram whenever we get a chance (i.e. at the next heap_trace_X() invocation from a non-IRAM function).

@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch 6 times, most recently from 45edcb9 to b159e90 Compare November 7, 2022 10:03
@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 7, 2022
@github-actions github-actions bot changed the title [Heap Trace Standalone] Support storing records in SPI RAM [Heap Trace Standalone] Support storing records in SPI RAM (IDFGH-8683) Nov 7, 2022
@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch 5 times, most recently from d6795ca to 3c48f48 Compare November 7, 2022 10:15
@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch 8 times, most recently from 6f0271d to 60abd2e Compare November 7, 2022 20:02
@chipweinberger
Copy link
Contributor Author

chipweinberger commented Nov 7, 2022

Gonna merge the 2 standalone implementations into one. Give me a a little bit.

@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch 5 times, most recently from 49a4266 to cea5159 Compare November 7, 2022 21:06
@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch 4 times, most recently from fa6b9e2 to 9ac13dd Compare November 7, 2022 21:35
@chipweinberger
Copy link
Contributor Author

Okay, changes are done. I re-tested heap_trace_init_standalone and heap_trace_init_standalone_with_spi_ram and they work as expected.

This should be good to merge.

@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch 4 times, most recently from 395a270 to 2c198da Compare November 8, 2022 00:17
@Alvin1Zhang
Copy link
Collaborator

Thanks for your contribution.

@igrr
Copy link
Member

igrr commented Nov 8, 2022

Question, why can't we simply allocate the trace buffer in external RAM? I.e. place heap_trace_record_t into external RAM and call heap_trace_init_standalone?

If the user application doesn't call heap-related functions when cache is disabled, then there shouldn't be a problem accessing external RAM from the heap tracing functions.

@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-spi-ram branch from 0cddfed to b8221aa Compare November 8, 2022 09:25
@chipweinberger
Copy link
Contributor Author

chipweinberger commented Nov 8, 2022

@igrr

From esp_heap_trace.h

* @param record_buffer Provide a buffer to use for heap trace data.
*  Must remain valid any time heap tracing is enabled, meaning
 * it must be allocated from internal memory not in PSRAM.

I'd think avoiding the "cache disabled" issue is important.

@igrr
Copy link
Member

igrr commented Nov 8, 2022

In ESP-IDF internal components we don't perform heap allocations from interrupt handlers, so there should be no cases of accessing heap functions while cache is disabled. This might only be the case in user code, if a heap related function is called from an ISR. However I don't think there is a need to support these two things simultaneously:

  1. heap functions called from ISRs (hence, possibly while cache is disabled)
  2. heap tracing into a buffer in external RAM

since 1) is a bad practice and is not recommended anyway.

Adding an assert somewhere in heap tracing code that !(trace buffer pointer is in external RAM && cache is disabled) seems to be sufficient to catch this potential issue. Plus, updating the lines of documentation you have pointed to say that the heap trace buffer can be in external RAM if no heap allocations from IRAM ISRs are performed.

@chipweinberger
Copy link
Contributor Author

Ok. Can you give more details on how to do this? !(trace buffer pointer is in external RAM && cache is disabled)

@chipweinberger
Copy link
Contributor Author

I'll close this PR and open a new one without the dual Internal / SPI ram buffers.

@igrr
Copy link
Member

igrr commented Nov 8, 2022

Sorry, I should have mentioned that. Probably something along the lines of: spi_flash_cache_enabled() || esp_ptr_internal(ptr)

@espressif-bot espressif-bot added Resolution: Duplicate This issue or pull request already exists Status: Done Issue is done internally and removed Status: Opened Issue is new labels Nov 9, 2022
@chipweinberger chipweinberger deleted the user/chip/heap-trace-standalone-spi-ram branch November 18, 2022 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Duplicate This issue or pull request already exists Status: Done Issue is done internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants