Fix memleak in hash map by first resizing and then adding entry

Also add some addition celix properties statistics.

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);

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);

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);
-}
+}

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.

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
 }

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,49 +286,43 @@ 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;
 }
 
 static celix_status_t celix_hashMap_put(celix_hash_map_t* map, const char* strKey, long longKey, void* v) {
     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));
     }
 }

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.

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;
 }