From 13d302e95ac2ee59aaa2bd6b743ef700640170c2 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Thu, 16 Apr 2020 18:05:37 +0200 Subject: [PATCH 01/10] unit-tests: Optionally report the times test cases ran --- src/libstrongswan/tests/test_runner.c | 65 ++++++++++++++++++++++++++- 1 file changed, 64 insertions(+), 1 deletion(-) diff --git a/src/libstrongswan/tests/test_runner.c b/src/libstrongswan/tests/test_runner.c index c6dd97716..603495c13 100644 --- a/src/libstrongswan/tests/test_runner.c +++ b/src/libstrongswan/tests/test_runner.c @@ -28,6 +28,7 @@ #include #include #include +#include /** * Get a tty color escape character for stderr @@ -442,6 +443,39 @@ static void print_failures(array_t *failures, bool warnings) threads_deinit(); } +#if defined(CLOCK_THREAD_CPUTIME_ID) && defined(HAVE_CLOCK_GETTIME) + +/** + * Start a timer + */ +static void start_timing(struct timespec *start) +{ + clock_gettime(CLOCK_THREAD_CPUTIME_ID, start); +} + +/** + * End a timer, return ms + */ +static double end_timing(struct timespec *start) +{ + struct timespec end; + + if (!getenv("TESTS_TIMING")) + { + return 0; + } + clock_gettime(CLOCK_THREAD_CPUTIME_ID, &end); + return (end.tv_nsec - start->tv_nsec) / 1000000.0 + + (end.tv_sec - start->tv_sec) * 1000.0; +} + +#else /* CLOCK_THREAD_CPUTIME_ID */ + +#define start_timing(start) ((start)->tv_sec = 0, (start)->tv_nsec = 0) +#define end_timing(...) (0) + +#endif /* CLOCK_THREAD_CPUTIME_ID */ + /** * Run a single test case with fixtures */ @@ -449,9 +483,20 @@ static bool run_case(test_case_t *tcase, test_runner_init_t init, char *cfg) { enumerator_t *enumerator; test_function_t *tfun; - int passed = 0; + double *times; + double total_time = 0; + int tests = 0, ti = 0, passed = 0; array_t *failures, *warnings; + /* determine the number of tests we will run */ + enumerator = array_create_enumerator(tcase->functions); + while (enumerator->enumerate(enumerator, &tfun)) + { + tests += tfun->end - tfun->start; + } + enumerator->destroy(enumerator); + times = calloc(tests, sizeof(double)); + failures = array_create(sizeof(failure_t), 0); warnings = array_create(sizeof(failure_t), 0); @@ -467,10 +512,12 @@ static bool run_case(test_case_t *tcase, test_runner_init_t init, char *cfg) { if (pre_test(init, cfg)) { + struct timespec start; bool ok = FALSE; int leaks = 0; test_setup_timeout(tcase->timeout); + start_timing(&start); if (call_fixture(tcase, TRUE)) { @@ -491,6 +538,8 @@ static bool run_case(test_case_t *tcase, test_runner_init_t init, char *cfg) ok = FALSE; } + times[ti] = end_timing(&start); + total_time += times[ti++]; test_setup_timeout(0); if (ok) @@ -530,6 +579,20 @@ static bool run_case(test_case_t *tcase, test_runner_init_t init, char *cfg) } enumerator->destroy(enumerator); + if (total_time) + { + fprintf(stderr, " %s%s%.3f ms%s", tty_escape_get(2, TTY_BOLD), + TTY(BLUE), total_time, tty_escape_get(2, TTY_RESET)); + if (ti > 1) + { + fprintf(stderr, " %s[", TTY(BLUE)); + for (ti = 0; ti < tests; ti++) + { + fprintf(stderr, "%s%.3f ms", times[ti], ti == 0 ? "" : ", "); + } + fprintf(stderr, "]%s", TTY(DEF)); + } + } fprintf(stderr, "\n"); print_failures(warnings, TRUE); From 54a2b35f0796e7924efd7dc11fa25d1d260021da Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 08:02:59 +0200 Subject: [PATCH 02/10] unit-tests: Add tests for larger number of items in hashtables --- .../tests/suites/test_hashtable.c | 154 ++++++++++++++++++ 1 file changed, 154 insertions(+) diff --git a/src/libstrongswan/tests/suites/test_hashtable.c b/src/libstrongswan/tests/suites/test_hashtable.c index de5c3f22e..5eadb2efe 100644 --- a/src/libstrongswan/tests/suites/test_hashtable.c +++ b/src/libstrongswan/tests/suites/test_hashtable.c @@ -309,6 +309,151 @@ START_TEST(test_remove_at_one_bucket) } END_TEST + +/******************************************************************************* + * many items + */ + +static u_int hash_int(int *key) +{ + return chunk_hash(chunk_create((u_char*)key, sizeof(int))); +} + +static bool equals_int(int *key1, int *key2) +{ + return *key1 == *key2; +} + +START_SETUP(setup_ht_many) +{ + ht = hashtable_create((hashtable_hash_t)hash_int, + (hashtable_equals_t)equals_int, 0); + ck_assert_int_eq(ht->get_count(ht), 0); +} +END_SETUP + +START_TEARDOWN(teardown_ht_many) +{ + ht->destroy_function(ht, (void*)free); +} +END_TEARDOWN + +START_TEST(test_many_items) +{ + u_int count = 250000; + int i, *val, r; + + for (i = 0; i < count; i++) + { + val = malloc_thing(int); + *val = i; + ht->put(ht, val, val); + } + for (i = 0; i < count; i++) + { + val = ht->get(ht, &i); + ck_assert_int_eq(i, *val); + } + for (i = 0; i < count; i++) + { + free(ht->remove(ht, &i)); + } + for (i = 0; i < count; i++) + { + val = malloc_thing(int); + *val = i; + ht->put(ht, val, val); + } + for (i = 0; i < count/2; i++) + { + free(ht->remove(ht, &i)); + } + for (i = 0; i < count; i++) + { + val = malloc_thing(int); + *val = i; + free(ht->put(ht, val, val)); + } + srandom(666); + for (i = 0; i < count; i++) + { + r = random() % count; + ht->get(ht, &r); + } + for (i = 0; i < count; i++) + { + free(ht->remove(ht, &i)); + } + for (i = 0; i < 2*count; i++) + { + val = malloc_thing(int); + *val = i; + ht->put(ht, val, val); + free(ht->remove(ht, val)); + } +} +END_TEST + +START_TEST(test_many_lookups_success) +{ + u_int count = 25000, lookups = 1000000; + int i, *val, r; + + for (i = 0; i < count; i++) + { + val = malloc_thing(int); + *val = i; + ht->put(ht, val, val); + } + srandom(666); + for (i = 0; i < lookups; i++) + { + r = random() % count; + ht->get(ht, &r); + } +} +END_TEST + +START_TEST(test_many_lookups_failure_larger) +{ + u_int count = 25000, lookups = 1000000; + int i, *val, r; + + for (i = 0; i < count; i++) + { + val = malloc_thing(int); + *val = i; + ht->put(ht, val, val); + } + srandom(666); + for (i = 0; i < lookups; i++) + { + r = random() % count + count; + ht->get(ht, &r); + } +} +END_TEST + +START_TEST(test_many_lookups_failure_smaller) +{ + u_int count = 25000, lookups = 1000000; + int i, *val, r; + + for (i = 0; i < count; i++) + { + val = malloc_thing(int); + *val = i + count; + ht->put(ht, val, val); + } + srandom(666); + for (i = 0; i < lookups; i++) + { + r = random() % count; + ht->get(ht, &r); + } +} +END_TEST + Suite *hashtable_suite_create() { Suite *s; @@ -342,5 +487,14 @@ Suite *hashtable_suite_create() tcase_add_test(tc, test_remove_at_one_bucket); suite_add_tcase(s, tc); + tc = tcase_create("many items"); + tcase_add_checked_fixture(tc, setup_ht_many, teardown_ht_many); + tcase_set_timeout(tc, 10); + tcase_add_test(tc, test_many_items); + tcase_add_test(tc, test_many_lookups_success); + tcase_add_test(tc, test_many_lookups_failure_larger); + tcase_add_test(tc, test_many_lookups_failure_smaller); + suite_add_tcase(s, tc); + return s; } From c66c850fc0e2c6b3c6ba5f13add8a9dda1b8efca Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 08:30:03 +0200 Subject: [PATCH 03/10] hashtable: Store items in buckets in insertion order This is more predictable when using get_match() in particular because the order does not change anymore when the table is rehashed. --- src/libstrongswan/collections/hashtable.c | 199 ++++++++++-------- .../tests/suites/test_hashtable.c | 45 +++- 2 files changed, 149 insertions(+), 95 deletions(-) diff --git a/src/libstrongswan/collections/hashtable.c b/src/libstrongswan/collections/hashtable.c index 64f154c4e..4607fad39 100644 --- a/src/libstrongswan/collections/hashtable.c +++ b/src/libstrongswan/collections/hashtable.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2014 Tobias Brunner + * Copyright (C) 2008-2020 Tobias Brunner * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it @@ -13,13 +13,16 @@ * for more details. */ - #include "hashtable.h" #include +/** The minimum capacity of the hash table (MUST be a power of 2) */ +#define MIN_CAPACITY 8 /** The maximum capacity of the hash table (MUST be a power of 2) */ #define MAX_CAPACITY (1 << 30) +/** Maximum load factor before the hash table is resized */ +#define LOAD_FACTOR 0.75f typedef struct pair_t pair_t; @@ -27,6 +30,7 @@ typedef struct pair_t pair_t; * This pair holds a pointer to the key and value it represents. */ struct pair_t { + /** * Key of a hash table item. */ @@ -71,6 +75,7 @@ typedef struct private_hashtable_t private_hashtable_t; * */ struct private_hashtable_t { + /** * Public part of hash table. */ @@ -91,11 +96,6 @@ struct private_hashtable_t { */ u_int mask; - /** - * The load factor. - */ - float load_factor; - /** * The actual table. */ @@ -205,10 +205,9 @@ static u_int get_nearest_powerof2(u_int n) */ static void init_hashtable(private_hashtable_t *this, u_int capacity) { - capacity = max(1, min(capacity, MAX_CAPACITY)); + capacity = max(MIN_CAPACITY, min(capacity, MAX_CAPACITY)); this->capacity = get_nearest_powerof2(capacity); this->mask = this->capacity - 1; - this->load_factor = 0.75; this->table = calloc(this->capacity, sizeof(pair_t*)); } @@ -218,8 +217,8 @@ static void init_hashtable(private_hashtable_t *this, u_int capacity) */ static void rehash(private_hashtable_t *this) { - pair_t **old_table; - u_int row, old_capacity; + pair_t **old_table, *to_move, *pair, *next; + u_int row, new_row, old_capacity; if (this->capacity >= MAX_CAPACITY) { @@ -233,91 +232,119 @@ static void rehash(private_hashtable_t *this) for (row = 0; row < old_capacity; row++) { - pair_t *pair, *next; - u_int new_row; - - pair = old_table[row]; - while (pair) - { /* insert pair at the front of new bucket*/ - next = pair->next; - new_row = pair->hash & this->mask; - pair->next = this->table[new_row]; - this->table[new_row] = pair; - pair = next; + to_move = old_table[row]; + while (to_move) + { + new_row = to_move->hash & this->mask; + pair = this->table[new_row]; + if (pair) + { + while (pair->next) + { + pair = pair->next; + } + pair->next = to_move; + } + else + { + this->table[new_row] = to_move; + } + next = to_move->next; + to_move->next = NULL; + to_move = next; } } free(old_table); } +/** + * Find the pair with the given key, optionally returning the hash and previous + * (or last) pair in the bucket. + */ +static inline pair_t *find_key(private_hashtable_t *this, const void *key, + hashtable_equals_t equals, u_int *out_hash, + pair_t **out_prev) +{ + pair_t *pair, *prev = NULL; + u_int hash; + + if (!this->count && !out_hash) + { /* no need to calculate the hash if not requested */ + return NULL; + } + + hash = this->hash(key); + if (out_hash) + { + *out_hash = hash; + } + + pair = this->table[hash & this->mask]; + while (pair) + { + if (hash == pair->hash && equals(key, pair->key)) + { + break; + } + prev = pair; + pair = pair->next; + } + if (out_prev) + { + *out_prev = prev; + } + return pair; +} + METHOD(hashtable_t, put, void*, private_hashtable_t *this, const void *key, void *value) { void *old_value = NULL; - pair_t *pair; - u_int hash, row; + pair_t *pair, *prev = NULL; + u_int hash; - hash = this->hash(key); - row = hash & this->mask; - pair = this->table[row]; - while (pair) - { /* search existing bucket for key */ - if (this->equals(key, pair->key)) - { - old_value = pair->value; - pair->value = value; - pair->key = key; - break; - } - pair = pair->next; - } - if (!pair) - { /* insert at the front of bucket */ - pair = pair_create(key, value, hash); - pair->next = this->table[row]; - this->table[row] = pair; - this->count++; - } - if (this->count >= this->capacity * this->load_factor) + if (this->count >= this->capacity * LOAD_FACTOR) { rehash(this); } + + pair = find_key(this, key, this->equals, &hash, &prev); + if (pair) + { + old_value = pair->value; + pair->value = value; + pair->key = key; + } + else + { + pair = pair_create(key, value, hash); + if (prev) + { + prev->next = pair; + } + else + { + this->table[hash & this->mask] = pair; + + } + this->count++; + } return old_value; } -static void *get_internal(private_hashtable_t *this, const void *key, - hashtable_equals_t equals) -{ - void *value = NULL; - pair_t *pair; - - if (!this->count) - { /* no need to calculate the hash */ - return NULL; - } - - pair = this->table[this->hash(key) & this->mask]; - while (pair) - { - if (equals(key, pair->key)) - { - value = pair->value; - break; - } - pair = pair->next; - } - return value; -} METHOD(hashtable_t, get, void*, private_hashtable_t *this, const void *key) { - return get_internal(this, key, this->equals); + pair_t *pair = find_key(this, key, this->equals, NULL, NULL); + return pair ? pair->value : NULL; } METHOD(hashtable_t, get_match, void*, private_hashtable_t *this, const void *key, hashtable_equals_t match) { - return get_internal(this, key, match); + pair_t *pair = find_key(this, key, match, NULL, NULL); + return pair ? pair->value : NULL; } METHOD(hashtable_t, remove_, void*, @@ -325,29 +352,21 @@ METHOD(hashtable_t, remove_, void*, { void *value = NULL; pair_t *pair, *prev = NULL; - u_int row; - row = this->hash(key) & this->mask; - pair = this->table[row]; - while (pair) + pair = find_key(this, key, this->equals, NULL, &prev); + if (pair) { - if (this->equals(key, pair->key)) + if (prev) { - if (prev) - { - prev->next = pair->next; - } - else - { - this->table[row] = pair->next; - } - value = pair->value; - this->count--; - free(pair); - break; + prev->next = pair->next; } - prev = pair; - pair = pair->next; + else + { + this->table[pair->hash & this->mask] = pair->next; + } + value = pair->value; + free(pair); + this->count--; } return value; } diff --git a/src/libstrongswan/tests/suites/test_hashtable.c b/src/libstrongswan/tests/suites/test_hashtable.c index 5eadb2efe..dfd38e6cf 100644 --- a/src/libstrongswan/tests/suites/test_hashtable.c +++ b/src/libstrongswan/tests/suites/test_hashtable.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2013 Tobias Brunner + * Copyright (C) 2010-2020 Tobias Brunner * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it @@ -135,6 +135,41 @@ START_TEST(test_get_match) } END_TEST +START_TEST(test_get_match_remove) +{ + char *k1 = "key1_a", *k2 = "key2", *k3 = "key1_b", *k4 = "key1_c"; + char *v1 = "val1", *v2 = "val2", *v3 = "val3", *value; + + ht = hashtable_create((hashtable_hash_t)hash_match, + (hashtable_equals_t)equals, 0); + + ht->put(ht, k1, v1); + ht->put(ht, k2, v2); + ht->put(ht, k3, v3); + ht->remove(ht, k1); + ht->put(ht, k1, v1); + ck_assert_int_eq(ht->get_count(ht), 3); + ck_assert(streq(ht->get(ht, k1), v1)); + ck_assert(streq(ht->get(ht, k2), v2)); + ck_assert(streq(ht->get(ht, k3), v3)); + + value = ht->get_match(ht, k1, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v1)); + value = ht->get_match(ht, k2, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v2)); + value = ht->get_match(ht, k3, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v3)); + value = ht->get_match(ht, k4, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v3)); + + ht->destroy(ht); +} +END_TEST + /******************************************************************************* * remove */ @@ -179,9 +214,8 @@ START_TEST(test_remove_one_bucket) char *k1 = "key1_a", *k2 = "key1_b", *k3 = "key1_c"; ht->destroy(ht); - /* set a capacity to avoid rehashing, which would change the items' order */ ht = hashtable_create((hashtable_hash_t)hash_match, - (hashtable_equals_t)equals, 8); + (hashtable_equals_t)equals, 0); do_remove(k1, k2, k3); } @@ -302,9 +336,9 @@ START_TEST(test_remove_at_one_bucket) char *k1 = "key1_a", *k2 = "key1_b", *k3 = "key1_c"; ht->destroy(ht); - /* set a capacity to avoid rehashing, which would change the items' order */ ht = hashtable_create((hashtable_hash_t)hash_match, - (hashtable_equals_t)equals, 8); + (hashtable_equals_t)equals, 0); + do_remove_at(k1, k2, k3); } END_TEST @@ -468,6 +502,7 @@ Suite *hashtable_suite_create() tc = tcase_create("get_match"); tcase_add_test(tc, test_get_match); + tcase_add_test(tc, test_get_match_remove); suite_add_tcase(s, tc); tc = tcase_create("remove"); From 87ceaefe2f69a3c33f3c766782f94c914f26c7a2 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 08:50:24 +0200 Subject: [PATCH 04/10] hashtable: Optionally sort keys/items in buckets in a specific way This can improve negative lookups, but is mostly intended to be used with get_match() so keys/items can be matched/enumerated in a specific order. It's like storing sorted linked lists under a shared key but with less memory overhead. --- src/libstrongswan/collections/hashtable.c | 137 +++++++++++++----- src/libstrongswan/collections/hashtable.h | 66 ++++++--- .../tests/suites/test_hashtable.c | 116 ++++++++++++++- 3 files changed, 253 insertions(+), 66 deletions(-) diff --git a/src/libstrongswan/collections/hashtable.c b/src/libstrongswan/collections/hashtable.c index 4607fad39..28f2bef8d 100644 --- a/src/libstrongswan/collections/hashtable.c +++ b/src/libstrongswan/collections/hashtable.c @@ -17,10 +17,10 @@ #include -/** The minimum capacity of the hash table (MUST be a power of 2) */ -#define MIN_CAPACITY 8 -/** The maximum capacity of the hash table (MUST be a power of 2) */ -#define MAX_CAPACITY (1 << 30) +/** The minimum size of the hash table (MUST be a power of 2) */ +#define MIN_SIZE 8 +/** The maximum size of the hash table (MUST be a power of 2) */ +#define MAX_SIZE (1 << 30) /** Maximum load factor before the hash table is resized */ #define LOAD_FACTOR 0.75f @@ -72,7 +72,6 @@ typedef struct private_hashtable_t private_hashtable_t; /** * Private data of a hashtable_t object. - * */ struct private_hashtable_t { @@ -87,12 +86,12 @@ struct private_hashtable_t { u_int count; /** - * The current capacity of the hash table (always a power of 2). + * The current size of the hash table (always a power of 2). */ - u_int capacity; + u_int size; /** - * The current mask to calculate the row index (capacity - 1). + * The current mask to calculate the row index (size - 1). */ u_int mask; @@ -110,6 +109,11 @@ struct private_hashtable_t { * The equality function. */ hashtable_equals_t equals; + + /** + * Alternative comparison function. + */ + hashtable_cmp_t cmp; }; typedef struct private_enumerator_t private_enumerator_t; @@ -203,13 +207,13 @@ static u_int get_nearest_powerof2(u_int n) /** * Init hash table parameters */ -static void init_hashtable(private_hashtable_t *this, u_int capacity) +static void init_hashtable(private_hashtable_t *this, u_int size) { - capacity = max(MIN_CAPACITY, min(capacity, MAX_CAPACITY)); - this->capacity = get_nearest_powerof2(capacity); - this->mask = this->capacity - 1; + size = max(MIN_SIZE, min(size, MAX_SIZE)); + this->size = get_nearest_powerof2(size); + this->mask = this->size - 1; - this->table = calloc(this->capacity, sizeof(pair_t*)); + this->table = calloc(this->size, sizeof(pair_t*)); } /** @@ -218,39 +222,48 @@ static void init_hashtable(private_hashtable_t *this, u_int capacity) static void rehash(private_hashtable_t *this) { pair_t **old_table, *to_move, *pair, *next; - u_int row, new_row, old_capacity; + u_int row, new_row, old_size; - if (this->capacity >= MAX_CAPACITY) + if (this->size >= MAX_SIZE) { return; } - old_capacity = this->capacity; + old_size = this->size; old_table = this->table; - init_hashtable(this, old_capacity << 1); + init_hashtable(this, old_size << 1); - for (row = 0; row < old_capacity; row++) + for (row = 0; row < old_size; row++) { to_move = old_table[row]; while (to_move) { + pair_t *prev = NULL; + new_row = to_move->hash & this->mask; pair = this->table[new_row]; - if (pair) + while (pair) { - while (pair->next) + if (this->cmp && this->cmp(to_move->key, pair->key) < 0) { - pair = pair->next; + break; } - pair->next = to_move; - } - else - { - this->table[new_row] = to_move; + prev = pair; + pair = pair->next; } next = to_move->next; to_move->next = NULL; + if (prev) + { + to_move->next = prev->next; + prev->next = to_move; + } + else + { + to_move->next = this->table[new_row]; + this->table[new_row] = to_move; + } to_move = next; } } @@ -266,6 +279,7 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, pair_t **out_prev) { pair_t *pair, *prev = NULL; + bool use_callback = equals != NULL; u_int hash; if (!this->count && !out_hash) @@ -273,6 +287,7 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, return NULL; } + equals = equals ?: this->equals; hash = this->hash(key); if (out_hash) { @@ -282,7 +297,23 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, pair = this->table[hash & this->mask]; while (pair) { - if (hash == pair->hash && equals(key, pair->key)) + /* when keys are ordered, we compare all items so we can abort earlier + * even if the hash does not match, but only as long as we don't + * have a callback */ + if (!use_callback && this->cmp) + { + int cmp = this->cmp(key, pair->key); + if (cmp == 0) + { + break; + } + else if (cmp < 0) + { /* no need to continue as the key we search is smaller */ + pair = NULL; + break; + } + } + else if (hash == pair->hash && equals(key, pair->key)) { break; } @@ -303,12 +334,12 @@ METHOD(hashtable_t, put, void*, pair_t *pair, *prev = NULL; u_int hash; - if (this->count >= this->capacity * LOAD_FACTOR) + if (this->count >= this->size * LOAD_FACTOR) { rehash(this); } - pair = find_key(this, key, this->equals, &hash, &prev); + pair = find_key(this, key, NULL, &hash, &prev); if (pair) { old_value = pair->value; @@ -320,12 +351,13 @@ METHOD(hashtable_t, put, void*, pair = pair_create(key, value, hash); if (prev) { + pair->next = prev->next; prev->next = pair; } else { + pair->next = this->table[hash & this->mask]; this->table[hash & this->mask] = pair; - } this->count++; } @@ -336,7 +368,7 @@ METHOD(hashtable_t, put, void*, METHOD(hashtable_t, get, void*, private_hashtable_t *this, const void *key) { - pair_t *pair = find_key(this, key, this->equals, NULL, NULL); + pair_t *pair = find_key(this, key, NULL, NULL, NULL); return pair ? pair->value : NULL; } @@ -353,7 +385,7 @@ METHOD(hashtable_t, remove_, void*, void *value = NULL; pair_t *pair, *prev = NULL; - pair = find_key(this, key, this->equals, NULL, &prev); + pair = find_key(this, key, NULL, NULL, &prev); if (pair) { if (prev) @@ -405,7 +437,7 @@ METHOD(enumerator_t, enumerate, bool, VA_ARGS_VGET(args, key, value); - while (this->count && this->row < this->table->capacity) + while (this->count && this->row < this->table->size) { this->prev = this->current; if (this->current) @@ -458,7 +490,7 @@ static void destroy_internal(private_hashtable_t *this, pair_t *pair, *next; u_int row; - for (row = 0; row < this->capacity; row++) + for (row = 0; row < this->size; row++) { pair = this->table[row]; while (pair) @@ -488,11 +520,11 @@ METHOD(hashtable_t, destroy_function, void, destroy_internal(this, fn); } -/* - * Described in header. +/** + * Create a hash table */ -hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, - u_int capacity) +static private_hashtable_t *hashtable_create_internal(hashtable_hash_t hash, + u_int size) { private_hashtable_t *this; @@ -509,10 +541,35 @@ hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, .destroy_function = _destroy_function, }, .hash = hash, - .equals = equals, ); - init_hashtable(this, capacity); + init_hashtable(this, size); + + return this; +} + +/* + * Described in header + */ +hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, + u_int size) +{ + private_hashtable_t *this = hashtable_create_internal(hash, size); + + this->equals = equals; + + return &this->public; +} + +/* + * Described in header + */ +hashtable_t *hashtable_create_sorted(hashtable_hash_t hash, + hashtable_cmp_t cmp, u_int size) +{ + private_hashtable_t *this = hashtable_create_internal(hash, size); + + this->cmp = cmp; return &this->public; } diff --git a/src/libstrongswan/collections/hashtable.h b/src/libstrongswan/collections/hashtable.h index ab217ea11..e644c461a 100644 --- a/src/libstrongswan/collections/hashtable.h +++ b/src/libstrongswan/collections/hashtable.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008-2012 Tobias Brunner + * Copyright (C) 2008-2020 Tobias Brunner * HSR Hochschule fuer Technik Rapperswil * * This program is free software; you can redistribute it and/or modify it @@ -76,10 +76,23 @@ bool hashtable_equals_ptr(const void *key, const void *other_key); */ bool hashtable_equals_str(const void *key, const void *other_key); +/** + * Prototype for a function that compares the two keys in order to sort them. + * + * @param key first key (the one we are looking for/inserting) + * @param other_key second key + * @return less than, equal to, or greater than 0 if key is + * less than, equal to, or greater than other_key + */ +typedef int (*hashtable_cmp_t)(const void *key, const void *other_key); + /** * Class implementing a hash table. * * General purpose hash table. This hash table is not synchronized. + * + * @note Any ordering only pertains to keys/items in the same bucket (with or + * without the same hash value), not to the order when enumerating. */ struct hashtable_t { @@ -88,7 +101,7 @@ struct hashtable_t { * * @return enumerator over (void *key, void *value) */ - enumerator_t *(*create_enumerator) (hashtable_t *this); + enumerator_t *(*create_enumerator)(hashtable_t *this); /** * Adds the given value with the given key to the hash table, if there @@ -100,7 +113,7 @@ struct hashtable_t { * @param value the value to store * @return NULL if no item was replaced, the old value otherwise */ - void *(*put) (hashtable_t *this, const void *key, void *value); + void *(*put)(hashtable_t *this, const void *key, void *value); /** * Returns the value with the given key, if the hash table contains such an @@ -109,24 +122,26 @@ struct hashtable_t { * @param key the key of the requested value * @return the value, NULL if not found */ - void *(*get) (hashtable_t *this, const void *key); + void *(*get)(hashtable_t *this, const void *key); /** - * Returns the value with a matching key, if the hash table contains such an - * entry, otherwise NULL is returned. + * Returns the first value with a matching key, if the hash table contains + * such an entry, otherwise NULL is returned. * * Compared to get() the given match function is used to compare the keys - * for equality. The hash function does have to be devised properly in + * for equality. The hash function does have to be devised specially in * order to make this work if the match function compares keys differently - * than the equals function provided to the constructor. This basically - * allows to enumerate all entries with the same hash value. + * than the equals/comparison function provided to the constructor. + * + * This basically allows to enumerate all entries with the same hash value + * in their key's order. * * @param key the key to match against * @param match match function to be used when comparing keys * @return the value, NULL if not found */ - void *(*get_match) (hashtable_t *this, const void *key, - hashtable_equals_t match); + void *(*get_match)(hashtable_t *this, const void *key, + hashtable_equals_t match); /** * Removes the value with the given key from the hash table and returns the @@ -135,7 +150,7 @@ struct hashtable_t { * @param key the key of the value to remove * @return the removed value, NULL if not found */ - void *(*remove) (hashtable_t *this, const void *key); + void *(*remove)(hashtable_t *this, const void *key); /** * Removes the key and value pair from the hash table at which the given @@ -143,19 +158,19 @@ struct hashtable_t { * * @param enumerator enumerator, from create_enumerator */ - void (*remove_at) (hashtable_t *this, enumerator_t *enumerator); + void (*remove_at)(hashtable_t *this, enumerator_t *enumerator); /** * Gets the number of items in the hash table. * * @return number of items */ - u_int (*get_count) (hashtable_t *this); + u_int (*get_count)(hashtable_t *this); /** * Destroys a hash table object. */ - void (*destroy) (hashtable_t *this); + void (*destroy)(hashtable_t *this); /** * Destroys a hash table object and calls the given function for each @@ -168,14 +183,27 @@ struct hashtable_t { }; /** - * Creates an empty hash table object. + * Creates an empty hash table object. Items in buckets are ordered in + * insertion order. * * @param hash hash function * @param equals equals function - * @param capacity initial capacity - * @return hashtable_t object. + * @param size initial size + * @return hashtable_t object */ hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, - u_int capacity); + u_int size); + +/** + * Creates an empty hash table object with keys in each bucket sorted according + * to the given comparison function. + * + * @param hash hash function + * @param cmp comparison function + * @param size initial size + * @return hashtable_t object. + */ +hashtable_t *hashtable_create_sorted(hashtable_hash_t hash, + hashtable_cmp_t cmp, u_int size); #endif /** HASHTABLE_H_ @}*/ diff --git a/src/libstrongswan/tests/suites/test_hashtable.c b/src/libstrongswan/tests/suites/test_hashtable.c index dfd38e6cf..25d85d09d 100644 --- a/src/libstrongswan/tests/suites/test_hashtable.c +++ b/src/libstrongswan/tests/suites/test_hashtable.c @@ -170,6 +170,43 @@ START_TEST(test_get_match_remove) } END_TEST +START_TEST(test_get_match_sorted) +{ + char *k1 = "key1_a", *k2 = "key2", *k3 = "key1_b", *k4 = "key1_c"; + char *v1 = "val1", *v2 = "val2", *v3 = "val3", *value; + + ht = hashtable_create_sorted((hashtable_hash_t)hash_match, + (hashtable_cmp_t)strcmp, 0); + + ht->put(ht, k3, v3); + ht->put(ht, k2, v2); + ht->put(ht, k1, v1); + ht->put(ht, k4, v1); + ht->remove(ht, k1); + ht->put(ht, k1, v1); + ck_assert_int_eq(ht->get_count(ht), 4); + ck_assert(streq(ht->get(ht, k1), v1)); + ck_assert(streq(ht->get(ht, k2), v2)); + ck_assert(streq(ht->get(ht, k3), v3)); + ck_assert(streq(ht->get(ht, k4), v1)); + + value = ht->get_match(ht, k1, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v1)); + value = ht->get_match(ht, k2, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v2)); + value = ht->get_match(ht, k3, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v1)); + value = ht->get_match(ht, k4, (hashtable_equals_t)equal_match); + ck_assert(value != NULL); + ck_assert(streq(value, v1)); + + ht->destroy(ht); +} +END_TEST + /******************************************************************************* * remove */ @@ -206,6 +243,8 @@ START_TEST(test_remove) char *k1 = "key1", *k2 = "key2", *k3 = "key3"; do_remove(k1, k2, k3); + do_remove(k3, k2, k1); + do_remove(k1, k3, k2); } END_TEST @@ -218,6 +257,36 @@ START_TEST(test_remove_one_bucket) (hashtable_equals_t)equals, 0); do_remove(k1, k2, k3); + do_remove(k3, k2, k1); + do_remove(k1, k3, k2); +} +END_TEST + +START_TEST(test_remove_sorted) +{ + char *k1 = "key1", *k2 = "key2", *k3 = "key3"; + + ht->destroy(ht); + ht = hashtable_create_sorted((hashtable_hash_t)hash, + (hashtable_cmp_t)strcmp, 0); + + do_remove(k1, k2, k3); + do_remove(k3, k2, k1); + do_remove(k1, k3, k2); +} +END_TEST + +START_TEST(test_remove_sorted_one_bucket) +{ + char *k1 = "key1_a", *k2 = "key1_b", *k3 = "key1_c"; + + ht->destroy(ht); + ht = hashtable_create_sorted((hashtable_hash_t)hash_match, + (hashtable_cmp_t)strcmp, 0); + + do_remove(k1, k2, k3); + do_remove(k3, k2, k1); + do_remove(k1, k3, k2); } END_TEST @@ -358,6 +427,11 @@ static bool equals_int(int *key1, int *key2) return *key1 == *key2; } +static int cmp_int(int *key1, int *key2) +{ + return *key1 - *key2; +} + START_SETUP(setup_ht_many) { ht = hashtable_create((hashtable_hash_t)hash_int, @@ -366,6 +440,14 @@ START_SETUP(setup_ht_many) } END_SETUP +START_SETUP(setup_ht_many_cmp) +{ + ht = hashtable_create_sorted((hashtable_hash_t)hash_int, + (hashtable_cmp_t)cmp_int, 0); + ck_assert_int_eq(ht->get_count(ht), 0); +} +END_SETUP + START_TEARDOWN(teardown_ht_many) { ht->destroy_function(ht, (void*)free); @@ -377,35 +459,42 @@ START_TEST(test_many_items) u_int count = 250000; int i, *val, r; +#define GET_VALUE(i) ({ _i == 0 ? i : (count-1-i); }) + for (i = 0; i < count; i++) { val = malloc_thing(int); - *val = i; + *val = GET_VALUE(i); ht->put(ht, val, val); } for (i = 0; i < count; i++) { - val = ht->get(ht, &i); - ck_assert_int_eq(i, *val); + r = GET_VALUE(i); + val = ht->get(ht, &r); + ck_assert_int_eq(GET_VALUE(i), *val); } + ck_assert_int_eq(count, ht->get_count(ht)); for (i = 0; i < count; i++) { - free(ht->remove(ht, &i)); + r = GET_VALUE(i); + free(ht->remove(ht, &r)); } + ck_assert_int_eq(0, ht->get_count(ht)); for (i = 0; i < count; i++) { val = malloc_thing(int); - *val = i; + *val = GET_VALUE(i); ht->put(ht, val, val); } for (i = 0; i < count/2; i++) { free(ht->remove(ht, &i)); } + ck_assert_int_eq(count/2, ht->get_count(ht)); for (i = 0; i < count; i++) { val = malloc_thing(int); - *val = i; + *val = GET_VALUE(i); free(ht->put(ht, val, val)); } srandom(666); @@ -418,6 +507,7 @@ START_TEST(test_many_items) { free(ht->remove(ht, &i)); } + ck_assert_int_eq(0, ht->get_count(ht)); for (i = 0; i < 2*count; i++) { val = malloc_thing(int); @@ -503,12 +593,15 @@ Suite *hashtable_suite_create() tc = tcase_create("get_match"); tcase_add_test(tc, test_get_match); tcase_add_test(tc, test_get_match_remove); + tcase_add_test(tc, test_get_match_sorted); suite_add_tcase(s, tc); tc = tcase_create("remove"); tcase_add_checked_fixture(tc, setup_ht, teardown_ht); tcase_add_test(tc, test_remove); tcase_add_test(tc, test_remove_one_bucket); + tcase_add_test(tc, test_remove_sorted); + tcase_add_test(tc, test_remove_sorted_one_bucket); suite_add_tcase(s, tc); tc = tcase_create("enumerator"); @@ -525,7 +618,16 @@ Suite *hashtable_suite_create() tc = tcase_create("many items"); tcase_add_checked_fixture(tc, setup_ht_many, teardown_ht_many); tcase_set_timeout(tc, 10); - tcase_add_test(tc, test_many_items); + tcase_add_loop_test(tc, test_many_items, 0, 2); + tcase_add_test(tc, test_many_lookups_success); + tcase_add_test(tc, test_many_lookups_failure_larger); + tcase_add_test(tc, test_many_lookups_failure_smaller); + suite_add_tcase(s, tc); + + tc = tcase_create("many items sorted"); + tcase_add_checked_fixture(tc, setup_ht_many_cmp, teardown_ht_many); + tcase_set_timeout(tc, 10); + tcase_add_loop_test(tc, test_many_items, 0, 2); tcase_add_test(tc, test_many_lookups_success); tcase_add_test(tc, test_many_lookups_failure_larger); tcase_add_test(tc, test_many_lookups_failure_smaller); From 31e6ca78dffd0b6564c9188a2e67aaa8dcef2bd4 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 10:56:50 +0200 Subject: [PATCH 05/10] hashtable: Optionally collect and report profiling data --- src/libstrongswan/collections/hashtable.c | 94 +++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/src/libstrongswan/collections/hashtable.c b/src/libstrongswan/collections/hashtable.c index 28f2bef8d..c89bf65e1 100644 --- a/src/libstrongswan/collections/hashtable.c +++ b/src/libstrongswan/collections/hashtable.c @@ -16,6 +16,10 @@ #include "hashtable.h" #include +#include +#ifdef HASHTABLE_PROFILER +#include +#endif /** The minimum size of the hash table (MUST be a power of 2) */ #define MIN_SIZE 8 @@ -114,8 +118,64 @@ struct private_hashtable_t { * Alternative comparison function. */ hashtable_cmp_t cmp; + +#ifdef HASHTABLE_PROFILER + /** + * Some stats to profile lookups in the table + */ + struct { + size_t count; + size_t probes; + size_t longest; + } success, failure; + + /** + * Stats on the memory usage of the table + */ + struct { + size_t count; + size_t size; + } max; + + /** + * Keep track of where the hash table was created + */ + backtrace_t *backtrace; +#endif }; +#ifdef HASHTABLE_PROFILER + +#define lookup_start() \ + u_int _lookup_probes = 0; + +#define lookup_probing() \ + _lookup_probes++; + +#define _lookup_done(table, result) \ + table->result.count++; \ + table->result.probes += _lookup_probes; \ + table->result.longest = max(table->result.longest, _lookup_probes); + +#define lookup_success(table) _lookup_done(table, success); +#define lookup_failure(table) _lookup_done(table, failure); + +#define profile_size(table) \ + table->max.size = max(table->max.size, table->size); +#define profile_count(table) \ + table->max.count = max(table->max.count, table->count); + +#else + +#define lookup_start(...) {} +#define lookup_probing(...) {} +#define lookup_success(...) {} +#define lookup_failure(...) {} +#define profile_size(...) {} +#define profile_count(...) {} + +#endif + typedef struct private_enumerator_t private_enumerator_t; /** @@ -212,6 +272,7 @@ static void init_hashtable(private_hashtable_t *this, u_int size) size = max(MIN_SIZE, min(size, MAX_SIZE)); this->size = get_nearest_powerof2(size); this->mask = this->size - 1; + profile_size(this); this->table = calloc(this->size, sizeof(pair_t*)); } @@ -294,9 +355,12 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, *out_hash = hash; } + lookup_start(); + pair = this->table[hash & this->mask]; while (pair) { + lookup_probing(); /* when keys are ordered, we compare all items so we can abort earlier * even if the hash does not match, but only as long as we don't * have a callback */ @@ -324,6 +388,14 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, { *out_prev = prev; } + if (pair) + { + lookup_success(this); + } + else + { + lookup_failure(this); + } return pair; } @@ -360,6 +432,7 @@ METHOD(hashtable_t, put, void*, this->table[hash & this->mask] = pair; } this->count++; + profile_count(this); } return old_value; } @@ -490,6 +563,23 @@ static void destroy_internal(private_hashtable_t *this, pair_t *pair, *next; u_int row; +#ifdef HASHTABLE_PROFILER + if (this->success.count || this->failure.count) + { + fprintf(stderr, "%zu elements [max. %zu], %zu buckets [%zu], %zu " + "successful / %zu failed lookups, %.4f [%zu] / %.4f " + "[%zu] avg. probes in table created at:", + this->count, this->max.count, this->size, this->max.size, + this->success.count, this->failure.count, + (double)this->success.probes/this->success.count, + this->success.longest, + (double)this->failure.probes/this->failure.count, + this->failure.longest); + this->backtrace->log(this->backtrace, stderr, TRUE); + } + this->backtrace->destroy(this->backtrace); +#endif + for (row = 0; row < this->size; row++) { pair = this->table[row]; @@ -545,6 +635,10 @@ static private_hashtable_t *hashtable_create_internal(hashtable_hash_t hash, init_hashtable(this, size); +#ifdef HASHTABLE_PROFILER + this->backtrace = backtrace_create(3); +#endif + return this; } From 4334f61284b18a539d6b900f67955134078224fe Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 14:45:21 +0200 Subject: [PATCH 06/10] unit-tests: Pass test iteration to fixtures --- src/libstrongswan/tests/test_runner.c | 12 ++++++------ src/libstrongswan/tests/test_suite.h | 6 +++--- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/libstrongswan/tests/test_runner.c b/src/libstrongswan/tests/test_runner.c index 603495c13..1e7f3ce1f 100644 --- a/src/libstrongswan/tests/test_runner.c +++ b/src/libstrongswan/tests/test_runner.c @@ -215,7 +215,7 @@ static bool run_test(test_function_t *tfun, int i) /** * Invoke fixture setup/teardown */ -static bool call_fixture(test_case_t *tcase, bool up) +static bool call_fixture(test_case_t *tcase, bool up, int i) { enumerator_t *enumerator; test_fixture_t *fixture; @@ -230,14 +230,14 @@ static bool call_fixture(test_case_t *tcase, bool up) { if (fixture->setup) { - fixture->setup(); + fixture->setup(i); } } else { if (fixture->teardown) { - fixture->teardown(); + fixture->teardown(i); } } } @@ -519,18 +519,18 @@ static bool run_case(test_case_t *tcase, test_runner_init_t init, char *cfg) test_setup_timeout(tcase->timeout); start_timing(&start); - if (call_fixture(tcase, TRUE)) + if (call_fixture(tcase, TRUE, i)) { if (run_test(tfun, i)) { - if (call_fixture(tcase, FALSE)) + if (call_fixture(tcase, FALSE, i)) { ok = TRUE; } } else { - call_fixture(tcase, FALSE); + call_fixture(tcase, FALSE, i); } } if (!post_test(init, ok, failures, tfun->name, i, &leaks)) diff --git a/src/libstrongswan/tests/test_suite.h b/src/libstrongswan/tests/test_suite.h index 3bc3b38ca..a35e9ec63 100644 --- a/src/libstrongswan/tests/test_suite.h +++ b/src/libstrongswan/tests/test_suite.h @@ -51,7 +51,7 @@ typedef void (*test_function_cb_t)(int); /** * Fixture for a test case. */ -typedef void (*test_fixture_cb_t)(void); +typedef void (*test_fixture_cb_t)(int); /** * A test suite; a collection of test cases with fixtures @@ -388,9 +388,9 @@ void test_fail_if_worker_failed(); #define suite_add_tcase test_suite_add_case #define START_TEST(name) static void name (int _i) { #define END_TEST test_fail_if_worker_failed(); } -#define START_SETUP(name) static void name() { +#define START_SETUP(name) static void name(int _i) { #define END_SETUP test_fail_if_worker_failed(); } -#define START_TEARDOWN(name) static void name() { +#define START_TEARDOWN(name) static void name(int _i) { #define END_TEARDOWN test_fail_if_worker_failed(); } #endif /** TEST_SUITE_H_ @}*/ From d9944102f53ae4a48c03be89fa1da0544df518b9 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 13:41:53 +0200 Subject: [PATCH 07/10] hashlist: Move get_match() and sorting into a separate class The main intention here is that we can change the hashtable_t implementation without being impeded by the special requirements imposed by get_match() and sorting the keys/items in buckets. --- .../kernel_netlink/kernel_netlink_net.c | 49 ++-- .../kernel_pfroute/kernel_pfroute_net.c | 17 +- src/libstrongswan/collections/hashtable.c | 187 ++++++++++--- src/libstrongswan/collections/hashtable.h | 86 ++++-- src/libstrongswan/plugins/plugin_loader.c | 14 +- .../tests/suites/test_hashtable.c | 257 ++++++++++-------- 6 files changed, 386 insertions(+), 224 deletions(-) diff --git a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c index e40697214..f174c2698 100644 --- a/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c +++ b/src/libcharon/plugins/kernel_netlink/kernel_netlink_net.c @@ -330,12 +330,12 @@ struct private_kernel_netlink_net_t { /** * Map for IP addresses to iface_entry_t objects (addr_map_entry_t) */ - hashtable_t *addrs; + hashlist_t *addrs; /** * Map for virtual IP addresses to iface_entry_t objects (addr_map_entry_t) */ - hashtable_t *vips; + hashlist_t *vips; /** * netlink rt socket (routing) @@ -375,7 +375,7 @@ struct private_kernel_netlink_net_t { /** * installed routes */ - hashtable_t *routes; + hashlist_t *routes; /** * mutex for routes @@ -499,7 +499,7 @@ static job_requeue_t reinstall_routes(private_kernel_netlink_net_t *this) this->net_changes_lock->lock(this->net_changes_lock); this->routes_lock->lock(this->routes_lock); - enumerator = this->routes->create_enumerator(this->routes); + enumerator = this->routes->ht.create_enumerator(&this->routes->ht); while (enumerator->enumerate(enumerator, NULL, (void**)&route)) { net_change_t *change, lookup = { @@ -617,7 +617,7 @@ static bool is_known_vip(private_kernel_netlink_net_t *this, host_t *ip) /** * Add an address map entry */ -static void addr_map_entry_add(hashtable_t *map, addr_entry_t *addr, +static void addr_map_entry_add(hashlist_t *map, addr_entry_t *addr, iface_entry_t *iface) { addr_map_entry_t *entry; @@ -627,14 +627,14 @@ static void addr_map_entry_add(hashtable_t *map, addr_entry_t *addr, .addr = addr, .iface = iface, ); - entry = map->put(map, entry, entry); + entry = map->ht.put(&map->ht, entry, entry); free(entry); } /** * Remove an address map entry */ -static void addr_map_entry_remove(hashtable_t *map, addr_entry_t *addr, +static void addr_map_entry_remove(hashlist_t *map, addr_entry_t *addr, iface_entry_t *iface) { addr_map_entry_t *entry, lookup = { @@ -643,7 +643,7 @@ static void addr_map_entry_remove(hashtable_t *map, addr_entry_t *addr, .iface = iface, }; - entry = map->remove(map, &lookup); + entry = map->ht.remove(&map->ht, &lookup); free(entry); } @@ -1241,7 +1241,7 @@ static void process_addr(private_kernel_netlink_net_t *this, }; addr_entry_t *addr; - entry = this->vips->get(this->vips, &lookup); + entry = this->vips->ht.get(&this->vips->ht, &lookup); if (entry) { if (hdr->nlmsg_type == RTM_NEWADDR) @@ -1261,7 +1261,7 @@ static void process_addr(private_kernel_netlink_net_t *this, host->destroy(host); return; } - entry = this->addrs->get(this->addrs, &lookup); + entry = this->addrs->ht.get(&this->addrs->ht, &lookup); if (entry) { if (hdr->nlmsg_type == RTM_DELADDR) @@ -2726,7 +2726,7 @@ METHOD(kernel_net_t, add_route, status_t, } this->routes_lock->lock(this->routes_lock); - found = this->routes->get(this->routes, &lookup.route); + found = this->routes->ht.get(&this->routes->ht, &lookup.route); if (found) { this->routes_lock->unlock(this->routes_lock); @@ -2755,7 +2755,7 @@ METHOD(kernel_net_t, add_route, status_t, if (status == SUCCESS) { found = route_entry_clone(&lookup.route); - this->routes->put(this->routes, found, found); + this->routes->ht.put(&this->routes->ht, found, found); } this->routes_lock->unlock(this->routes_lock); return status; @@ -2785,7 +2785,7 @@ METHOD(kernel_net_t, del_route, status_t, } this->routes_lock->lock(this->routes_lock); - found = this->routes->remove(this->routes, &lookup.route); + found = this->routes->ht.remove(&this->routes->ht, &lookup.route); if (!found) { this->routes_lock->unlock(this->routes_lock); @@ -3023,18 +3023,9 @@ static void check_kernel_features(private_kernel_netlink_net_t *this) /** * Destroy an address to iface map */ -static void addr_map_destroy(hashtable_t *map) +static void addr_map_destroy(hashlist_t *map) { - enumerator_t *enumerator; - addr_map_entry_t *addr; - - enumerator = map->create_enumerator(map); - while (enumerator->enumerate(enumerator, NULL, (void**)&addr)) - { - free(addr); - } - enumerator->destroy(enumerator); - map->destroy(map); + map->ht.destroy_function(&map->ht, (void*)free); } METHOD(kernel_net_t, destroy, void, @@ -3055,7 +3046,7 @@ METHOD(kernel_net_t, destroy, void, lib->watcher->remove(lib->watcher, this->socket_events); close(this->socket_events); } - enumerator = this->routes->create_enumerator(this->routes); + enumerator = this->routes->ht.create_enumerator(&this->routes->ht); while (enumerator->enumerate(enumerator, NULL, (void**)&route)) { manage_srcroute(this, RTM_DELROUTE, 0, route->dst_net, route->prefixlen, @@ -3112,15 +3103,15 @@ kernel_netlink_net_t *kernel_netlink_net_create() lib->settings->get_bool(lib->settings, "%s.plugins.kernel-netlink.parallel_route", FALSE, lib->ns)), .rt_exclude = linked_list_create(), - .routes = hashtable_create((hashtable_hash_t)route_entry_hash, - (hashtable_equals_t)route_entry_equals, 16), + .routes = hashlist_create((hashtable_hash_t)route_entry_hash, + (hashtable_equals_t)route_entry_equals, 16), .net_changes = hashtable_create( (hashtable_hash_t)net_change_hash, (hashtable_equals_t)net_change_equals, 16), - .addrs = hashtable_create( + .addrs = hashlist_create( (hashtable_hash_t)addr_map_entry_hash, (hashtable_equals_t)addr_map_entry_equals, 16), - .vips = hashtable_create((hashtable_hash_t)addr_map_entry_hash, + .vips = hashlist_create((hashtable_hash_t)addr_map_entry_hash, (hashtable_equals_t)addr_map_entry_equals, 16), .routes_lock = mutex_create(MUTEX_TYPE_DEFAULT), .net_changes_lock = mutex_create(MUTEX_TYPE_DEFAULT), diff --git a/src/libcharon/plugins/kernel_pfroute/kernel_pfroute_net.c b/src/libcharon/plugins/kernel_pfroute/kernel_pfroute_net.c index d28a6aa1f..97eeda7d3 100644 --- a/src/libcharon/plugins/kernel_pfroute/kernel_pfroute_net.c +++ b/src/libcharon/plugins/kernel_pfroute/kernel_pfroute_net.c @@ -323,7 +323,7 @@ struct private_kernel_pfroute_net_t /** * Map for IP addresses to iface_entry_t objects (addr_map_entry_t) */ - hashtable_t *addrs; + hashlist_t *addrs; /** * List of tun devices we installed for virtual IPs @@ -524,7 +524,7 @@ static void addr_map_entry_add(private_kernel_pfroute_net_t *this, .addr = addr, .iface = iface, ); - entry = this->addrs->put(this->addrs, entry, entry); + entry = this->addrs->ht.put(&this->addrs->ht, entry, entry); free(entry); } @@ -541,7 +541,7 @@ static void addr_map_entry_remove(addr_entry_t *addr, iface_entry_t *iface, .iface = iface, }; - entry = this->addrs->remove(this->addrs, &lookup); + entry = this->addrs->ht.remove(&this->addrs->ht, &lookup); free(entry); } @@ -2013,7 +2013,6 @@ METHOD(kernel_net_t, destroy, void, { enumerator_t *enumerator; route_entry_t *route; - addr_entry_t *addr; enumerator = this->routes->create_enumerator(this->routes); while (enumerator->enumerate(enumerator, NULL, (void**)&route)) @@ -2036,13 +2035,7 @@ METHOD(kernel_net_t, destroy, void, this->net_changes->destroy(this->net_changes); this->net_changes_lock->destroy(this->net_changes_lock); - enumerator = this->addrs->create_enumerator(this->addrs); - while (enumerator->enumerate(enumerator, NULL, (void**)&addr)) - { - free(addr); - } - enumerator->destroy(enumerator); - this->addrs->destroy(this->addrs); + this->addrs->ht.destroy_function(&this->addrs->ht, (void*)free); this->ifaces->destroy_function(this->ifaces, (void*)iface_entry_destroy); this->tuns->destroy(this->tuns); this->lock->destroy(this->lock); @@ -2078,7 +2071,7 @@ kernel_pfroute_net_t *kernel_pfroute_net_create() }, .pid = getpid(), .ifaces = linked_list_create(), - .addrs = hashtable_create( + .addrs = hashlist_create( (hashtable_hash_t)addr_map_entry_hash, (hashtable_equals_t)addr_map_entry_equals, 16), .routes = hashtable_create((hashtable_hash_t)route_entry_hash, diff --git a/src/libstrongswan/collections/hashtable.c b/src/libstrongswan/collections/hashtable.c index c89bf65e1..e569e581e 100644 --- a/src/libstrongswan/collections/hashtable.c +++ b/src/libstrongswan/collections/hashtable.c @@ -144,6 +144,25 @@ struct private_hashtable_t { #endif }; +typedef struct private_hashlist_t private_hashlist_t; + +/** + * Private data of a hashlist_t object. + */ +struct private_hashlist_t { + + /** + * Public part of hash table. + */ + hashlist_t public; + + /** + * Inherited private part of hash table (we get the public part too, but + * ignore it). + */ + private_hashtable_t super; +}; + #ifdef HASHTABLE_PROFILER #define lookup_start() \ @@ -361,7 +380,7 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, while (pair) { lookup_probing(); - /* when keys are ordered, we compare all items so we can abort earlier + /* when keys are sorted, we compare all items so we can abort earlier * even if the hash does not match, but only as long as we don't * have a callback */ if (!use_callback && this->cmp) @@ -445,13 +464,6 @@ METHOD(hashtable_t, get, void*, return pair ? pair->value : NULL; } -METHOD(hashtable_t, get_match, void*, - private_hashtable_t *this, const void *key, hashtable_equals_t match) -{ - pair_t *pair = find_key(this, key, match, NULL, NULL); - return pair ? pair->value : NULL; -} - METHOD(hashtable_t, remove_, void*, private_hashtable_t *this, const void *key) { @@ -595,51 +607,77 @@ static void destroy_internal(private_hashtable_t *this, } } free(this->table); - free(this); } METHOD(hashtable_t, destroy, void, private_hashtable_t *this) { destroy_internal(this, NULL); + free(this); } METHOD(hashtable_t, destroy_function, void, private_hashtable_t *this, void (*fn)(void*,const void*)) { destroy_internal(this, fn); + free(this); } -/** - * Create a hash table - */ -static private_hashtable_t *hashtable_create_internal(hashtable_hash_t hash, - u_int size) +METHOD(hashtable_t, create_enumerator_hashlist, enumerator_t*, + private_hashlist_t *this) { - private_hashtable_t *this; + return create_enumerator(&this->super); +} - INIT(this, - .public = { - .put = _put, - .get = _get, - .get_match = _get_match, - .remove = _remove_, - .remove_at = (void*)_remove_at, - .get_count = _get_count, - .create_enumerator = _create_enumerator, - .destroy = _destroy, - .destroy_function = _destroy_function, - }, - .hash = hash, - ); +METHOD(hashtable_t, put_hashlist, void*, + private_hashlist_t *this, const void *key, void *value) +{ + return put(&this->super, key, value); +} - init_hashtable(this, size); +METHOD(hashtable_t, get_hashlist, void*, + private_hashlist_t *this, const void *key) +{ + return get(&this->super, key); +} -#ifdef HASHTABLE_PROFILER - this->backtrace = backtrace_create(3); -#endif +METHOD(hashlist_t, get_match, void*, + private_hashlist_t *this, const void *key, hashtable_equals_t match) +{ + pair_t *pair = find_key(&this->super, key, match, NULL, NULL); + return pair ? pair->value : NULL; +} - return this; +METHOD(hashtable_t, remove_hashlist, void*, + private_hashlist_t *this, const void *key) +{ + return remove_(&this->super, key); +} + +METHOD(hashtable_t, remove_at_hashlist, void, + private_hashlist_t *this, private_enumerator_t *enumerator) +{ + remove_at(&this->super, enumerator); +} + +METHOD(hashtable_t, get_count_hashlist, u_int, + private_hashlist_t *this) +{ + return get_count(&this->super); +} + +METHOD2(hashtable_t, hashlist_t, destroy_hashlist, void, + private_hashlist_t *this) +{ + destroy_internal(&this->super, NULL); + free(this); +} + +METHOD(hashtable_t, destroy_function_hashlist, void, + private_hashlist_t *this, void (*fn)(void*,const void*)) +{ + destroy_internal(&this->super, fn); + free(this); } /* @@ -648,9 +686,78 @@ static private_hashtable_t *hashtable_create_internal(hashtable_hash_t hash, hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, u_int size) { - private_hashtable_t *this = hashtable_create_internal(hash, size); + private_hashtable_t *this; - this->equals = equals; + INIT(this, + .public = { + .put = _put, + .get = _get, + .remove = _remove_, + .remove_at = (void*)_remove_at, + .get_count = _get_count, + .create_enumerator = _create_enumerator, + .destroy = _destroy, + .destroy_function = _destroy_function, + }, + .hash = hash, + .equals = equals, + ); + + init_hashtable(this, size); + +#ifdef HASHTABLE_PROFILER + this->backtrace = backtrace_create(3); +#endif + + return &this->public; +} + +/** + * Create a hash table + */ +static private_hashlist_t *hashlist_create_internal(hashtable_hash_t hash, + u_int size) +{ + private_hashlist_t *this; + + INIT(this, + .public = { + .ht = { + .put = _put_hashlist, + .get = _get_hashlist, + .remove = _remove_hashlist, + .remove_at = (void*)_remove_at_hashlist, + .get_count = _get_count_hashlist, + .create_enumerator = _create_enumerator_hashlist, + .destroy = _destroy_hashlist, + .destroy_function = _destroy_function_hashlist, + }, + .get_match = _get_match, + .destroy = _destroy_hashlist, + }, + .super = { + .hash = hash, + } + ); + + init_hashtable(&this->super, size); + +#ifdef HASHTABLE_PROFILER + this->super.backtrace = backtrace_create(3); +#endif + + return this; +} + +/* + * Described in header + */ +hashlist_t *hashlist_create(hashtable_hash_t hash, hashtable_equals_t equals, + u_int size) +{ + private_hashlist_t *this = hashlist_create_internal(hash, size); + + this->super.equals = equals; return &this->public; } @@ -658,12 +765,12 @@ hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, /* * Described in header */ -hashtable_t *hashtable_create_sorted(hashtable_hash_t hash, - hashtable_cmp_t cmp, u_int size) +hashlist_t *hashlist_create_sorted(hashtable_hash_t hash, + hashtable_cmp_t cmp, u_int size) { - private_hashtable_t *this = hashtable_create_internal(hash, size); + private_hashlist_t *this = hashlist_create_internal(hash, size); - this->cmp = cmp; + this->super.cmp = cmp; return &this->public; } diff --git a/src/libstrongswan/collections/hashtable.h b/src/libstrongswan/collections/hashtable.h index e644c461a..de0284920 100644 --- a/src/libstrongswan/collections/hashtable.h +++ b/src/libstrongswan/collections/hashtable.h @@ -24,6 +24,7 @@ #include typedef struct hashtable_t hashtable_t; +typedef struct hashlist_t hashlist_t; /** * Prototype for a function that computes the hash code from the given key. @@ -124,25 +125,6 @@ struct hashtable_t { */ void *(*get)(hashtable_t *this, const void *key); - /** - * Returns the first value with a matching key, if the hash table contains - * such an entry, otherwise NULL is returned. - * - * Compared to get() the given match function is used to compare the keys - * for equality. The hash function does have to be devised specially in - * order to make this work if the match function compares keys differently - * than the equals/comparison function provided to the constructor. - * - * This basically allows to enumerate all entries with the same hash value - * in their key's order. - * - * @param key the key to match against - * @param match match function to be used when comparing keys - * @return the value, NULL if not found - */ - void *(*get_match)(hashtable_t *this, const void *key, - hashtable_equals_t match); - /** * Removes the value with the given key from the hash table and returns the * removed value (or NULL if no such value existed). @@ -183,8 +165,52 @@ struct hashtable_t { }; /** - * Creates an empty hash table object. Items in buckets are ordered in - * insertion order. + * Class implementing a hash table with ordered keys/items and special query + * method. + * + * @note The ordering only pertains to keys/items in the same bucket (with or + * without the same hash value), not to the order when enumerating. + * + * This is intended to be used with hash functions that intentionally return the + * same hash value for different keys so multiple items can be retrieved for a + * key. + * + * It's like storing sorted linked lists in a hash table but with less overhead. + */ +struct hashlist_t { + + /** + * Implements the hash table interface. + */ + hashtable_t ht; + + /** + * Returns the first value with a matching key if the hash table contains + * such an entry, otherwise NULL is returned. + * + * Compared to get() the given match function is used to compare the keys + * for equality. The hash function does have to be devised specially in + * order to make this work if the match function compares keys differently + * than the equals/comparison function provided to the constructor. + * + * This basically allows to enumerate all entries with the same hash value + * in their key's order. + * + * @param key the key to match against + * @param match match function to be used when comparing keys + * @return the value, NULL if not found + */ + void *(*get_match)(hashlist_t *this, const void *key, + hashtable_equals_t match); + + /** + * Destroys a hash list object. + */ + void (*destroy)(hashlist_t *this); +}; + +/** + * Creates an empty hash table object. * * @param hash hash function * @param equals equals function @@ -195,15 +221,25 @@ hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, u_int size); /** - * Creates an empty hash table object with keys in each bucket sorted according - * to the given comparison function. + * Creates an empty hash list object with each bucket's keys in insertion order. + * + * @param hash hash function + * @param equals equals function + * @param size initial size + * @return hashtable_t object + */ +hashlist_t *hashlist_create(hashtable_hash_t hash, hashtable_equals_t equals, + u_int size); + +/** + * Creates an empty hash list object with sorted keys in each bucket. * * @param hash hash function * @param cmp comparison function * @param size initial size * @return hashtable_t object. */ -hashtable_t *hashtable_create_sorted(hashtable_hash_t hash, - hashtable_cmp_t cmp, u_int size); +hashlist_t *hashlist_create_sorted(hashtable_hash_t hash, + hashtable_cmp_t cmp, u_int size); #endif /** HASHTABLE_H_ @}*/ diff --git a/src/libstrongswan/plugins/plugin_loader.c b/src/libstrongswan/plugins/plugin_loader.c index b694e97cd..8e70e5fe8 100644 --- a/src/libstrongswan/plugins/plugin_loader.c +++ b/src/libstrongswan/plugins/plugin_loader.c @@ -65,7 +65,7 @@ struct private_plugin_loader_t { /** * Hashtable for registered features, as registered_feature_t */ - hashtable_t *features; + hashlist_t *features; /** * Loaded features (stored in reverse order), as provided_feature_t @@ -911,14 +911,16 @@ static void register_features(private_plugin_loader_t *this, { case FEATURE_PROVIDE: lookup.feature = feature; - registered = this->features->get(this->features, &lookup); + registered = this->features->ht.get(&this->features->ht, + &lookup); if (!registered) { INIT(registered, .feature = feature, .plugins = linked_list_create(), ); - this->features->put(this->features, registered, registered); + this->features->ht.put(&this->features->ht, registered, + registered); } INIT(provided, .entry = entry, @@ -950,13 +952,13 @@ static void unregister_feature(private_plugin_loader_t *this, registered_feature_t *registered, lookup; lookup.feature = provided->feature; - registered = this->features->get(this->features, &lookup); + registered = this->features->ht.get(&this->features->ht, &lookup); if (registered) { registered->plugins->remove(registered->plugins, provided, NULL); if (registered->plugins->get_count(registered->plugins) == 0) { - this->features->remove(this->features, &lookup); + this->features->ht.remove(&this->features->ht, &lookup); registered->plugins->destroy(registered->plugins); free(registered); } @@ -1444,7 +1446,7 @@ plugin_loader_t *plugin_loader_create() }, .plugins = linked_list_create(), .loaded = linked_list_create(), - .features = hashtable_create( + .features = hashlist_create( (hashtable_hash_t)registered_feature_hash, (hashtable_equals_t)registered_feature_equals, 64), ); diff --git a/src/libstrongswan/tests/suites/test_hashtable.c b/src/libstrongswan/tests/suites/test_hashtable.c index 25d85d09d..efc672e63 100644 --- a/src/libstrongswan/tests/suites/test_hashtable.c +++ b/src/libstrongswan/tests/suites/test_hashtable.c @@ -19,17 +19,22 @@ #include /******************************************************************************* - * string hash table functions + * hash table functions */ -static u_int hash(char *key) +static u_int hash_match(char *key) { - return chunk_hash(chunk_from_str(key)); + return chunk_hash(chunk_create(key, 4)); } -static bool equals(char *key1, char *key2) +static bool equal_match(char *key1, char *key2) { - return streq(key1, key2); + if (!strneq(key1, key2, 4)) + { + return FALSE; + } + /* look for an item with a key < than what we look for */ + return strcmp(key1, key2) >= 0; } /******************************************************************************* @@ -38,17 +43,77 @@ static bool equals(char *key1, char *key2) static hashtable_t *ht; +typedef enum { + /* regular string hash table */ + HASHTABLE_REGULAR, + /* regular string hash list */ + HASHLIST_REGULAR, + /* sorted string hash list */ + HASHLIST_REGULAR_SORTED, + REGULAR_MAX, + /* hash table with only 4 characters hashed -> one bucket tests */ + HASHTABLE_FUZZY = REGULAR_MAX, + /* hash list with only 4 characters hashed */ + HASHLIST_FUZZY, + /* sorted string hash list with only 4 characters hashed */ + HASHLIST_FUZZY_SORTED, + HASHTABLE_MAX, +} hashtable_type_t; + +/** + * Create a specific hash table/list + */ +static hashtable_t *create_hashtable(int i) +{ + hashlist_t *hl = NULL; + + DESTROY_IF(ht); + + switch (i) + { + case HASHTABLE_REGULAR: + ht = hashtable_create(hashtable_hash_str, + hashtable_equals_str, 0); + break; + case HASHLIST_REGULAR: + hl = hashlist_create(hashtable_hash_str, + hashtable_equals_str, 0); + break; + case HASHLIST_REGULAR_SORTED: + hl = hashlist_create_sorted(hashtable_hash_str, + (hashtable_cmp_t)strcmp, 0); + break; + case HASHTABLE_FUZZY: + ht = hashtable_create((hashtable_hash_t)hash_match, + hashtable_equals_str, 0); + break; + case HASHLIST_FUZZY: + hl = hashlist_create((hashtable_hash_t)hash_match, + hashtable_equals_str, 0); + break; + case HASHLIST_FUZZY_SORTED: + hl = hashlist_create_sorted((hashtable_hash_t)hash_match, + (hashtable_cmp_t)strcmp, 0); + break; + } + if (hl) + { + ht = &hl->ht; + } + ck_assert_int_eq(ht->get_count(ht), 0); + return ht; +} + START_SETUP(setup_ht) { - ht = hashtable_create((hashtable_hash_t)hash, - (hashtable_equals_t)equals, 0); - ck_assert_int_eq(ht->get_count(ht), 0); + create_hashtable(_i); } END_SETUP START_TEARDOWN(teardown_ht) { ht->destroy(ht); + ht = NULL; } END_TEARDOWN @@ -86,28 +151,13 @@ END_TEST * get_match */ -static u_int hash_match(char *key) -{ - return chunk_hash(chunk_create(key, 4)); -} - -static bool equal_match(char *key1, char *key2) -{ - if (!strneq(key1, key2, 4)) - { - return FALSE; - } - /* look for an item with a key < than what we look for */ - return strcmp(key1, key2) >= 0; -} - START_TEST(test_get_match) { + hashlist_t *hl; char *k1 = "key1_a", *k2 = "key2", *k3 = "key1_b", *k4 = "key1_c"; char *v1 = "val1", *v2 = "val2", *v3 = "val3", *value; - ht = hashtable_create((hashtable_hash_t)hash_match, - (hashtable_equals_t)equals, 0); + hl = (hashlist_t*)create_hashtable(HASHLIST_FUZZY); ht->put(ht, k1, v1); ht->put(ht, k2, v2); @@ -118,31 +168,31 @@ START_TEST(test_get_match) ck_assert(streq(ht->get(ht, k3), v3)); ck_assert(value == NULL); - value = ht->get_match(ht, k1, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k1, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - value = ht->get_match(ht, k2, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k2, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v2)); - value = ht->get_match(ht, k3, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k3, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - value = ht->get_match(ht, k4, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k4, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - - ht->destroy(ht); } END_TEST START_TEST(test_get_match_remove) { + hashlist_t *hl; char *k1 = "key1_a", *k2 = "key2", *k3 = "key1_b", *k4 = "key1_c"; char *v1 = "val1", *v2 = "val2", *v3 = "val3", *value; - ht = hashtable_create((hashtable_hash_t)hash_match, - (hashtable_equals_t)equals, 0); + hl = (hashlist_t*)create_hashtable(HASHLIST_FUZZY); + /* by removing and reinserting the first item we verify that insertion + * order is adhered */ ht->put(ht, k1, v1); ht->put(ht, k2, v2); ht->put(ht, k3, v3); @@ -153,31 +203,30 @@ START_TEST(test_get_match_remove) ck_assert(streq(ht->get(ht, k2), v2)); ck_assert(streq(ht->get(ht, k3), v3)); - value = ht->get_match(ht, k1, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k1, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - value = ht->get_match(ht, k2, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k2, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v2)); - value = ht->get_match(ht, k3, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k3, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v3)); - value = ht->get_match(ht, k4, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k4, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v3)); - - ht->destroy(ht); } END_TEST START_TEST(test_get_match_sorted) { + hashlist_t *hl; char *k1 = "key1_a", *k2 = "key2", *k3 = "key1_b", *k4 = "key1_c"; char *v1 = "val1", *v2 = "val2", *v3 = "val3", *value; - ht = hashtable_create_sorted((hashtable_hash_t)hash_match, - (hashtable_cmp_t)strcmp, 0); + hl = (hashlist_t*)create_hashtable(HASHLIST_FUZZY_SORTED); + /* since the keys are sorted, the insertion order doesn't matter */ ht->put(ht, k3, v3); ht->put(ht, k2, v2); ht->put(ht, k1, v1); @@ -190,20 +239,18 @@ START_TEST(test_get_match_sorted) ck_assert(streq(ht->get(ht, k3), v3)); ck_assert(streq(ht->get(ht, k4), v1)); - value = ht->get_match(ht, k1, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k1, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - value = ht->get_match(ht, k2, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k2, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v2)); - value = ht->get_match(ht, k3, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k3, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - value = ht->get_match(ht, k4, (hashtable_equals_t)equal_match); + value = hl->get_match(hl, k4, (hashtable_equals_t)equal_match); ck_assert(value != NULL); ck_assert(streq(value, v1)); - - ht->destroy(ht); } END_TEST @@ -252,38 +299,6 @@ START_TEST(test_remove_one_bucket) { char *k1 = "key1_a", *k2 = "key1_b", *k3 = "key1_c"; - ht->destroy(ht); - ht = hashtable_create((hashtable_hash_t)hash_match, - (hashtable_equals_t)equals, 0); - - do_remove(k1, k2, k3); - do_remove(k3, k2, k1); - do_remove(k1, k3, k2); -} -END_TEST - -START_TEST(test_remove_sorted) -{ - char *k1 = "key1", *k2 = "key2", *k3 = "key3"; - - ht->destroy(ht); - ht = hashtable_create_sorted((hashtable_hash_t)hash, - (hashtable_cmp_t)strcmp, 0); - - do_remove(k1, k2, k3); - do_remove(k3, k2, k1); - do_remove(k1, k3, k2); -} -END_TEST - -START_TEST(test_remove_sorted_one_bucket) -{ - char *k1 = "key1_a", *k2 = "key1_b", *k3 = "key1_c"; - - ht->destroy(ht); - ht = hashtable_create_sorted((hashtable_hash_t)hash_match, - (hashtable_cmp_t)strcmp, 0); - do_remove(k1, k2, k3); do_remove(k3, k2, k1); do_remove(k1, k3, k2); @@ -404,15 +419,10 @@ START_TEST(test_remove_at_one_bucket) { char *k1 = "key1_a", *k2 = "key1_b", *k3 = "key1_c"; - ht->destroy(ht); - ht = hashtable_create((hashtable_hash_t)hash_match, - (hashtable_equals_t)equals, 0); - do_remove_at(k1, k2, k3); } END_TEST - /******************************************************************************* * many items */ @@ -432,34 +442,63 @@ static int cmp_int(int *key1, int *key2) return *key1 - *key2; } +/** + * Create a specific hash table with integers as keys. + */ +static hashtable_t *create_int_hashtable(int i) +{ + hashlist_t *hl = NULL; + + DESTROY_IF(ht); + + switch (i) + { + case HASHTABLE_REGULAR: + ht = hashtable_create((hashtable_hash_t)hash_int, + (hashtable_equals_t)equals_int, 0); + break; + case HASHLIST_REGULAR: + hl = hashlist_create((hashtable_hash_t)hash_int, + (hashtable_equals_t)equals_int, 0); + break; + case HASHLIST_REGULAR_SORTED: + hl = hashlist_create_sorted((hashtable_hash_t)hash_int, + (hashtable_cmp_t)cmp_int, 0); + break; + } + if (hl) + { + ht = &hl->ht; + } + ck_assert_int_eq(ht->get_count(ht), 0); + return ht; +} + START_SETUP(setup_ht_many) { - ht = hashtable_create((hashtable_hash_t)hash_int, - (hashtable_equals_t)equals_int, 0); - ck_assert_int_eq(ht->get_count(ht), 0); + create_int_hashtable(_i >> 1); } END_SETUP -START_SETUP(setup_ht_many_cmp) +START_SETUP(setup_ht_lookups) { - ht = hashtable_create_sorted((hashtable_hash_t)hash_int, - (hashtable_cmp_t)cmp_int, 0); - ck_assert_int_eq(ht->get_count(ht), 0); + create_int_hashtable(_i); } END_SETUP START_TEARDOWN(teardown_ht_many) { ht->destroy_function(ht, (void*)free); + ht = NULL; } END_TEARDOWN START_TEST(test_many_items) { - u_int count = 250000; + u_int count = 100000; int i, *val, r; -#define GET_VALUE(i) ({ _i == 0 ? i : (count-1-i); }) +#define GET_VALUE(i) ({ (_i % 2) == 0 ? i : (count-1-i); }) for (i = 0; i < count; i++) { @@ -587,10 +626,11 @@ Suite *hashtable_suite_create() tc = tcase_create("put/get"); tcase_add_checked_fixture(tc, setup_ht, teardown_ht); - tcase_add_test(tc, test_put_get); + tcase_add_loop_test(tc, test_put_get, 0, HASHTABLE_MAX); suite_add_tcase(s, tc); tc = tcase_create("get_match"); + tcase_add_checked_fixture(tc, NULL, teardown_ht); tcase_add_test(tc, test_get_match); tcase_add_test(tc, test_get_match_remove); tcase_add_test(tc, test_get_match_sorted); @@ -598,39 +638,32 @@ Suite *hashtable_suite_create() tc = tcase_create("remove"); tcase_add_checked_fixture(tc, setup_ht, teardown_ht); - tcase_add_test(tc, test_remove); - tcase_add_test(tc, test_remove_one_bucket); - tcase_add_test(tc, test_remove_sorted); - tcase_add_test(tc, test_remove_sorted_one_bucket); + tcase_add_loop_test(tc, test_remove, 0, REGULAR_MAX); + tcase_add_loop_test(tc, test_remove_one_bucket, HASHTABLE_FUZZY, HASHTABLE_MAX); suite_add_tcase(s, tc); tc = tcase_create("enumerator"); tcase_add_checked_fixture(tc, setup_ht, teardown_ht); - tcase_add_test(tc, test_enumerator); + tcase_add_loop_test(tc, test_enumerator, 0, HASHTABLE_MAX); suite_add_tcase(s, tc); tc = tcase_create("remove_at"); tcase_add_checked_fixture(tc, setup_ht, teardown_ht); - tcase_add_test(tc, test_remove_at); - tcase_add_test(tc, test_remove_at_one_bucket); + tcase_add_loop_test(tc, test_remove_at, 0, REGULAR_MAX); + tcase_add_loop_test(tc, test_remove_at_one_bucket, HASHTABLE_FUZZY, HASHTABLE_MAX); suite_add_tcase(s, tc); tc = tcase_create("many items"); tcase_add_checked_fixture(tc, setup_ht_many, teardown_ht_many); tcase_set_timeout(tc, 10); - tcase_add_loop_test(tc, test_many_items, 0, 2); - tcase_add_test(tc, test_many_lookups_success); - tcase_add_test(tc, test_many_lookups_failure_larger); - tcase_add_test(tc, test_many_lookups_failure_smaller); + tcase_add_loop_test(tc, test_many_items, 0, REGULAR_MAX << 1); suite_add_tcase(s, tc); - tc = tcase_create("many items sorted"); - tcase_add_checked_fixture(tc, setup_ht_many_cmp, teardown_ht_many); - tcase_set_timeout(tc, 10); - tcase_add_loop_test(tc, test_many_items, 0, 2); - tcase_add_test(tc, test_many_lookups_success); - tcase_add_test(tc, test_many_lookups_failure_larger); - tcase_add_test(tc, test_many_lookups_failure_smaller); + tc = tcase_create("many lookups"); + tcase_add_checked_fixture(tc, setup_ht_lookups, teardown_ht_many); + tcase_add_loop_test(tc, test_many_lookups_success, 0, REGULAR_MAX); + tcase_add_loop_test(tc, test_many_lookups_failure_larger, 0, REGULAR_MAX); + tcase_add_loop_test(tc, test_many_lookups_failure_smaller, 0, REGULAR_MAX); suite_add_tcase(s, tc); return s; From 45376040ce6346693a5acafd6449085f3896350c Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 15:51:17 +0200 Subject: [PATCH 08/10] hashtable: Maintain insertion order when enumerating With the previous approach we'd require at least an additional pointer per item to store them in a list (15-18% increase in the overhead per item). Instead we switch from handling collisions with overflow lists to an open addressing scheme and store the actual table as variable-sized indices pointing into an array of all inserted items in their original order. This can reduce the memory overhead even compared to the previous implementation (especially for smaller tables), but because the array for items is preallocated whenever the table is resized, it can be worse for certain numbers of items. However, avoiding all the allocations required by the previous design is actually a big advantage. Depending on the usage pattern, the performance can improve quite a bit (in particular when inserting many items). The raw lookup performance is a bit slower as probing lengths increase with open addressing, but there are some caching benefits due to the compact storage. So for general usage the performance should be better. For instance, one test I did was counting the occurrences of words in a list of 1'000'000 randomly selected words from a dictionary of ~58'000 words (i.e. using a counter stored under each word as key). The new implementation was ~8% faster on average while requiring 10% less memory. Since we can't remove items from the array (would change the indices of all items that follow it) we just mark them as removed and remove them once the hash table is resized/rehashed (the cells in the hash table for these may be reused). Due to this the latter may also happen if the number of stored items does not increase e.g. after a series of remove/put operations (each insertion requires storage in the array, no matter if items were removed). So if the capacity is exhausted, the table is resized/rehashed (after lots of removals the size may even be reduced) and all items marked as removed are simply skipped. Compared to the previous implementation the load factor/capacity is lowered to reduce chances of collisions and to avoid primary clustering to some degree. However, the latter in particular, but the open addressing scheme in general, make this implementation completely unsuited for the get_match() functionality (purposefully hashing to the same value and, therefore, increasing the probing length and clustering). And keeping the keys optionally sorted would complicate the code significantly. So we just keep the existing hashlist_t implementation without adding code to maintain the overall insertion order (we could add that feature optionally later, but with the mentioned overhead for one or two pointers). The maximum size is currently not changed. With the new implementation this translates to a hard limit for the maximum number of items that can be held in the table (=CAPACITY(MAX_SIZE)). Since this equals 715'827'882 items with the current settings, this shouldn't be a problem in practice, the table alone would require 20 GiB in memory for that many items. The hashlist_t implementation doesn't have that limitation due to the overflow lists (it can store beyond it's capacity) but it itself would require over 29 GiB of memory to hold that many items. --- src/libstrongswan/Android.mk | 2 +- src/libstrongswan/Makefile.am | 3 +- src/libstrongswan/collections/hashlist.c | 548 ++++++++++++++ src/libstrongswan/collections/hashtable.c | 699 +++++++----------- src/libstrongswan/collections/hashtable.h | 12 +- .../collections/hashtable_profiler.h | 119 +++ .../tests/suites/test_hashtable.c | 60 ++ 7 files changed, 990 insertions(+), 453 deletions(-) create mode 100644 src/libstrongswan/collections/hashlist.c create mode 100644 src/libstrongswan/collections/hashtable_profiler.h diff --git a/src/libstrongswan/Android.mk b/src/libstrongswan/Android.mk index 97a9f6870..6d87447ea 100644 --- a/src/libstrongswan/Android.mk +++ b/src/libstrongswan/Android.mk @@ -6,7 +6,7 @@ libstrongswan_la_SOURCES = \ library.c \ asn1/asn1.c asn1/asn1_parser.c asn1/oid.c bio/bio_reader.c bio/bio_writer.c \ collections/blocking_queue.c collections/enumerator.c collections/hashtable.c \ -collections/array.c \ +collections/hashlist.c collections/array.c \ collections/linked_list.c crypto/crypters/crypter.c \ crypto/drbgs/drbg.c crypto/hashers/hasher.c \ crypto/hashers/hash_algorithm_set.c crypto/proposal/proposal.c \ diff --git a/src/libstrongswan/Makefile.am b/src/libstrongswan/Makefile.am index fda7463ed..0754d7bc5 100644 --- a/src/libstrongswan/Makefile.am +++ b/src/libstrongswan/Makefile.am @@ -4,7 +4,7 @@ libstrongswan_la_SOURCES = \ library.c \ asn1/asn1.c asn1/asn1_parser.c asn1/oid.c bio/bio_reader.c bio/bio_writer.c \ collections/blocking_queue.c collections/enumerator.c collections/hashtable.c \ -collections/array.c \ +collections/hashlist.c collections/array.c \ collections/linked_list.c crypto/crypters/crypter.c \ crypto/drbgs/drbg.c crypto/hashers/hasher.c \ crypto/hashers/hash_algorithm_set.c crypto/proposal/proposal.c \ @@ -69,6 +69,7 @@ nobase_strongswan_include_HEADERS = \ library.h \ asn1/asn1.h asn1/asn1_parser.h asn1/oid.h bio/bio_reader.h bio/bio_writer.h \ collections/blocking_queue.h collections/enumerator.h collections/hashtable.h \ +collections/hashtable_profiler.h \ collections/linked_list.h collections/array.h collections/dictionary.h \ crypto/crypters/crypter.h crypto/drbgs/drbg.h crypto/hashers/hasher.h \ crypto/hashers/hash_algorithm_set.h crypto/mac.h crypto/proposal/proposal.h \ diff --git a/src/libstrongswan/collections/hashlist.c b/src/libstrongswan/collections/hashlist.c new file mode 100644 index 000000000..13ef2a254 --- /dev/null +++ b/src/libstrongswan/collections/hashlist.c @@ -0,0 +1,548 @@ +/* + * Copyright (C) 2008-2020 Tobias Brunner + * HSR Hochschule fuer Technik Rapperswil + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. See . + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#include "hashtable.h" +#include "hashtable_profiler.h" + +#include +#include +#ifdef HASHTABLE_PROFILER +#include +#endif + +/** The minimum size of the hash table (MUST be a power of 2) */ +#define MIN_SIZE 8 +/** The maximum size of the hash table (MUST be a power of 2) */ +#define MAX_SIZE (1 << 30) + +/** Maximum load factor before the hash table is resized */ +#define LOAD_FACTOR 0.75f + +/** Provided by hashtable_t */ +u_int hashtable_get_nearest_powerof2(u_int n); + +typedef struct pair_t pair_t; + +/** + * This pair holds a pointer to the key and value it represents. + */ +struct pair_t { + + /** + * Key of a hash table item. + */ + const void *key; + + /** + * Value of a hash table item. + */ + void *value; + + /** + * Cached hash (used in case of a resize). + */ + u_int hash; + + /** + * Next pair in an overflow list. + */ + pair_t *next; +}; + +typedef struct private_hashlist_t private_hashlist_t; + +/** + * Private data of a hashlist_t object. + */ +struct private_hashlist_t { + + /** + * Public interface. + */ + hashlist_t public; + + /** + * The number of items in the hash table. + */ + u_int count; + + /** + * The current size of the hash table (always a power of 2). + */ + u_int size; + + /** + * The current mask to calculate the row index (size - 1). + */ + u_int mask; + + /** + * The actual table. + */ + pair_t **table; + + /** + * The hashing function. + */ + hashtable_hash_t hash; + + /** + * The equality function. + */ + hashtable_equals_t equals; + + /** + * Alternative comparison function. + */ + hashtable_cmp_t cmp; + + /** + * Profiling information + */ + hashtable_profile_t profile; +}; + +typedef struct private_enumerator_t private_enumerator_t; + +/** + * Hash table enumerator implementation + */ +struct private_enumerator_t { + + /** + * Implements enumerator interface. + */ + enumerator_t enumerator; + + /** + * Associated hash table. + */ + private_hashlist_t *table; + + /** + * Current row index. + */ + u_int row; + + /** + * Number of remaining items to enumerate. + */ + u_int count; + + /** + * Current pair. + */ + pair_t *current; + + /** + * Previous pair (used by remove_at). + */ + pair_t *prev; +}; + +/** + * Init hash table parameters + */ +static void init_hashtable(private_hashlist_t *this, u_int size) +{ + size = max(MIN_SIZE, min(size, MAX_SIZE)); + this->size = hashtable_get_nearest_powerof2(size); + this->mask = this->size - 1; + profile_size(&this->profile, this->size); + + this->table = calloc(this->size, sizeof(pair_t*)); +} + +/** + * Insert an item into a bucket. + */ +static inline void insert_pair(private_hashlist_t *this, pair_t *to_insert, + pair_t *prev) +{ + u_int row; + + if (prev) + { + to_insert->next = prev->next; + prev->next = to_insert; + } + else + { + row = to_insert->hash & this->mask; + to_insert->next = this->table[row]; + this->table[row] = to_insert; + } +} + +/** + * Double the size of the hash table and rehash all the elements. + */ +static void rehash(private_hashlist_t *this) +{ + pair_t **old_table, *to_move, *pair, *next; + u_int row, old_size; + + if (this->size >= MAX_SIZE) + { + return; + } + + old_size = this->size; + old_table = this->table; + + init_hashtable(this, old_size << 1); + + for (row = 0; row < old_size; row++) + { + to_move = old_table[row]; + while (to_move) + { + pair_t *prev = NULL; + + pair = this->table[to_move->hash & this->mask]; + while (pair) + { + if (this->cmp && this->cmp(to_move->key, pair->key) < 0) + { + break; + } + prev = pair; + pair = pair->next; + } + next = to_move->next; + insert_pair(this, to_move, prev); + to_move = next; + } + } + free(old_table); +} + +/** + * Find the pair with the given key, optionally returning the hash and previous + * (or last) pair in the bucket. + */ +static inline pair_t *find_key(private_hashlist_t *this, const void *key, + hashtable_equals_t equals, u_int *out_hash, + pair_t **out_prev) +{ + pair_t *pair, *prev = NULL; + bool use_callback = equals != NULL; + u_int hash; + + if (!this->count && !out_hash) + { /* no need to calculate the hash if not requested */ + return NULL; + } + + equals = equals ?: this->equals; + hash = this->hash(key); + if (out_hash) + { + *out_hash = hash; + } + + lookup_start(); + + pair = this->table[hash & this->mask]; + while (pair) + { + lookup_probing(); + /* when keys are sorted, we compare all items so we can abort earlier + * even if the hash does not match, but only as long as we don't + * have a callback */ + if (!use_callback && this->cmp) + { + int cmp = this->cmp(key, pair->key); + if (cmp == 0) + { + break; + } + else if (cmp < 0) + { /* no need to continue as the key we search is smaller */ + pair = NULL; + break; + } + } + else if (hash == pair->hash && equals(key, pair->key)) + { + break; + } + prev = pair; + pair = pair->next; + } + if (out_prev) + { + *out_prev = prev; + } + if (pair) + { + lookup_success(&this->profile); + } + else + { + lookup_failure(&this->profile); + } + return pair; +} + +METHOD(hashtable_t, put, void*, + private_hashlist_t *this, const void *key, void *value) +{ + void *old_value = NULL; + pair_t *pair, *prev = NULL; + u_int hash; + + if (this->count >= this->size * LOAD_FACTOR) + { + rehash(this); + } + + pair = find_key(this, key, NULL, &hash, &prev); + if (pair) + { + old_value = pair->value; + pair->value = value; + pair->key = key; + } + else + { + INIT(pair, + .key = key, + .value = value, + .hash = hash, + ); + insert_pair(this, pair, prev); + this->count++; + profile_count(&this->profile, this->count); + } + return old_value; +} + + +METHOD(hashtable_t, get, void*, + private_hashlist_t *this, const void *key) +{ + pair_t *pair = find_key(this, key, NULL, NULL, NULL); + return pair ? pair->value : NULL; +} + +METHOD(hashlist_t, get_match, void*, + private_hashlist_t *this, const void *key, hashtable_equals_t match) +{ + pair_t *pair = find_key(this, key, match, NULL, NULL); + return pair ? pair->value : NULL; +} + +METHOD(hashtable_t, remove_, void*, + private_hashlist_t *this, const void *key) +{ + void *value = NULL; + pair_t *pair, *prev = NULL; + + pair = find_key(this, key, NULL, NULL, &prev); + if (pair) + { + if (prev) + { + prev->next = pair->next; + } + else + { + this->table[pair->hash & this->mask] = pair->next; + } + value = pair->value; + free(pair); + this->count--; + } + return value; +} + +METHOD(hashtable_t, remove_at, void, + private_hashlist_t *this, private_enumerator_t *enumerator) +{ + if (enumerator->table == this && enumerator->current) + { + pair_t *current = enumerator->current; + if (enumerator->prev) + { + enumerator->prev->next = current->next; + } + else + { + this->table[enumerator->row] = current->next; + } + enumerator->current = enumerator->prev; + free(current); + this->count--; + } +} + +METHOD(hashtable_t, get_count, u_int, + private_hashlist_t *this) +{ + return this->count; +} + +METHOD(enumerator_t, enumerate, bool, + private_enumerator_t *this, va_list args) +{ + const void **key; + void **value; + + VA_ARGS_VGET(args, key, value); + + while (this->count && this->row < this->table->size) + { + this->prev = this->current; + if (this->current) + { + this->current = this->current->next; + } + else + { + this->current = this->table->table[this->row]; + } + if (this->current) + { + if (key) + { + *key = this->current->key; + } + if (value) + { + *value = this->current->value; + } + this->count--; + return TRUE; + } + this->row++; + } + return FALSE; +} + +METHOD(hashtable_t, create_enumerator, enumerator_t*, + private_hashlist_t *this) +{ + private_enumerator_t *enumerator; + + INIT(enumerator, + .enumerator = { + .enumerate = enumerator_enumerate_default, + .venumerate = _enumerate, + .destroy = (void*)free, + }, + .table = this, + .count = this->count, + ); + + return &enumerator->enumerator; +} + +static void destroy_internal(private_hashlist_t *this, + void (*fn)(void*,const void*)) +{ + pair_t *pair, *next; + u_int row; + + profiler_cleanup(&this->profile, this->count, this->size); + + for (row = 0; row < this->size; row++) + { + pair = this->table[row]; + while (pair) + { + if (fn) + { + fn(pair->value, pair->key); + } + next = pair->next; + free(pair); + pair = next; + } + } + free(this->table); + free(this); +} + +METHOD2(hashlist_t, hashtable_t, destroy, void, + private_hashlist_t *this) +{ + destroy_internal(this, NULL); +} + +METHOD(hashtable_t, destroy_function, void, + private_hashlist_t *this, void (*fn)(void*,const void*)) +{ + destroy_internal(this, fn); +} + +/** + * Create a hash list + */ +static private_hashlist_t *hashlist_create_internal(hashtable_hash_t hash, + u_int size) +{ + private_hashlist_t *this; + + INIT(this, + .public = { + .ht = { + .put = _put, + .get = _get, + .remove = _remove_, + .remove_at = (void*)_remove_at, + .get_count = _get_count, + .create_enumerator = _create_enumerator, + .destroy = _destroy, + .destroy_function = _destroy_function, + }, + .get_match = _get_match, + .destroy = _destroy, + }, + .hash = hash, + ); + + init_hashtable(this, size); + + profiler_init(&this->profile, 3); + + return this; +} + +/* + * Described in header + */ +hashlist_t *hashlist_create(hashtable_hash_t hash, hashtable_equals_t equals, + u_int size) +{ + private_hashlist_t *this = hashlist_create_internal(hash, size); + + this->equals = equals; + + return &this->public; +} + +/* + * Described in header + */ +hashlist_t *hashlist_create_sorted(hashtable_hash_t hash, + hashtable_cmp_t cmp, u_int size) +{ + private_hashlist_t *this = hashlist_create_internal(hash, size); + + this->cmp = cmp; + + return &this->public; +} diff --git a/src/libstrongswan/collections/hashtable.c b/src/libstrongswan/collections/hashtable.c index e569e581e..8c290ac2d 100644 --- a/src/libstrongswan/collections/hashtable.c +++ b/src/libstrongswan/collections/hashtable.c @@ -14,19 +14,34 @@ */ #include "hashtable.h" +#include "hashtable_profiler.h" #include #include -#ifdef HASHTABLE_PROFILER -#include -#endif /** The minimum size of the hash table (MUST be a power of 2) */ #define MIN_SIZE 8 /** The maximum size of the hash table (MUST be a power of 2) */ #define MAX_SIZE (1 << 30) -/** Maximum load factor before the hash table is resized */ -#define LOAD_FACTOR 0.75f + +/** Determine the capacity/maximum load of the table (higher values cause + * more collisions, lower values increase the memory overhead) */ +#define CAPACITY(size) (size / 3 * 2) +/** Factor for the new table size based on the number of items when resizing, + * with the above load factor this results in doubling the size when growing */ +#define RESIZE_FACTOR 3 + +/** + * A note about these parameters: + * + * The maximum number of items that can be stored in this implementation + * is MAX_COUNT = CAPACITY(MAX_SIZE). + * Since we use u_int throughout, MAX_COUNT * RESIZE_FACTOR must not overflow + * this type. + */ +#if (UINT_MAX / RESIZE_FACTOR < CAPACITY(MAX_SIZE)) + #error Hahstable parameters invalid! +#endif typedef struct pair_t pair_t; @@ -49,33 +64,13 @@ struct pair_t { * Cached hash (used in case of a resize). */ u_int hash; - - /** - * Next pair in an overflow list. - */ - pair_t *next; }; -/** - * Creates an empty pair object. - */ -static inline pair_t *pair_create(const void *key, void *value, u_int hash) -{ - pair_t *this; - - INIT(this, - .key = key, - .value = value, - .hash = hash, - ); - - return this; -} - typedef struct private_hashtable_t private_hashtable_t; /** * Private data of a hashtable_t object. + * */ struct private_hashtable_t { @@ -100,9 +95,27 @@ struct private_hashtable_t { u_int mask; /** - * The actual table. + * All items in the order they were inserted (removed items are marked by + * setting the key to NULL until resized). */ - pair_t **table; + pair_t *items; + + /** + * Number of available slots in the array above and the table in general, + * is set to CAPACITY(size) when the hash table is initialized. + */ + u_int capacity; + + /** + * Number of used slots in the array above. + */ + u_int items_count; + + /** + * Hash table with indices into the array above. The type depends on the + * current capacity. + */ + void *table; /** * The hashing function. @@ -115,126 +128,36 @@ struct private_hashtable_t { hashtable_equals_t equals; /** - * Alternative comparison function. + * Profiling data */ - hashtable_cmp_t cmp; - -#ifdef HASHTABLE_PROFILER - /** - * Some stats to profile lookups in the table - */ - struct { - size_t count; - size_t probes; - size_t longest; - } success, failure; - - /** - * Stats on the memory usage of the table - */ - struct { - size_t count; - size_t size; - } max; - - /** - * Keep track of where the hash table was created - */ - backtrace_t *backtrace; -#endif + hashtable_profile_t profile; }; -typedef struct private_hashlist_t private_hashlist_t; - -/** - * Private data of a hashlist_t object. - */ -struct private_hashlist_t { - - /** - * Public part of hash table. - */ - hashlist_t public; - - /** - * Inherited private part of hash table (we get the public part too, but - * ignore it). - */ - private_hashtable_t super; -}; - -#ifdef HASHTABLE_PROFILER - -#define lookup_start() \ - u_int _lookup_probes = 0; - -#define lookup_probing() \ - _lookup_probes++; - -#define _lookup_done(table, result) \ - table->result.count++; \ - table->result.probes += _lookup_probes; \ - table->result.longest = max(table->result.longest, _lookup_probes); - -#define lookup_success(table) _lookup_done(table, success); -#define lookup_failure(table) _lookup_done(table, failure); - -#define profile_size(table) \ - table->max.size = max(table->max.size, table->size); -#define profile_count(table) \ - table->max.count = max(table->max.count, table->count); - -#else - -#define lookup_start(...) {} -#define lookup_probing(...) {} -#define lookup_success(...) {} -#define lookup_failure(...) {} -#define profile_size(...) {} -#define profile_count(...) {} - -#endif - typedef struct private_enumerator_t private_enumerator_t; /** - * hash table enumerator implementation + * Hash table enumerator implementation */ struct private_enumerator_t { /** - * implements enumerator interface + * Implements enumerator interface */ enumerator_t enumerator; /** - * associated hash table + * Associated hash table */ private_hashtable_t *table; /** - * current row index + * Current index */ - u_int row; - - /** - * number of remaining items in hashtable - */ - u_int count; - - /** - * current pair - */ - pair_t *current; - - /** - * previous pair (used by remove_at) - */ - pair_t *prev; + u_int index; }; /* - * See header. + * Described in header */ u_int hashtable_hash_ptr(const void *key) { @@ -242,7 +165,7 @@ u_int hashtable_hash_ptr(const void *key) } /* - * See header. + * Described in header */ u_int hashtable_hash_str(const void *key) { @@ -250,7 +173,7 @@ u_int hashtable_hash_str(const void *key) } /* - * See header. + * Described in header */ bool hashtable_equals_ptr(const void *key, const void *other_key) { @@ -258,20 +181,58 @@ bool hashtable_equals_ptr(const void *key, const void *other_key) } /* - * See header. + * Described in header */ bool hashtable_equals_str(const void *key, const void *other_key) { return streq(key, other_key); } +/** + * Returns the index stored in the given bucket. If the bucket is empty, + * 0 is returned. + */ +static inline u_int get_index(private_hashtable_t *this, u_int row) +{ + if (this->capacity <= 0xff) + { + return ((uint8_t*)this->table)[row]; + } + else if (this->capacity <= 0xffff) + { + return ((uint16_t*)this->table)[row]; + } + return ((u_int*)this->table)[row]; +} + +/** + * Set the index stored in the given bucket. Set to 0 to clear a bucket. + */ +static inline void set_index(private_hashtable_t *this, u_int row, u_int index) +{ + if (this->capacity <= 0xff) + { + ((uint8_t*)this->table)[row] = index; + } + else if (this->capacity <= 0xffff) + { + ((uint16_t*)this->table)[row] = index; + } + else + { + ((u_int*)this->table)[row] = index; + } +} + /** * This function returns the next-highest power of two for the given number. * The algorithm works by setting all bits on the right-hand side of the most * significant 1 to 1 and then increments the whole number so it rolls over * to the nearest power of two. Note: returns 0 for n == 0 + * + * Also used by hashlist_t. */ -static u_int get_nearest_powerof2(u_int n) +u_int hashtable_get_nearest_powerof2(u_int n) { u_int i; @@ -284,227 +245,224 @@ static u_int get_nearest_powerof2(u_int n) } /** - * Init hash table parameters + * Init hash table to the given size */ static void init_hashtable(private_hashtable_t *this, u_int size) { - size = max(MIN_SIZE, min(size, MAX_SIZE)); - this->size = get_nearest_powerof2(size); + u_int index_size = sizeof(u_int); + + this->size = max(MIN_SIZE, min(size, MAX_SIZE)); + this->size = hashtable_get_nearest_powerof2(this->size); this->mask = this->size - 1; - profile_size(this); + profile_size(&this->profile, this->size); - this->table = calloc(this->size, sizeof(pair_t*)); + this->capacity = CAPACITY(this->size); + this->items = calloc(this->capacity, sizeof(pair_t)); + this->items_count = 0; + + if (this->capacity <= 0xff) + { + index_size = sizeof(uint8_t); + } + else if (this->capacity <= 0xffff) + { + index_size = sizeof(uint16_t); + } + this->table = calloc(this->size, index_size); } /** - * Double the size of the hash table and rehash all the elements. + * Calculate the next bucket using simple linear probing for now. */ -static void rehash(private_hashtable_t *this) +static inline u_int get_next(private_hashtable_t *this, u_int row) { - pair_t **old_table, *to_move, *pair, *next; - u_int row, new_row, old_size; - - if (this->size >= MAX_SIZE) - { - return; - } - - old_size = this->size; - old_table = this->table; - - init_hashtable(this, old_size << 1); - - for (row = 0; row < old_size; row++) - { - to_move = old_table[row]; - while (to_move) - { - pair_t *prev = NULL; - - new_row = to_move->hash & this->mask; - pair = this->table[new_row]; - while (pair) - { - if (this->cmp && this->cmp(to_move->key, pair->key) < 0) - { - break; - } - prev = pair; - pair = pair->next; - } - next = to_move->next; - to_move->next = NULL; - if (prev) - { - to_move->next = prev->next; - prev->next = to_move; - } - else - { - to_move->next = this->table[new_row]; - this->table[new_row] = to_move; - } - to_move = next; - } - } - free(old_table); + return (row + 1) & this->mask; } /** - * Find the pair with the given key, optionally returning the hash and previous - * (or last) pair in the bucket. + * Find the pair with the given key, optionally returns the hash and first empty + * or previously used row if the key is not found. */ static inline pair_t *find_key(private_hashtable_t *this, const void *key, - hashtable_equals_t equals, u_int *out_hash, - pair_t **out_prev) + u_int *out_hash, u_int *out_row) { - pair_t *pair, *prev = NULL; - bool use_callback = equals != NULL; - u_int hash; + pair_t *pair; + u_int hash, row, removed, index; + bool found_removed = FALSE; - if (!this->count && !out_hash) - { /* no need to calculate the hash if not requested */ - return NULL; - } - - equals = equals ?: this->equals; - hash = this->hash(key); - if (out_hash) + if (!this->count && !out_hash && !out_row) { - *out_hash = hash; + return NULL; } lookup_start(); - pair = this->table[hash & this->mask]; - while (pair) + hash = this->hash(key); + row = hash & this->mask; + index = get_index(this, row); + while (index) { lookup_probing(); - /* when keys are sorted, we compare all items so we can abort earlier - * even if the hash does not match, but only as long as we don't - * have a callback */ - if (!use_callback && this->cmp) + pair = &this->items[index-1]; + + if (!pair->key) { - int cmp = this->cmp(key, pair->key); - if (cmp == 0) + if (!found_removed && out_row) { - break; - } - else if (cmp < 0) - { /* no need to continue as the key we search is smaller */ - pair = NULL; - break; + removed = row; + found_removed = TRUE; } } - else if (hash == pair->hash && equals(key, pair->key)) + else if (pair->hash == hash && this->equals(key, pair->key)) { - break; + lookup_success(&this->profile); + return pair; } - prev = pair; - pair = pair->next; + row = get_next(this, row); + index = get_index(this, row); } - if (out_prev) + if (out_hash) { - *out_prev = prev; + *out_hash = hash; } - if (pair) + if (out_row) { - lookup_success(this); + *out_row = found_removed ? removed : row; } - else + lookup_failure(&this->profile); + return NULL; +} + +/** + * Helper to insert a new item into the table and items array, + * returns its new index into the latter. + */ +static inline u_int insert_item(private_hashtable_t *this, u_int row) +{ + u_int index = this->items_count++; + + /* we use 0 to mark unused buckets, so increase the index */ + set_index(this, row, index + 1); + return index; +} + +/** + * Resize the hash table to the given size and rehash all the elements, + * size may be smaller or even the same (e.g. if it's necessary to clear + * previously used buckets). + */ +static bool rehash(private_hashtable_t *this, u_int size) +{ + pair_t *old_items, *pair; + u_int old_count, i, row, index; + + if (size > MAX_SIZE) { - lookup_failure(this); + return FALSE; } - return pair; + + old_items = this->items; + old_count = this->items_count; + free(this->table); + init_hashtable(this, size); + + /* no need to do anything if the table is empty and we are just cleaning + * up previously used items */ + if (this->count) + { + for (i = 0; i < old_count; i++) + { + pair = &old_items[i]; + + if (pair->key) + { + row = pair->hash & this->mask; + index = get_index(this, row); + while (index) + { + row = get_next(this, row); + index = get_index(this, row); + } + index = insert_item(this, row); + this->items[index] = *pair; + } + } + } + free(old_items); + return TRUE; } METHOD(hashtable_t, put, void*, private_hashtable_t *this, const void *key, void *value) { void *old_value = NULL; - pair_t *pair, *prev = NULL; - u_int hash; + pair_t *pair; + u_int index, hash = 0, row = 0; - if (this->count >= this->size * LOAD_FACTOR) + if (this->items_count >= this->capacity && + !rehash(this, this->count * RESIZE_FACTOR)) { - rehash(this); + DBG1(DBG_LIB, "!!! FAILED TO RESIZE HASHTABLE TO %u !!!", + this->count * RESIZE_FACTOR); + return NULL; } - - pair = find_key(this, key, NULL, &hash, &prev); + pair = find_key(this, key, &hash, &row); if (pair) { old_value = pair->value; pair->value = value; pair->key = key; + return old_value; } - else - { - pair = pair_create(key, value, hash); - if (prev) - { - pair->next = prev->next; - prev->next = pair; - } - else - { - pair->next = this->table[hash & this->mask]; - this->table[hash & this->mask] = pair; - } - this->count++; - profile_count(this); - } - return old_value; + index = insert_item(this, row); + this->items[index] = (pair_t){ + .hash = hash, + .key = key, + .value = value, + }; + this->count++; + profile_count(&this->profile, this->count); + return NULL; } - METHOD(hashtable_t, get, void*, private_hashtable_t *this, const void *key) { - pair_t *pair = find_key(this, key, NULL, NULL, NULL); + pair_t *pair = find_key(this, key, NULL, NULL); return pair ? pair->value : NULL; } -METHOD(hashtable_t, remove_, void*, - private_hashtable_t *this, const void *key) +/** + * Remove the given item from the table, returns the currently stored value. + */ +static void *remove_internal(private_hashtable_t *this, pair_t *pair) { void *value = NULL; - pair_t *pair, *prev = NULL; - pair = find_key(this, key, NULL, NULL, &prev); if (pair) - { - if (prev) - { - prev->next = pair->next; - } - else - { - this->table[pair->hash & this->mask] = pair->next; - } + { /* this does not decrease the item count as we keep the previously + * used items until the table is rehashed/resized */ value = pair->value; - free(pair); + pair->key = NULL; this->count--; } return value; } +METHOD(hashtable_t, remove_, void*, + private_hashtable_t *this, const void *key) +{ + pair_t *pair = find_key(this, key, NULL, NULL); + return remove_internal(this, pair); +} + METHOD(hashtable_t, remove_at, void, private_hashtable_t *this, private_enumerator_t *enumerator) { - if (enumerator->table == this && enumerator->current) - { - pair_t *current = enumerator->current; - if (enumerator->prev) - { - enumerator->prev->next = current->next; - } - else - { - this->table[enumerator->row] = current->next; - } - enumerator->current = enumerator->prev; - free(current); - this->count--; + if (enumerator->table == this && enumerator->index) + { /* the index is already advanced by one */ + u_int index = enumerator->index - 1; + remove_internal(this, &this->items[index]); } } @@ -519,34 +477,25 @@ METHOD(enumerator_t, enumerate, bool, { const void **key; void **value; + pair_t *pair; VA_ARGS_VGET(args, key, value); - while (this->count && this->row < this->table->size) + while (this->index < this->table->items_count) { - this->prev = this->current; - if (this->current) - { - this->current = this->current->next; - } - else - { - this->current = this->table->table[this->row]; - } - if (this->current) + pair = &this->table->items[this->index++]; + if (pair->key) { if (key) { - *key = this->current->key; + *key = pair->key; } if (value) { - *value = this->current->value; + *value = pair->value; } - this->count--; return TRUE; } - this->row++; } return FALSE; } @@ -563,125 +512,48 @@ METHOD(hashtable_t, create_enumerator, enumerator_t*, .destroy = (void*)free, }, .table = this, - .count = this->count, ); - return &enumerator->enumerator; } static void destroy_internal(private_hashtable_t *this, void (*fn)(void*,const void*)) { - pair_t *pair, *next; - u_int row; + pair_t *pair; + u_int i; -#ifdef HASHTABLE_PROFILER - if (this->success.count || this->failure.count) - { - fprintf(stderr, "%zu elements [max. %zu], %zu buckets [%zu], %zu " - "successful / %zu failed lookups, %.4f [%zu] / %.4f " - "[%zu] avg. probes in table created at:", - this->count, this->max.count, this->size, this->max.size, - this->success.count, this->failure.count, - (double)this->success.probes/this->success.count, - this->success.longest, - (double)this->failure.probes/this->failure.count, - this->failure.longest); - this->backtrace->log(this->backtrace, stderr, TRUE); - } - this->backtrace->destroy(this->backtrace); -#endif + profiler_cleanup(&this->profile, this->count, this->size); - for (row = 0; row < this->size; row++) + if (fn) { - pair = this->table[row]; - while (pair) + for (i = 0; i < this->items_count; i++) { - if (fn) + pair = &this->items[i]; + if (pair->key) { fn(pair->value, pair->key); } - next = pair->next; - free(pair); - pair = next; } } + free(this->items); free(this->table); + free(this); } METHOD(hashtable_t, destroy, void, private_hashtable_t *this) { destroy_internal(this, NULL); - free(this); } METHOD(hashtable_t, destroy_function, void, private_hashtable_t *this, void (*fn)(void*,const void*)) { destroy_internal(this, fn); - free(this); -} - -METHOD(hashtable_t, create_enumerator_hashlist, enumerator_t*, - private_hashlist_t *this) -{ - return create_enumerator(&this->super); -} - -METHOD(hashtable_t, put_hashlist, void*, - private_hashlist_t *this, const void *key, void *value) -{ - return put(&this->super, key, value); -} - -METHOD(hashtable_t, get_hashlist, void*, - private_hashlist_t *this, const void *key) -{ - return get(&this->super, key); -} - -METHOD(hashlist_t, get_match, void*, - private_hashlist_t *this, const void *key, hashtable_equals_t match) -{ - pair_t *pair = find_key(&this->super, key, match, NULL, NULL); - return pair ? pair->value : NULL; -} - -METHOD(hashtable_t, remove_hashlist, void*, - private_hashlist_t *this, const void *key) -{ - return remove_(&this->super, key); -} - -METHOD(hashtable_t, remove_at_hashlist, void, - private_hashlist_t *this, private_enumerator_t *enumerator) -{ - remove_at(&this->super, enumerator); -} - -METHOD(hashtable_t, get_count_hashlist, u_int, - private_hashlist_t *this) -{ - return get_count(&this->super); -} - -METHOD2(hashtable_t, hashlist_t, destroy_hashlist, void, - private_hashlist_t *this) -{ - destroy_internal(&this->super, NULL); - free(this); -} - -METHOD(hashtable_t, destroy_function_hashlist, void, - private_hashlist_t *this, void (*fn)(void*,const void*)) -{ - destroy_internal(&this->super, fn); - free(this); } /* - * Described in header + * Described in header. */ hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, u_int size) @@ -705,72 +577,7 @@ hashtable_t *hashtable_create(hashtable_hash_t hash, hashtable_equals_t equals, init_hashtable(this, size); -#ifdef HASHTABLE_PROFILER - this->backtrace = backtrace_create(3); -#endif - - return &this->public; -} - -/** - * Create a hash table - */ -static private_hashlist_t *hashlist_create_internal(hashtable_hash_t hash, - u_int size) -{ - private_hashlist_t *this; - - INIT(this, - .public = { - .ht = { - .put = _put_hashlist, - .get = _get_hashlist, - .remove = _remove_hashlist, - .remove_at = (void*)_remove_at_hashlist, - .get_count = _get_count_hashlist, - .create_enumerator = _create_enumerator_hashlist, - .destroy = _destroy_hashlist, - .destroy_function = _destroy_function_hashlist, - }, - .get_match = _get_match, - .destroy = _destroy_hashlist, - }, - .super = { - .hash = hash, - } - ); - - init_hashtable(&this->super, size); - -#ifdef HASHTABLE_PROFILER - this->super.backtrace = backtrace_create(3); -#endif - - return this; -} - -/* - * Described in header - */ -hashlist_t *hashlist_create(hashtable_hash_t hash, hashtable_equals_t equals, - u_int size) -{ - private_hashlist_t *this = hashlist_create_internal(hash, size); - - this->super.equals = equals; - - return &this->public; -} - -/* - * Described in header - */ -hashlist_t *hashlist_create_sorted(hashtable_hash_t hash, - hashtable_cmp_t cmp, u_int size) -{ - private_hashlist_t *this = hashlist_create_internal(hash, size); - - this->super.cmp = cmp; + profiler_init(&this->profile, 2); return &this->public; } diff --git a/src/libstrongswan/collections/hashtable.h b/src/libstrongswan/collections/hashtable.h index de0284920..2eef38260 100644 --- a/src/libstrongswan/collections/hashtable.h +++ b/src/libstrongswan/collections/hashtable.h @@ -53,7 +53,7 @@ u_int hashtable_hash_str(const void *key); /** * Prototype for a function that compares the two keys for equality. * - * @param key first key (the one we are looking for) + * @param key first key (the one we are looking for/inserting) * @param other_key second key * @return TRUE if the keys are equal */ @@ -92,8 +92,7 @@ typedef int (*hashtable_cmp_t)(const void *key, const void *other_key); * * General purpose hash table. This hash table is not synchronized. * - * @note Any ordering only pertains to keys/items in the same bucket (with or - * without the same hash value), not to the order when enumerating. + * The insertion order is maintained when enumerating entries. */ struct hashtable_t { @@ -169,7 +168,9 @@ struct hashtable_t { * method. * * @note The ordering only pertains to keys/items in the same bucket (with or - * without the same hash value), not to the order when enumerating. + * without the same hash value), not to the order when enumerating. So unlike + * hashtable_t this class does not guarantee any specific order when enumerating + * all entries. * * This is intended to be used with hash functions that intentionally return the * same hash value for different keys so multiple items can be retrieved for a @@ -194,7 +195,8 @@ struct hashlist_t { * than the equals/comparison function provided to the constructor. * * This basically allows to enumerate all entries with the same hash value - * in their key's order. + * in their key's order (insertion order, i.e. without comparison function) + * is only guaranteed for items with the same hash value. * * @param key the key to match against * @param match match function to be used when comparing keys diff --git a/src/libstrongswan/collections/hashtable_profiler.h b/src/libstrongswan/collections/hashtable_profiler.h new file mode 100644 index 000000000..e65b89371 --- /dev/null +++ b/src/libstrongswan/collections/hashtable_profiler.h @@ -0,0 +1,119 @@ +/* + * Copyright (C) 2020 Tobias Brunner + * HSR Hochschule fuer Technik Rapperswil + * + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the + * Free Software Foundation; either version 2 of the License, or (at your + * option) any later version. See . + * + * This program is distributed in the hope that it will be useful, but + * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY + * or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License + * for more details. + */ + +#ifndef HASHTABLE_PROFILER_H_ +#define HASHTABLE_PROFILER_H_ + +#ifdef HASHTABLE_PROFILER + +#include +#include + +typedef struct hashtable_profile_t hashtable_profile_t; + +struct hashtable_profile_t { + + /** + * Some stats to profile lookups in the table + */ + struct { + size_t count; + size_t probes; + size_t longest; + } success, failure; + + /** + * Stats on the memory usage of the table + */ + struct { + size_t count; + size_t size; + } max; + + /** + * Keep track of where the hash table was created + */ + backtrace_t *backtrace; +}; + +/** + * Print and cleanup profiling data + */ +static inline void profiler_cleanup(hashtable_profile_t *profile, u_int count, + u_int size) +{ + if (profile->success.count || profile->failure.count) + { + fprintf(stderr, "%zu elements [max. %zu], %zu buckets [%zu], %zu " + "successful / %zu failed lookups, %.4f [%zu] / %.4f " + "[%zu] avg. probes in table created at:", + count, profile->max.count, size, profile->max.size, + profile->success.count, profile->failure.count, + (double)profile->success.probes/profile->success.count, + profile->success.longest, + (double)profile->failure.probes/profile->failure.count, + profile->failure.longest); + profile->backtrace->log(profile->backtrace, stderr, TRUE); + } + profile->backtrace->destroy(profile->backtrace); +} + +/** + * Initialize profiling data + */ +static inline void profiler_init(hashtable_profile_t *profile, int skip) +{ + profile->backtrace = backtrace_create(skip); +} + +#define lookup_start() \ + u_int _lookup_probes = 0; + +#define lookup_probing() \ + _lookup_probes++; + +#define _lookup_done(profile, result) \ + (profile)->result.count++; \ + (profile)->result.probes += _lookup_probes; \ + (profile)->result.longest = max((profile)->result.longest, _lookup_probes); + +#define lookup_success(profile) _lookup_done(profile, success); +#define lookup_failure(profile) _lookup_done(profile, failure); + +static inline void profile_size(hashtable_profile_t *profile, u_int size) +{ + profile->max.size = max(profile->max.size, size); +} + +static inline void profile_count(hashtable_profile_t *profile, u_int count) +{ + profile->max.count = max(profile->max.count, count); +} + +#else /* !HASHTABLE_PROFILER */ + +#define hashtable_profile_t struct {} +#define profiler_cleanup(...) {} +#define profiler_init(...) {} +#define lookup_start(...) {} +#define lookup_probing(...) {} +#define lookup_success(...) {} +#define lookup_failure(...) {} +#define profile_size(...) {} +#define profile_count(...) {} + +#endif /* HASHTABLE_PROFILER */ + +#endif /* HASHTABLE_PROFILER_H_ */ diff --git a/src/libstrongswan/tests/suites/test_hashtable.c b/src/libstrongswan/tests/suites/test_hashtable.c index efc672e63..4e3baba6e 100644 --- a/src/libstrongswan/tests/suites/test_hashtable.c +++ b/src/libstrongswan/tests/suites/test_hashtable.c @@ -358,6 +358,65 @@ START_TEST(test_enumerator) } END_TEST +START_TEST(test_enumerator_order) +{ + char *k1 = "key1", *k2 = "key2", *k3 = "key3", *key; + char *v1 = "val1", *v2 = "val2", *v3 = "val3", *v4 = "val4", *value; + enumerator_t *enumerator; + int count; + + ht->put(ht, k1, v1); + ht->put(ht, k2, v2); + ht->put(ht, k3, v3); + + count = 0; + enumerator = ht->create_enumerator(ht); + while (enumerator->enumerate(enumerator, &key, &value)) + { + switch (count) + { + case 0: + ck_assert(streq(key, k1) && streq(value, v1)); + break; + case 1: + ck_assert(streq(key, k2) && streq(value, v2)); + break; + case 2: + ck_assert(streq(key, k3) && streq(value, v3)); + break; + } + count++; + } + enumerator->destroy(enumerator); + ck_assert_int_eq(count, 3); + + value = ht->remove(ht, k2); + ht->put(ht, k2, v2); + ht->put(ht, k1, v4); + + count = 0; + enumerator = ht->create_enumerator(ht); + while (enumerator->enumerate(enumerator, &key, &value)) + { + switch (count) + { + case 0: + ck_assert(streq(key, k1) && streq(value, v4)); + break; + case 1: + ck_assert(streq(key, k3) && streq(value, v3)); + break; + case 2: + ck_assert(streq(key, k2) && streq(value, v2)); + break; + } + count++; + } + enumerator->destroy(enumerator); + ck_assert_int_eq(count, 3); +} +END_TEST + /******************************************************************************* * remove_at */ @@ -645,6 +704,7 @@ Suite *hashtable_suite_create() tc = tcase_create("enumerator"); tcase_add_checked_fixture(tc, setup_ht, teardown_ht); tcase_add_loop_test(tc, test_enumerator, 0, HASHTABLE_MAX); + tcase_add_test(tc, test_enumerator_order); suite_add_tcase(s, tc); tc = tcase_create("remove_at"); From 736fae4e6c98e52472b6373a4772bd7ea342995a Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 15:16:26 +0200 Subject: [PATCH 09/10] vici: Store configs in a hashtable This makes updates more efficient if many configs are loaded. Configs still have to be enumerated to select them. --- src/libcharon/plugins/vici/vici_config.c | 128 +++++++++++------------ 1 file changed, 62 insertions(+), 66 deletions(-) diff --git a/src/libcharon/plugins/vici/vici_config.c b/src/libcharon/plugins/vici/vici_config.c index 1eb7a24cd..0f8113f92 100644 --- a/src/libcharon/plugins/vici/vici_config.c +++ b/src/libcharon/plugins/vici/vici_config.c @@ -48,6 +48,7 @@ #include #include #include +#include #include #include @@ -102,12 +103,12 @@ struct private_vici_config_t { vici_dispatcher_t *dispatcher; /** - * List of loaded connections, as peer_cfg_t + * Hashtable of loaded connections, as peer_cfg_t */ - linked_list_t *conns; + hashtable_t *conns; /** - * Lock for conns list + * Lock for conns table */ rwlock_t *lock; @@ -133,12 +134,27 @@ struct private_vici_config_t { }; +CALLBACK(peer_filter, bool, + void *data, enumerator_t *orig, va_list args) +{ + peer_cfg_t **out; + + VA_ARGS_VGET(args, out); + + if (orig->enumerate(orig, NULL, out)) + { + return TRUE; + } + return FALSE; +} + METHOD(backend_t, create_peer_cfg_enumerator, enumerator_t*, private_vici_config_t *this, identification_t *me, identification_t *other) { this->lock->read_lock(this->lock); - return enumerator_create_cleaner(this->conns->create_enumerator(this->conns), - (void*)this->lock->unlock, this->lock); + return enumerator_create_filter(this->conns->create_enumerator(this->conns), + peer_filter, this->lock, + (void*)this->lock->unlock); } CALLBACK(ike_filter, bool, @@ -149,7 +165,7 @@ CALLBACK(ike_filter, bool, VA_ARGS_VGET(args, out); - if (orig->enumerate(orig, &cfg)) + if (orig->enumerate(orig, NULL, &cfg)) { *out = cfg->get_ike_cfg(cfg); return TRUE; @@ -169,23 +185,15 @@ METHOD(backend_t, create_ike_cfg_enumerator, enumerator_t*, METHOD(backend_t, get_peer_cfg_by_name, peer_cfg_t*, private_vici_config_t *this, char *name) { - peer_cfg_t *current, *found = NULL; - enumerator_t *enumerator; + peer_cfg_t *found; this->lock->read_lock(this->lock); - enumerator = this->conns->create_enumerator(this->conns); - while (enumerator->enumerate(enumerator, ¤t)) + found = this->conns->get(this->conns, name); + if (found) { - if (streq(current->get_name(current), name)) - { - found = current; - found->get_ref(found); - break; - } + found->get_ref(found); } - enumerator->destroy(enumerator); this->lock->unlock(this->lock); - return found; } @@ -2341,10 +2349,8 @@ static void replace_children(private_vici_config_t *this, */ static void merge_config(private_vici_config_t *this, peer_cfg_t *peer_cfg) { - enumerator_t *enumerator; - peer_cfg_t *current; + peer_cfg_t *found; ike_cfg_t *ike_cfg; - bool merged = FALSE; this->lock->write_lock(this->lock); while (this->handling_actions) @@ -2352,40 +2358,33 @@ static void merge_config(private_vici_config_t *this, peer_cfg_t *peer_cfg) this->condvar->wait(this->condvar, this->lock); } - enumerator = this->conns->create_enumerator(this->conns); - while (enumerator->enumerate(enumerator, ¤t)) + found = this->conns->get(this->conns, peer_cfg->get_name(peer_cfg)); + if (found) { - if (streq(peer_cfg->get_name(peer_cfg), current->get_name(current))) + ike_cfg = found->get_ike_cfg(found); + if (peer_cfg->equals(peer_cfg, found) && + ike_cfg->equals(ike_cfg, peer_cfg->get_ike_cfg(peer_cfg))) { - ike_cfg = current->get_ike_cfg(current); - if (peer_cfg->equals(peer_cfg, current) && - ike_cfg->equals(ike_cfg, peer_cfg->get_ike_cfg(peer_cfg))) - { - DBG1(DBG_CFG, "updated vici connection: %s", - peer_cfg->get_name(peer_cfg)); - replace_children(this, peer_cfg, current); - peer_cfg->destroy(peer_cfg); - } - else - { - DBG1(DBG_CFG, "replaced vici connection: %s", - peer_cfg->get_name(peer_cfg)); - this->conns->insert_before(this->conns, enumerator, peer_cfg); - this->conns->remove_at(this->conns, enumerator); - handle_start_actions(this, current, TRUE); - handle_start_actions(this, peer_cfg, FALSE); - current->destroy(current); - } - merged = TRUE; - break; + DBG1(DBG_CFG, "updated vici connection: %s", + peer_cfg->get_name(peer_cfg)); + replace_children(this, peer_cfg, found); + peer_cfg->destroy(peer_cfg); + } + else + { + DBG1(DBG_CFG, "replaced vici connection: %s", + peer_cfg->get_name(peer_cfg)); + this->conns->put(this->conns, peer_cfg->get_name(peer_cfg), + peer_cfg); + handle_start_actions(this, found, TRUE); + handle_start_actions(this, peer_cfg, FALSE); + found->destroy(found); } } - enumerator->destroy(enumerator); - - if (!merged) + else { DBG1(DBG_CFG, "added vici connection: %s", peer_cfg->get_name(peer_cfg)); - this->conns->insert_last(this->conns, peer_cfg); + this->conns->put(this->conns, peer_cfg->get_name(peer_cfg), peer_cfg); handle_start_actions(this, peer_cfg, FALSE); } this->condvar->signal(this->condvar); @@ -2659,10 +2658,8 @@ CALLBACK(load_conn, vici_message_t*, CALLBACK(unload_conn, vici_message_t*, private_vici_config_t *this, char *name, u_int id, vici_message_t *message) { - enumerator_t *enumerator; peer_cfg_t *cfg; char *conn_name; - bool found = FALSE; conn_name = message->get_str(message, NULL, "name"); if (!conn_name) @@ -2675,23 +2672,16 @@ CALLBACK(unload_conn, vici_message_t*, { this->condvar->wait(this->condvar, this->lock); } - enumerator = this->conns->create_enumerator(this->conns); - while (enumerator->enumerate(enumerator, &cfg)) + cfg = this->conns->remove(this->conns, conn_name); + if (cfg) { - if (streq(cfg->get_name(cfg), conn_name)) - { - this->conns->remove_at(this->conns, enumerator); - handle_start_actions(this, cfg, TRUE); - cfg->destroy(cfg); - found = TRUE; - break; - } + handle_start_actions(this, cfg, TRUE); + cfg->destroy(cfg); } - enumerator->destroy(enumerator); this->condvar->signal(this->condvar); this->lock->unlock(this->lock); - if (!found) + if (!cfg) { return create_reply("unload: connection '%s' not found", conn_name); } @@ -2710,7 +2700,7 @@ CALLBACK(get_conns, vici_message_t*, this->lock->read_lock(this->lock); enumerator = this->conns->create_enumerator(this->conns); - while (enumerator->enumerate(enumerator, &cfg)) + while (enumerator->enumerate(enumerator, NULL, &cfg)) { builder->add_li(builder, "%s", cfg->get_name(cfg)); } @@ -2739,11 +2729,17 @@ static void manage_commands(private_vici_config_t *this, bool reg) manage_command(this, "get-conns", get_conns, reg); } +CALLBACK(destroy_conn, void, + peer_cfg_t *cfg, const void *key) +{ + cfg->destroy(cfg); +} + METHOD(vici_config_t, destroy, void, private_vici_config_t *this) { manage_commands(this, FALSE); - this->conns->destroy_offset(this->conns, offsetof(peer_cfg_t, destroy)); + this->conns->destroy_function(this->conns, destroy_conn); this->condvar->destroy(this->condvar); this->lock->destroy(this->lock); free(this); @@ -2768,7 +2764,7 @@ vici_config_t *vici_config_create(vici_dispatcher_t *dispatcher, .destroy = _destroy, }, .dispatcher = dispatcher, - .conns = linked_list_create(), + .conns = hashtable_create(hashtable_hash_str, hashtable_equals_str, 32), .lock = rwlock_create(RWLOCK_TYPE_DEFAULT), .condvar = rwlock_condvar_create(), .authority = authority, From 93b2c2066fad82ce3249dcbe22e8a861438e75c0 Mon Sep 17 00:00:00 2001 From: Tobias Brunner Date: Fri, 24 Apr 2020 19:11:25 +0200 Subject: [PATCH 10/10] hashtable: Use quadratic probing This reduces the clustering problem (primary clustering) but is not completely free of it (secondary clustering) it still reduces the maximum and average probing lengths. --- src/libstrongswan/collections/hashtable.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/libstrongswan/collections/hashtable.c b/src/libstrongswan/collections/hashtable.c index 8c290ac2d..fa1e4a093 100644 --- a/src/libstrongswan/collections/hashtable.c +++ b/src/libstrongswan/collections/hashtable.c @@ -272,11 +272,13 @@ static void init_hashtable(private_hashtable_t *this, u_int size) } /** - * Calculate the next bucket using simple linear probing for now. + * Calculate the next bucket using quadratic probing (the sequence is h(k) + 1, + * h(k) + 3, h(k) + 6, h(k) + 10, ...). */ -static inline u_int get_next(private_hashtable_t *this, u_int row) +static inline u_int get_next(private_hashtable_t *this, u_int row, u_int *p) { - return (row + 1) & this->mask; + *p += 1; + return (row + *p) & this->mask; } /** @@ -287,7 +289,7 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, u_int *out_hash, u_int *out_row) { pair_t *pair; - u_int hash, row, removed, index; + u_int hash, row, p = 0, removed, index; bool found_removed = FALSE; if (!this->count && !out_hash && !out_row) @@ -318,7 +320,7 @@ static inline pair_t *find_key(private_hashtable_t *this, const void *key, lookup_success(&this->profile); return pair; } - row = get_next(this, row); + row = get_next(this, row, &p); index = get_index(this, row); } if (out_hash) @@ -354,7 +356,7 @@ static inline u_int insert_item(private_hashtable_t *this, u_int row) static bool rehash(private_hashtable_t *this, u_int size) { pair_t *old_items, *pair; - u_int old_count, i, row, index; + u_int old_count, i, p, row, index; if (size > MAX_SIZE) { @@ -378,9 +380,9 @@ static bool rehash(private_hashtable_t *this, u_int size) { row = pair->hash & this->mask; index = get_index(this, row); - while (index) + for (p = 0; index;) { - row = get_next(this, row); + row = get_next(this, row, &p); index = get_index(this, row); } index = insert_item(this, row);