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] fix terrible Leaks perf on large records by using doubly linked list (IDFGH-8690) #10133

Conversation

chipweinberger
Copy link
Contributor

@chipweinberger chipweinberger commented Nov 8, 2022

This PR is rejected in favor or a new PR: #10478

This augments: #10127 (Edit: this PR will need to be rebased onto #10141)

Leaks: The original implementation memmove()'s the entire record buffer every time a record is freed. This is obviously terrible for perf.

My ESP32-S3 was not useable after ~300 records in SPI ram.

With this change, I can keep Heap Trace Standalone enabled without a noticeable impact on perf, tested up to 2540 records!

@chipweinberger chipweinberger force-pushed the user/chip/heap-trace-standalone-linked-list branch from ede5484 to 30dee16 Compare November 8, 2022 08:57
@espressif-bot espressif-bot added the Status: Opened Issue is new label Nov 8, 2022
@github-actions github-actions bot changed the title [Heap Trace Standalone] fix terrible Leaks perf on large records by using doubly linked list [Heap Trace Standalone] fix terrible Leaks perf on large records by using doubly linked list (IDFGH-8690) Nov 8, 2022
@chipweinberger
Copy link
Contributor Author

chipweinberger commented Nov 10, 2022

Please wait on reviewing this PR.

  • need to rebase
  • plan to add a optional very simple hashmap lookup, for more speedup. It wont handle collisions. Just falls back to traversing the linked list if the record->address is not the first item it looked up.
typedef struct {
   heap_trace_record_t* record;
} heap_trace_hashmap_item_t;

// recommended size: 8x-12x the number of records
esp_err_t heap_trace_add_hashmap(heap_trace_hashmap_item_t* items, size_t count)

@chipweinberger
Copy link
Contributor Author

Closing this PR in favor of a new refactored PR.

#10478

@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 Jan 4, 2023
@chipweinberger chipweinberger deleted the user/chip/heap-trace-standalone-linked-list branch May 20, 2023 10:51
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

3 participants