From 07165308f60edf50a9180bea48f9c74b79d72daa Mon Sep 17 00:00:00 2001 From: Guillaume Souchere Date: Thu, 12 Oct 2023 12:24:15 +0200 Subject: [PATCH] feat(heap): update hash map to use singly linked list Previously, the hash map was a doubly linked list but was never using the characteristics of it. This commit switches the hash map to a singly linked list instead This commit also fixes memory leak in heap trace tests by setting hashmap size to 10 for the tests (instead of the default value of 250) See https://github.com/espressif/esp-idf/issues/11173 --- components/heap/heap_trace_standalone.c | 80 +++++++++++++------ components/heap/include/esp_heap_trace.h | 2 +- .../heap_tests/main/test_heap_trace.c | 2 + .../sdkconfig.ci.heap_trace_hashmap | 1 + 4 files changed, 61 insertions(+), 24 deletions(-) diff --git a/components/heap/heap_trace_standalone.c b/components/heap/heap_trace_standalone.c index 4a01d5e024..3a62d24cf7 100644 --- a/components/heap/heap_trace_standalone.c +++ b/components/heap/heap_trace_standalone.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2015-2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2015-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -66,7 +66,8 @@ static void list_setup(void); static void list_remove(heap_trace_record_t *r_remove); static heap_trace_record_t* list_add(const heap_trace_record_t *r_append); static heap_trace_record_t* list_pop_unused(void); -static heap_trace_record_t* list_find_address_reverse(void *p); +static heap_trace_record_t* list_find(void *p); +static void list_find_and_remove(void* p); /* The actual records. */ static records_t records; @@ -86,7 +87,9 @@ static size_t r_get_idx; // We use a hash_map to make locating a record by memory address very fast. // Key: addr // the memory address returned by malloc, calloc, realloc // Value: hash_map[hash(key)] // a list of records ptrs, which contains the relevant record. -static heap_trace_record_list_t* hash_map; // array of lists +SLIST_HEAD(heap_trace_hash_list_struct_t, heap_trace_record_t); +typedef struct heap_trace_hash_list_struct_t heap_trace_hash_list_t; +static heap_trace_hash_list_t* hash_map; // array of lists static size_t total_hashmap_hits; static size_t total_hashmap_miss; @@ -104,20 +107,20 @@ static HEAP_IRAM_ATTR size_t hash_idx(void* p) static HEAP_IRAM_ATTR void map_add(heap_trace_record_t *r_add) { size_t idx = hash_idx(r_add->address); - TAILQ_INSERT_TAIL(&hash_map[idx], r_add, tailq_hashmap); + SLIST_INSERT_HEAD(&hash_map[idx], r_add, slist_hashmap); } static HEAP_IRAM_ATTR void map_remove(heap_trace_record_t *r_remove) { size_t idx = hash_idx(r_remove->address); - TAILQ_REMOVE(&hash_map[idx], r_remove, tailq_hashmap); + SLIST_REMOVE(&hash_map[idx], r_remove, heap_trace_record_t, slist_hashmap); } static HEAP_IRAM_ATTR heap_trace_record_t* map_find(void *p) { size_t idx = hash_idx(p); heap_trace_record_t *r_cur = NULL; - TAILQ_FOREACH(r_cur, &hash_map[idx], tailq_hashmap) { + SLIST_FOREACH(r_cur, &hash_map[idx], slist_hashmap) { if (r_cur->address == p) { total_hashmap_hits++; return r_cur; @@ -126,6 +129,21 @@ static HEAP_IRAM_ATTR heap_trace_record_t* map_find(void *p) total_hashmap_miss++; return NULL; } + +static HEAP_IRAM_ATTR heap_trace_record_t* map_find_and_remove(void *p) +{ + size_t idx = hash_idx(p); + heap_trace_record_t *r_cur = NULL; + SLIST_FOREACH(r_cur, &hash_map[idx], slist_hashmap) { + if (r_cur->address == p) { + total_hashmap_hits++; + SLIST_REMOVE(&hash_map[idx], r_cur, heap_trace_record_t, slist_hashmap); + return r_cur; + } + } + total_hashmap_miss++; + return NULL; +} #endif // CONFIG_HEAP_TRACE_HASH_MAP esp_err_t heap_trace_init_standalone(heap_trace_record_t *record_buffer, size_t num_records) @@ -182,7 +200,7 @@ esp_err_t heap_trace_start(heap_trace_mode_t mode_param) #if CONFIG_HEAP_TRACE_HASH_MAP for (size_t i = 0; i < (size_t)CONFIG_HEAP_TRACE_HASH_MAP_SIZE; i++) { - TAILQ_INIT(&hash_map[i]); + SLIST_INIT(&hash_map[i]); } total_hashmap_hits = 0; @@ -416,6 +434,11 @@ static HEAP_IRAM_ATTR void record_allocation(const heap_trace_record_t *r_alloca heap_trace_record_t *r_first = TAILQ_FIRST(&records.list); + // always remove from hashmap first since list_remove is setting address field + // of the record to 0x00 +#if CONFIG_HEAP_TRACE_HASH_MAP + map_remove(r_first); +#endif list_remove(r_first); } // push onto end of list @@ -453,18 +476,16 @@ static HEAP_IRAM_ATTR void record_free(void *p, void **callers) total_frees++; - heap_trace_record_t *r_found = list_find_address_reverse(p); - if (r_found) { - if (mode == HEAP_TRACE_ALL) { - + if (mode == HEAP_TRACE_ALL) { + heap_trace_record_t *r_found = list_find(p); + if (r_found != NULL) { // add 'freed_by' info to the record memcpy(r_found->freed_by, callers, sizeof(void *) * STACK_DEPTH); - - } else { // HEAP_TRACE_LEAKS - // Leak trace mode, once an allocation is freed - // we remove it from the list & hashmap - list_remove(r_found); } + } else { // HEAP_TRACE_LEAKS + // Leak trace mode, once an allocation is freed + // we remove it from the list & hashmap + list_find_and_remove(p); } } @@ -491,10 +512,6 @@ static HEAP_IRAM_ATTR void list_remove(heap_trace_record_t* r_remove) { assert(records.count > 0); -#if CONFIG_HEAP_TRACE_HASH_MAP - map_remove(r_remove); -#endif - // remove from records.list TAILQ_REMOVE(&records.list, r_remove, tailq_list); @@ -579,8 +596,8 @@ static HEAP_IRAM_ATTR heap_trace_record_t* list_add(const heap_trace_record_t *r } } -// search records.list backwards for the allocation record matching this address -static HEAP_IRAM_ATTR heap_trace_record_t* list_find_address_reverse(void* p) +// search records.list for the allocation record matching this address +static HEAP_IRAM_ATTR heap_trace_record_t* list_find(void* p) { heap_trace_record_t *r_found = NULL; @@ -593,7 +610,6 @@ static HEAP_IRAM_ATTR heap_trace_record_t* list_find_address_reverse(void* p) } #endif - // Perf: We search backwards because new allocations are appended // to the end of the list and most allocations are short lived. heap_trace_record_t *r_cur = NULL; TAILQ_FOREACH(r_cur, &records.list, tailq_list) { @@ -606,6 +622,24 @@ static HEAP_IRAM_ATTR heap_trace_record_t* list_find_address_reverse(void* p) return r_found; } +static HEAP_IRAM_ATTR void list_find_and_remove(void* p) +{ +#if CONFIG_HEAP_TRACE_HASH_MAP + heap_trace_record_t *r_found = map_find_and_remove(p); + if (r_found != NULL) { + list_remove(r_found); + return; + } +#endif + heap_trace_record_t *r_cur = NULL; + TAILQ_FOREACH(r_cur, &records.list, tailq_list) { + if (r_cur->address == p) { + list_remove(r_cur); + break; + } + } +} + #include "heap_trace.inc" #endif // CONFIG_HEAP_TRACING_STANDALONE diff --git a/components/heap/include/esp_heap_trace.h b/components/heap/include/esp_heap_trace.h index 2b0daa2e4c..977d729fea 100644 --- a/components/heap/include/esp_heap_trace.h +++ b/components/heap/include/esp_heap_trace.h @@ -39,7 +39,7 @@ typedef struct heap_trace_record_t { #if CONFIG_HEAP_TRACING_STANDALONE TAILQ_ENTRY(heap_trace_record_t) tailq_list; ///< Linked list: prev & next records #if CONFIG_HEAP_TRACE_HASH_MAP - TAILQ_ENTRY(heap_trace_record_t) tailq_hashmap; ///< Linked list: prev & next in hashmap entry list + SLIST_ENTRY(heap_trace_record_t) slist_hashmap; ///< Linked list: next in hashmap entry list #endif // CONFIG_HEAP_TRACE_HASH_MAP #endif // CONFIG_HEAP_TRACING_STANDALONE } heap_trace_record_t; diff --git a/components/heap/test_apps/heap_tests/main/test_heap_trace.c b/components/heap/test_apps/heap_tests/main/test_heap_trace.c index 8369b1ee13..5755f499ea 100644 --- a/components/heap/test_apps/heap_tests/main/test_heap_trace.c +++ b/components/heap/test_apps/heap_tests/main/test_heap_trace.c @@ -70,6 +70,8 @@ TEST_CASE("heap trace leak check", "[heap-trace]") so recs[0].address is 0*/ TEST_ASSERT_EQUAL_PTR(recs[0].address, 0x00); + free(b); + heap_trace_stop(); } diff --git a/components/heap/test_apps/heap_tests/sdkconfig.ci.heap_trace_hashmap b/components/heap/test_apps/heap_tests/sdkconfig.ci.heap_trace_hashmap index 26ffa6c779..58fd6d102c 100644 --- a/components/heap/test_apps/heap_tests/sdkconfig.ci.heap_trace_hashmap +++ b/components/heap/test_apps/heap_tests/sdkconfig.ci.heap_trace_hashmap @@ -3,3 +3,4 @@ CONFIG_SPIRAM=y CONFIG_HEAP_TRACING_STANDALONE=y CONFIG_HEAP_TRACE_HASH_MAP=y CONFIG_HEAP_TRACE_HASH_MAP_IN_EXT_RAM=y +CONFIG_HEAP_TRACE_HASH_MAP_SIZE=10