Hash table inefficiencies (#4149)

* Update vine_manager.c

* Update vine_file_replica_table.c

* Fixes invalid read of size 1

* move cache_name copy idea to outter function

* Added hash_table.c halve buckets function

* implemented bucket halving in ITERATE function

* Updated benchmark test script to include insertion and removal times

* reduce count until above DEFAULT_MIN_LOAD

* reduce on delete

* simplify reduction

* add function hash_table_load

* update benchmark

* aux function to insert entry to buckets array

* same for reduce buckets

* update benchmark

* fix reduce

* update benchmark

* revert incorrect changes to vine_manager.c

---------

Co-authored-by: Ian Setia <[email protected]>
Co-authored-by: Benjamin Tovar <[email protected]>

Author: 3 people

dttools/src/Makefile

@@ -193,7 +193,7 @@ PROGRAMS = $(MOST_PROGRAMS) catalog_query
 
 SCRIPTS = cctools_gpu_autodetect
 TARGETS = $(LIBRARIES) $(PRELOAD_LIBRARIES) $(PROGRAMS) $(TEST_PROGRAMS)
-TEST_PROGRAMS = auth_test disk_alloc_test jx_test microbench multirun jx_count_obj_test jx_canonicalize_test jx_merge_test hash_table_offset_test hash_table_fromkey_test histogram_test category_test jx_binary_test bucketing_base_test bucketing_manager_test priority_queue_test
+TEST_PROGRAMS = auth_test disk_alloc_test jx_test microbench multirun jx_count_obj_test jx_canonicalize_test jx_merge_test hash_table_offset_test hash_table_fromkey_test hash_table_benchmark histogram_test category_test jx_binary_test bucketing_base_test bucketing_manager_test priority_queue_test
 
 all: $(TARGETS) catalog_query
 

dttools/src/hash_table.c

@@ -12,7 +12,8 @@ See the file COPYING for details.
 #include <string.h>
 
 #define DEFAULT_SIZE 127
-#define DEFAULT_LOAD 0.75
+#define DEFAULT_MAX_LOAD 0.75
+#define DEFAULT_MIN_LOAD 0.125
 #define DEFAULT_FUNC hash_string
 
 struct entry {
@@ -155,72 +156,113 @@ int hash_table_size(struct hash_table *h)
 	return h->size;
 }
 
-static int hash_table_double_buckets(struct hash_table *h)
+double hash_table_load(struct hash_table *h)
 {
-	struct hash_table *hn = hash_table_create(2 * h->bucket_count, h->hash_func);
+	return ((double)h->size) / h->bucket_count;
+}
 
-	if (!hn)
-		return 0;
+static int insert_to_buckets_aux(struct entry **buckets, int bucket_count, struct entry *new_entry)
+{
+	unsigned index;
+	struct entry *e;
 
-	/* Move pairs to new hash */
-	char *key;
-	void *value;
-	hash_table_firstkey(h);
-	while (hash_table_nextkey(h, &key, &value))
-		if (!hash_table_insert(hn, key, value)) {
-			hash_table_delete(hn);
+	index = new_entry->hash % bucket_count;
+	e = buckets[index];
+
+	while (e) {
+		/* check that this key does not already exist in the table */
+		if (new_entry->hash == e->hash && !strcmp(new_entry->key, e->key)) {
 			return 0;
 		}
+		e = e->next;
+	}
+
+	new_entry->next = buckets[index];
+	buckets[index] = new_entry;
+
+	return 1;
+}
+
+static int hash_table_double_buckets(struct hash_table *h)
+{
+	int new_count = (2 * (h->bucket_count + 1)) - 1;
+	struct entry **new_buckets = (struct entry **)calloc(new_count, sizeof(struct entry *));
+	if (!new_buckets) {
+		return 0;
+	}
 
-	/* Delete all old pairs */
 	struct entry *e, *f;
-	int i;
-	for (i = 0; i < h->bucket_count; i++) {
+	for (int i = 0; i < h->bucket_count; i++) {
 		e = h->buckets[i];
 		while (e) {
 			f = e->next;
-			free(e->key);
-			free(e);
+			e->next = NULL;
+			insert_to_buckets_aux(new_buckets, new_count, e);
 			e = f;
 		}
 	}
 
 	/* Make the old point to the new */
 	free(h->buckets);
-	h->buckets = hn->buckets;
-	h->bucket_count = hn->bucket_count;
-	h->size = hn->size;
+	h->buckets = new_buckets;
+	h->bucket_count = new_count;
 
 	/* structure of hash table changed completely, thus a nextkey would be incorrect. */
 	h->cant_iterate_yet = 1;
 
-	/* Delete reference to new, so old is safe */
-	free(hn);
-
 	return 1;
 }
 
-int hash_table_insert(struct hash_table *h, const char *key, const void *value)
+static int hash_table_reduce_buckets(struct hash_table *h)
 {
-	struct entry *e;
-	unsigned hash, index;
+	int new_count = ((h->bucket_count + 1) / 2) - 1;
 
-	if (((float)h->size / h->bucket_count) > DEFAULT_LOAD)
-		hash_table_double_buckets(h);
+	/* DEFAULT_SIZE is the minimum size */
+	if (new_count < DEFAULT_SIZE) {
+		return 1;
+	}
 
-	hash = h->hash_func(key);
-	index = hash % h->bucket_count;
-	e = h->buckets[index];
+	/* Table cannot be reduced above DEFAULT_MAX_LOAD */
+	if (((float)h->size / new_count) > DEFAULT_MAX_LOAD) {
+		return 1;
+	}
 
-	while (e) {
-		if (hash == e->hash && !strcmp(key, e->key))
-			return 0;
-		e = e->next;
+	struct entry **new_buckets = (struct entry **)calloc(new_count, sizeof(struct entry *));
+	if (!new_buckets) {
+		return 0;
 	}
 
-	e = (struct entry *)malloc(sizeof(struct entry));
-	if (!e)
+	struct entry *e, *f;
+	for (int i = 0; i < h->bucket_count; i++) {
+		e = h->buckets[i];
+		while (e) {
+			f = e->next;
+			e->next = NULL;
+			insert_to_buckets_aux(new_buckets, new_count, e);
+			e = f;
+		}
+	}
+
+	/* Make the old point to the new */
+	free(h->buckets);
+	h->buckets = new_buckets;
+	h->bucket_count = new_count;
+
+	/* structure of hash table changed completely, thus a nextkey would be incorrect. */
+	h->cant_iterate_yet = 1;
+
+	return 1;
+}
+
+int hash_table_insert(struct hash_table *h, const char *key, const void *value)
+{
+	if (((float)h->size / h->bucket_count) > DEFAULT_MAX_LOAD)
+		hash_table_double_buckets(h);
+
+	struct entry *e = (struct entry *)malloc(sizeof(struct entry));
+	if (!e) {
 		return 0;
+	}
 
 	e->key = strdup(key);
 	if (!e->key) {
@@ -229,16 +271,18 @@ int hash_table_insert(struct hash_table *h, const char *key, const void *value)
 	}
 
 	e->value = (void *)value;
-	e->hash = hash;
-	e->next = h->buckets[index];
-	h->buckets[index] = e;
-	h->size++;
+	e->hash = h->hash_func(e->key);
 
-	/* inserting cause different behaviours with nextkey (e.g., sometimes the new
-	 * key would be included or skipped in the iteration */
-	h->cant_iterate_yet = 1;
+	int inserted = insert_to_buckets_aux(h->buckets, h->bucket_count, e);
+	if (inserted) {
+		h->size++;
 
-	return 1;
+		/* inserting cause different behaviours with nextkey (e.g., sometimes the new
+		 * key would be included or skipped in the iteration */
+		h->cant_iterate_yet = 1;
+	}
+
+	return inserted;
 }
 
 void *hash_table_remove(struct hash_table *h, const char *key)
@@ -264,8 +308,9 @@ void *hash_table_remove(struct hash_table *h, const char *key)
 			free(e);
 			h->size--;
 
-			/* the deletion may cause nextkey to fail */
-			h->cant_iterate_yet = 1;
+			if (((float)h->size / h->bucket_count) < DEFAULT_MIN_LOAD) {
+				hash_table_reduce_buckets(h);
+			}
 
 			return value;
 		}

dttools/src/hash_table.h

@@ -70,6 +70,14 @@ void hash_table_delete(struct hash_table *h);
 
 int hash_table_size(struct hash_table *h);
 
+/** Return the proportion of elements
+vs buckets in the table.
+@return The load of the table.
+@param h A pointer to a hash table.
+*/
+
+double hash_table_load(struct hash_table *h);
+
 /** Insert a key and value.
 This call will fail if the table already contains the same key.
 You must call @ref hash_table_remove to remove it.

dttools/src/hash_table_benchmark.c

@@ -0,0 +1,81 @@
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <time.h>
+#include <math.h>
+#include <stdint.h>
+#include "hash_table.h"
+
+#define MAX_KEY_LEN 32
+
+// Generate a unique string key from an integer
+void generate_key(int i, char *key)
+{
+	snprintf(key, MAX_KEY_LEN, "key%d", i);
+}
+
+double measure_iteration_time(struct hash_table *h)
+{
+	struct timespec start, end;
+	clock_gettime(CLOCK_MONOTONIC, &start);
+
+	char *key;
+	void *value;
+	HASH_TABLE_ITERATE(h, key, value);
+
+	clock_gettime(CLOCK_MONOTONIC, &end);
+
+	double start_sec = start.tv_sec + start.tv_nsec / 1e9;
+	double end_sec = end.tv_sec + end.tv_nsec / 1e9;
+
+	return end_sec - start_sec;
+}
+
+int main()
+{
+	int power_step = 0;
+	int power_max_step = 15; // ~64k max entries
+
+	struct hash_table *h = hash_table_create(0, 0);
+
+	char key[MAX_KEY_LEN];
+	int entries_counter = 0;
+
+	printf("INSERTION PHASE:\n");
+	for (power_step = 0; power_step <= power_max_step; power_step++) {
+		double total_time = 0;
+		double max_load = hash_table_load(h);
+		double entries_to_add_remove = pow(2, power_step);
+		for (int i = 0; i < entries_to_add_remove; i++) {
+			entries_counter++;
+			generate_key(entries_counter, key);
+			hash_table_insert(h, key, NULL);
+			max_load = hash_table_load(h) > max_load ? hash_table_load(h) : max_load;
+			total_time += measure_iteration_time(h);
+		}
+
+		printf("step %3d size %8d buckets %8d load_max %3.6f load_now %3.6f time %3.6f time_norm %3.6f\n", power_step, hash_table_size(h), (int)ceil(hash_table_size(h) / hash_table_load(h)), max_load, hash_table_load(h), total_time, total_time / entries_to_add_remove);
+	}
+
+	printf("REMOVAL PHASE:\n");
+
+	entries_counter = 0;
+	for (power_step = power_max_step; power_step > 0; power_step--) {
+		double total_time = 0;
+		double min_load = hash_table_load(h);
+		double entries_to_add_remove = pow(2, power_step);
+		for (int i = 0; i < entries_to_add_remove; i++) {
+			entries_counter++;
+			generate_key(entries_counter, key);
+			hash_table_remove(h, key);
+			min_load = hash_table_load(h) < min_load ? hash_table_load(h) : min_load;
+			total_time += measure_iteration_time(h);
+		}
+
+		printf("step %3d size %8d buckets %8d load_min %3.6f load_now %3.6f time %3.6f time_norm %3.6f\n", power_step, hash_table_size(h), (int)ceil(hash_table_size(h) / hash_table_load(h)), min_load, hash_table_load(h), total_time, total_time / entries_to_add_remove);
+	}
+
+	hash_table_delete(h);
+
+	return 0;
+}