From 10e16f65caddd4e95237e96746eba352ac0414a7 Mon Sep 17 00:00:00 2001 From: Pepijn Noltes Date: Sun, 12 Nov 2023 19:01:29 +0100 Subject: [PATCH] Fix memleak in hash map by first resizing and then adding entry Also add some addition celix properties statistics. --- .../benchmark/src/LongHashmapBenchmark.cc | 4 +- .../benchmark/src/StringHashmapBenchmark.cc | 16 ++++--- libs/utils/gtest/src/PropertiesTestSuite.cc | 22 +++++---- libs/utils/include/celix_string_hash_map.h | 2 + .../celix_properties_internal.h | 12 ++++- libs/utils/src/celix_hash_map.c | 48 +++++++++++-------- libs/utils/src/celix_hash_map_private.h | 7 +-- libs/utils/src/properties.c | 16 ++++++- 8 files changed, 79 insertions(+), 48 deletions(-) diff --git a/libs/utils/benchmark/src/LongHashmapBenchmark.cc b/libs/utils/benchmark/src/LongHashmapBenchmark.cc index 7669fc889..5b0d9b219 100644 --- a/libs/utils/benchmark/src/LongHashmapBenchmark.cc +++ b/libs/utils/benchmark/src/LongHashmapBenchmark.cc @@ -122,8 +122,6 @@ static void LongHashmapBenchmark_addEntryToDeprecatedHashmap(benchmark::State& s hashMap_put(benchmark.deprecatedHashMap, (void*)42, (void*)42); } state.SetItemsProcessed(state.iterations()); - state.counters["average_bucket_size"] = NAN; - state.counters["stddev_bucket_size"] = NAN; } static void LongHashmapBenchmark_findEntryFromStdMap(benchmark::State& state) { @@ -212,7 +210,7 @@ static void LongHashmapBenchmark_fillDeprecatedHashMap(benchmark::State& state) #define CELIX_BENCHMARK(name) \ BENCHMARK(name)->MeasureProcessCPUTime()->UseRealTime()->Unit(benchmark::kNanosecond) \ - ->RangeMultiplier(10)->Range(10, 1000000) + ->RangeMultiplier(10)->Range(10, 100000) CELIX_BENCHMARK(LongHashmapBenchmark_addEntryToStdMap); //reference CELIX_BENCHMARK(LongHashmapBenchmark_addEntryToCelixHashmap); diff --git a/libs/utils/benchmark/src/StringHashmapBenchmark.cc b/libs/utils/benchmark/src/StringHashmapBenchmark.cc index a19fa25ed..134a77dae 100644 --- a/libs/utils/benchmark/src/StringHashmapBenchmark.cc +++ b/libs/utils/benchmark/src/StringHashmapBenchmark.cc @@ -222,10 +222,14 @@ static void StringHashmapBenchmark_findEntryFromCelixProperties(benchmark::State state.SetItemsProcessed(state.iterations()); auto stats = celix_properties_getStatistics(benchmark.celixProperties); - state.counters["nrOfBuckets"] = (double)stats.nrOfBuckets; - state.counters["resizeCount"] = (double)stats.resizeCount; - state.counters["averageNrOfEntriesPerBucket"] = stats.averageNrOfEntriesPerBucket; - state.counters["stdDeviationNrOfEntriesPerBucket"] = stats.stdDeviationNrOfEntriesPerBucket; + state.counters["nrOfBuckets"] = (double)stats.mapStatistics.nrOfBuckets; + state.counters["resizeCount"] = (double)stats.mapStatistics.resizeCount; + state.counters["averageNrOfEntriesPerBucket"] = stats.mapStatistics.averageNrOfEntriesPerBucket; + state.counters["stdDeviationNrOfEntriesPerBucket"] = stats.mapStatistics.stdDeviationNrOfEntriesPerBucket; + state.counters["sizeOfKeysAndStringValues"] = (double)stats.sizeOfKeysAndStringValues; + state.counters["averageSizeOfKeysAndStringValues"] = (double)stats.averageSizeOfKeysAndStringValues; + state.counters["fillStringOptimizationBufferPercentage"] = stats.fillStringOptimizationBufferPercentage; + state.counters["fillEntriesOptimizationBufferPercentage"] = stats.fillEntriesOptimizationBufferPercentage; } static void StringHashmapBenchmark_fillStdMap(benchmark::State& state) { @@ -276,8 +280,8 @@ static void StringHashmapBenchmark_fillProperties(benchmark::State& state) { } #define CELIX_BENCHMARK(name) \ - BENCHMARK(name)->MeasureProcessCPUTime()->UseRealTime()->Unit(benchmark::kMicrosecond) \ - ->RangeMultiplier(10)->Range(10, 1000000) + BENCHMARK(name)->MeasureProcessCPUTime()->UseRealTime()->Unit(benchmark::kNanosecond) \ + ->RangeMultiplier(10)->Range(10, 100000) CELIX_BENCHMARK(StringHashmapBenchmark_addEntryToStdMap); //reference CELIX_BENCHMARK(StringHashmapBenchmark_addEntryToCelixHashmap); diff --git a/libs/utils/gtest/src/PropertiesTestSuite.cc b/libs/utils/gtest/src/PropertiesTestSuite.cc index 1c6af59d4..7dd9cf582 100644 --- a/libs/utils/gtest/src/PropertiesTestSuite.cc +++ b/libs/utils/gtest/src/PropertiesTestSuite.cc @@ -33,13 +33,17 @@ class PropertiesTestSuite : public ::testing::Test { celix_err_resetErrors(); } - void printStats(const celix_hash_map_statistics_t* stats) { + void printStats(const celix_properties_statistics_t* stats) { printf("Properties statistics:\n"); - printf("|- nr of entries: %zu\n", stats->nrOfEntries); - printf("|- nr of buckets: %zu\n", stats->nrOfBuckets); - printf("|- average nr of entries in bucket: %f\n", stats->averageNrOfEntriesPerBucket); - printf("|- stddev nr of entries in bucket: %f\n", stats->stdDeviationNrOfEntriesPerBucket); - printf("|- resize count: %zu\n", stats->resizeCount); + printf("|- nr of entries: %zu\n", stats->mapStatistics.nrOfEntries); + printf("|- nr of buckets: %zu\n", stats->mapStatistics.nrOfBuckets); + printf("|- average nr of entries in bucket: %f\n", stats->mapStatistics.averageNrOfEntriesPerBucket); + printf("|- stddev nr of entries in bucket: %f\n", stats->mapStatistics.stdDeviationNrOfEntriesPerBucket); + printf("|- resize count: %zu\n", stats->mapStatistics.resizeCount); + printf("|- size of keys and string values: %zu bytes\n", stats->sizeOfKeysAndStringValues); + printf("|- average size of keys and string values: %f bytes\n", stats->averageSizeOfKeysAndStringValues); + printf("|- fill string optimization buffer percentage: %f\n", stats->fillStringOptimizationBufferPercentage); + printf("|- fill entries optimization buffer percentage: %f\n", stats->fillEntriesOptimizationBufferPercentage); } }; @@ -701,7 +705,7 @@ TEST_F(PropertiesTestSuite, GetStatsTest) { celix_properties_set(props, std::to_string(i).c_str(), std::to_string(i).c_str()); } - celix_hash_map_statistics_t stats = celix_properties_getStatistics(props); - EXPECT_EQ(stats.nrOfEntries, 200); + auto stats = celix_properties_getStatistics(props); + EXPECT_EQ(stats.mapStatistics.nrOfEntries, 200); printStats(&stats); -} \ No newline at end of file +} diff --git a/libs/utils/include/celix_string_hash_map.h b/libs/utils/include/celix_string_hash_map.h index e64fb7f8f..186012d0b 100644 --- a/libs/utils/include/celix_string_hash_map.h +++ b/libs/utils/include/celix_string_hash_map.h @@ -125,6 +125,8 @@ typedef struct celix_string_hash_map_create_options { * If the key is not already in use, the celix_stringHashMap_put* will store the provided key and the caller * should not free the provided key. * + * Uf a celix_stringHashMap_put* returns a error, the caller may free the provided key. + * * @note This changes the default behaviour of the celix_stringHashMap_put* functions. * * Default is false. diff --git a/libs/utils/include_internal/celix_properties_internal.h b/libs/utils/include_internal/celix_properties_internal.h index 00cb3e4a3..34df8eb12 100644 --- a/libs/utils/include_internal/celix_properties_internal.h +++ b/libs/utils/include_internal/celix_properties_internal.h @@ -34,10 +34,18 @@ extern "C" { #endif +typedef struct celix_properties_statistics_t { + celix_hash_map_statistics_t mapStatistics; /**< The statistics of the underlining hash map. */ + size_t sizeOfKeysAndStringValues; /**< The size of the keys and string value representations in bytes. */ + double averageSizeOfKeysAndStringValues; /**< The average size of the keys and string values in bytes. */ + double fillStringOptimizationBufferPercentage; /**< The percentage of the fill string optimization buffer. */ + double fillEntriesOptimizationBufferPercentage; /**< The percentage of the fill entries optimization buffer. */ +} celix_properties_statistics_t; + /** - * @brief Return the statistics for the underlining string hash map of the provided properties set. + * @brief Return the statistics for the of the provided properties set. */ -CELIX_UTILS_EXPORT celix_hash_map_statistics_t celix_properties_getStatistics(const celix_properties_t* properties); +CELIX_UTILS_EXPORT celix_properties_statistics_t celix_properties_getStatistics(const celix_properties_t* properties); #ifdef __cplusplus } diff --git a/libs/utils/src/celix_hash_map.c b/libs/utils/src/celix_hash_map.c index 431759af8..93e14d351 100644 --- a/libs/utils/src/celix_hash_map.c +++ b/libs/utils/src/celix_hash_map.c @@ -98,8 +98,11 @@ static unsigned int celix_longHashMap_hash(long key) { return (key ^ (key >> (sizeof(key)*8/2)) * CELIX_HASHMAP_HASH_PRIME); } +/** + * @brief Check if hash map needs to be resized if a extra entry is added. + */ static bool celix_hashMap_needsResize(const celix_hash_map_t* map) { - double loadFactor = (double)map->size / (double)map->bucketsSize; + double loadFactor = (double)(map->size +1) / (double)map->bucketsSize; return loadFactor > map->maxLoadFactor; } @@ -237,7 +240,18 @@ static void celix_hashMap_destroyRemovedEntry(celix_hash_map_t* map, celix_hash_ free(removedEntry); } -celix_status_t celix_hashMap_addEntry(celix_hash_map_t* map, unsigned int hash, const celix_hash_map_key_t* key, const celix_hash_map_value_t* value, unsigned int bucketIndex) { +celix_status_t celix_hashMap_addEntry(celix_hash_map_t* map, const celix_hash_map_key_t* key, const celix_hash_map_value_t* value) { + //resize (if needed) first, so that if allocation fails, no entry is yet created + if (celix_hashMap_needsResize(map)) { + celix_status_t status = celix_hashMap_resize(map); + if (status != CELIX_SUCCESS) { + return status; + } + } + + bool isStringKey = map->keyType == CELIX_HASH_MAP_STRING_KEY; + unsigned int hash = isStringKey ? celix_utils_stringHash(key->strKey) : celix_longHashMap_hash(key->longKey); + unsigned int bucketIndex = celix_hashMap_indexFor(hash, map->bucketsSize); celix_hash_map_entry_t* entry = map->buckets[bucketIndex]; celix_hash_map_entry_t* newEntry = malloc(sizeof(*newEntry)); if (!newEntry) { @@ -246,7 +260,7 @@ celix_status_t celix_hashMap_addEntry(celix_hash_map_t* map, unsigned int hash, } newEntry->hash = hash; - if (map->keyType == CELIX_HASH_MAP_STRING_KEY) { + if (isStringKey) { newEntry->key.strKey = map->storeKeysWeakly ? key->strKey : celix_utils_strdup(key->strKey); } else { newEntry->key.longKey = key->longKey; @@ -255,16 +269,14 @@ celix_status_t celix_hashMap_addEntry(celix_hash_map_t* map, unsigned int hash, newEntry->next = entry; map->buckets[bucketIndex] = newEntry; map->size += 1; - if (celix_hashMap_needsResize(map)) { - return celix_hashMap_resize(map); - } + return CELIX_SUCCESS; } /** * @brief Put the value in the map. If long hash is used, strKey should be NULL. */ -static celix_status_t celix_hashMap_putValue(celix_hash_map_t* map, const char* strKey, long longKey, const celix_hash_map_value_t* value, celix_hash_map_value_t* replacedValueOut) { +static celix_status_t celix_hashMap_putValue(celix_hash_map_t* map, const char* strKey, long longKey, const celix_hash_map_value_t* value) { celix_hash_map_key_t key; if (strKey) { key.strKey = strKey; @@ -274,20 +286,14 @@ static celix_status_t celix_hashMap_putValue(celix_hash_map_t* map, const char* celix_hash_map_entry_t* entryFound = celix_hashMap_getEntry(map, strKey, longKey); if (entryFound) { - if (replacedValueOut != NULL) { - *replacedValueOut = entryFound->value; - } + //replace value celix_hashMap_callRemovedCallback(map, entryFound); memcpy(&entryFound->value, value, sizeof(*value)); return CELIX_SUCCESS; } - unsigned int hash = strKey ? celix_utils_stringHash(strKey) : celix_longHashMap_hash(longKey); - unsigned int index = celix_hashMap_indexFor(hash, map->bucketsSize); - celix_status_t status = celix_hashMap_addEntry(map, hash, &key, value, index); - if (status == CELIX_SUCCESS && replacedValueOut != NULL) { - memset(replacedValueOut, 0, sizeof(*replacedValueOut)); - } + //new entry + celix_status_t status = celix_hashMap_addEntry(map, &key, value); return status; } @@ -295,28 +301,28 @@ static celix_status_t celix_hashMap_put(celix_hash_map_t* map, const char* strKe celix_hash_map_value_t value; memset(&value, 0, sizeof(value)); value.ptrValue = v; - return celix_hashMap_putValue(map, strKey, longKey, &value, NULL); + return celix_hashMap_putValue(map, strKey, longKey, &value); } static celix_status_t celix_hashMap_putLong(celix_hash_map_t* map, const char* strKey, long longKey, long v) { celix_hash_map_value_t value; memset(&value, 0, sizeof(value)); value.longValue = v; - return celix_hashMap_putValue(map, strKey, longKey, &value, NULL); + return celix_hashMap_putValue(map, strKey, longKey, &value); } static celix_status_t celix_hashMap_putDouble(celix_hash_map_t* map, const char* strKey, long longKey, double v) { celix_hash_map_value_t value; memset(&value, 0, sizeof(value)); value.doubleValue = v; - return celix_hashMap_putValue(map, strKey, longKey, &value, NULL); + return celix_hashMap_putValue(map, strKey, longKey, &value); } static celix_status_t celix_hashMap_putBool(celix_hash_map_t* map, const char* strKey, long longKey, bool v) { celix_hash_map_value_t value; memset(&value, 0, sizeof(value)); value.boolValue = v; - return celix_hashMap_putValue(map, strKey, longKey, &value, NULL); + return celix_hashMap_putValue(map, strKey, longKey, &value); } /** @@ -722,7 +728,7 @@ void celix_stringHashMapIterator_next(celix_string_hash_map_iterator_t* iter) { iter->value = entry->value; } else { iter->_internal[1] = NULL; - iter->key = NULL; + iter->key = ""; memset(&iter->value, 0, sizeof(iter->value)); } } diff --git a/libs/utils/src/celix_hash_map_private.h b/libs/utils/src/celix_hash_map_private.h index 148826bd9..4fb97a24f 100644 --- a/libs/utils/src/celix_hash_map_private.h +++ b/libs/utils/src/celix_hash_map_private.h @@ -47,11 +47,8 @@ celix_status_t celix_hashMap_resize(celix_hash_map_t* map); /** * @brief Add a new entry to the hash map. */ -celix_status_t celix_hashMap_addEntry(celix_hash_map_t* map, - unsigned int hash, - const celix_hash_map_key_t* key, - const celix_hash_map_value_t* value, - unsigned int bucketIndex); +celix_status_t +celix_hashMap_addEntry(celix_hash_map_t* map, const celix_hash_map_key_t* key, const celix_hash_map_value_t* value); /** * @brief Initialize the hash map. diff --git a/libs/utils/src/properties.c b/libs/utils/src/properties.c index 6df827d66..af0320594 100644 --- a/libs/utils/src/properties.c +++ b/libs/utils/src/properties.c @@ -929,6 +929,18 @@ bool celix_propertiesIterator_equals(const celix_properties_iterator_t* a, const return celix_stringHashMapIterator_equals(&internalIterA.mapIter, &internalIterB.mapIter); } -celix_hash_map_statistics_t celix_properties_getStatistics(const celix_properties_t* properties) { - return celix_stringHashMap_getStatistics(properties->map); +celix_properties_statistics_t celix_properties_getStatistics(const celix_properties_t* properties) { + size_t sizeOfKeysAndStringValues = 0; + CELIX_PROPERTIES_ITERATE(properties, iter) { + sizeOfKeysAndStringValues += celix_utils_strlen(iter.key) + 1; + sizeOfKeysAndStringValues += celix_utils_strlen(iter.entry.value) + 1; + } + + celix_properties_statistics_t stats; + stats.sizeOfKeysAndStringValues = sizeOfKeysAndStringValues; + stats.averageSizeOfKeysAndStringValues = (double)sizeOfKeysAndStringValues / celix_properties_size(properties) * 2; + stats.fillStringOptimizationBufferPercentage = (double)properties->currentStringBufferIndex / CELIX_PROPERTIES_OPTIMIZATION_STRING_BUFFER_SIZE; + stats.fillEntriesOptimizationBufferPercentage = (double)properties->currentEntriesBufferIndex / CELIX_PROPERTIES_OPTIMIZATION_ENTRIES_BUFFER_SIZE; + stats.mapStatistics = celix_stringHashMap_getStatistics(properties->map); + return stats; }