From cb62bebadf62613b770f79cd4fd43e20ec7db10c Mon Sep 17 00:00:00 2001 From: David Steele Date: Sat, 28 Sep 2019 10:08:20 -0400 Subject: [PATCH] Use bsearch() on sorted lists rather than an iterative method. bsearch() is far more efficient than an iterative approach except in the most trivial cases. For now insert will reset the sort order to none and the list will need to be resorted before bsearch() can be used. This is necessary because item pointers are not stable after a sort, i.e. they can move around. Until lists are stable it's not a good idea to surprise the caller by mixing up their pointers on insert. --- src/common/type/list.c | 115 ++++++++++++++++++------ src/common/type/list.h | 3 + src/common/type/stringList.c | 26 +----- test/define.yaml | 2 +- test/src/module/common/typeListTest.c | 37 ++++++++ test/src/module/common/typeStringTest.c | 1 + 6 files changed, 132 insertions(+), 52 deletions(-) diff --git a/src/common/type/list.c b/src/common/type/list.c index 5f386e4eb..c61b43b90 100644 --- a/src/common/type/list.c +++ b/src/common/type/list.c @@ -21,6 +21,7 @@ struct List size_t itemSize; unsigned int listSize; unsigned int listSizeMax; + SortOrder sortOrder; unsigned char *list; ListComparator *comparator; }; @@ -38,7 +39,7 @@ lstNew(size_t itemSize) FUNCTION_TEST_PARAM(SIZE, itemSize); FUNCTION_TEST_END(); - FUNCTION_TEST_RETURN(lstNewP(itemSize, .comparator = lstComparatorStr)); + FUNCTION_TEST_RETURN(lstNewP(itemSize)); } List * @@ -57,6 +58,7 @@ lstNewParam(size_t itemSize, ListParam param) this = memNew(sizeof(List)); this->memContext = MEM_CONTEXT_NEW(); this->itemSize = itemSize; + this->sortOrder = param.sortOrder; this->comparator = param.comparator; } MEM_CONTEXT_NEW_END(); @@ -109,7 +111,7 @@ lstClear(List *this) } /*********************************************************************************************************************************** -Compare Strings or structs with a String as the first member +Compare Strings ascending or structs with a String as the first member ***********************************************************************************************************************************/ int lstComparatorStr(const void *item1, const void *item2) @@ -125,6 +127,17 @@ lstComparatorStr(const void *item1, const void *item2) FUNCTION_TEST_RETURN(strCmp(*(String **)item1, *(String **)item2)); } +/*********************************************************************************************************************************** +General function for a descending comparator that simply switches the parameters on the main comparator (which should be asc) +***********************************************************************************************************************************/ +static const List *comparatorDescList = NULL; + +static int +lstComparatorDesc(const void *item1, const void *item2) +{ + return comparatorDescList->comparator(item2, item1); +} + /*********************************************************************************************************************************** Get an item from the list ***********************************************************************************************************************************/ @@ -146,9 +159,57 @@ lstGet(const List *this, unsigned int listIdx) FUNCTION_TEST_RETURN(this->list + (listIdx * this->itemSize)); } +/*********************************************************************************************************************************** +Does an item exist in the list? +***********************************************************************************************************************************/ +bool +lstExists(const List *this, const void *item) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(LIST, this); + FUNCTION_TEST_PARAM_P(VOID, item); + FUNCTION_TEST_END(); + + ASSERT(this != NULL); + ASSERT(item != NULL); + + FUNCTION_TEST_RETURN(lstFind(this, item) != NULL); +} + /*********************************************************************************************************************************** Find an item in the list ***********************************************************************************************************************************/ +void * +lstFind(const List *this, const void *item) +{ + FUNCTION_TEST_BEGIN(); + FUNCTION_TEST_PARAM(LIST, this); + FUNCTION_TEST_PARAM_P(VOID, item); + FUNCTION_TEST_END(); + + ASSERT(this != NULL); + ASSERT(item != NULL); + + if (this->sortOrder == sortOrderAsc) + FUNCTION_TEST_RETURN(bsearch(item, this->list, this->listSize, this->itemSize, this->comparator)); + else if (this->sortOrder == sortOrderDesc) + { + // Assign the list for the descending comparator to use + comparatorDescList = this; + + FUNCTION_TEST_RETURN(bsearch(item, this->list, this->listSize, this->itemSize, lstComparatorDesc)); + } + + // Fall back on an iterative search + for (unsigned int listIdx = 0; listIdx < lstSize(this); listIdx++) + { + if (this->comparator(item, lstGet(this, listIdx)) == 0) + FUNCTION_TEST_RETURN(lstGet(this, listIdx)); + } + + FUNCTION_TEST_RETURN(NULL); +} + unsigned int lstFindIdx(const List *this, const void *item) { @@ -160,18 +221,9 @@ lstFindIdx(const List *this, const void *item) ASSERT(this != NULL); ASSERT(item != NULL); - unsigned int result = LIST_NOT_FOUND; + void *result = lstFind(this, item); - for (unsigned int listIdx = 0; listIdx < lstSize(this); listIdx++) - { - if (this->comparator(item, lstGet(this, listIdx)) == 0) - { - result = listIdx; - break; - } - } - - FUNCTION_TEST_RETURN(result); + FUNCTION_TEST_RETURN(result == NULL ? LIST_NOT_FOUND : lstIdx(this, result)); } void * @@ -186,13 +238,16 @@ lstFindDefault(const List *this, const void *item, void *itemDefault) ASSERT(this != NULL); ASSERT(item != NULL); - unsigned int listIdx = lstFindIdx(this, item); + void *result= lstFind(this, item); - FUNCTION_TEST_RETURN(listIdx == LIST_NOT_FOUND ? itemDefault : lstGet(this, listIdx)); + FUNCTION_TEST_RETURN(result == NULL ? itemDefault : result); } -void * -lstFind(const List *this, const void *item) +/*********************************************************************************************************************************** +Get the index of a list item +***********************************************************************************************************************************/ +unsigned int +lstIdx(const List *this, const void *item) { FUNCTION_TEST_BEGIN(); FUNCTION_TEST_PARAM(LIST, this); @@ -202,7 +257,15 @@ lstFind(const List *this, const void *item) ASSERT(this != NULL); ASSERT(item != NULL); - FUNCTION_TEST_RETURN(lstFindDefault(this, item, NULL)); + // Item pointers should always be aligned with the beginning of an item in the list + ASSERT((size_t)((unsigned char * const)item - this->list) % this->itemSize == 0); + + size_t result = (size_t)((unsigned char * const)item - this->list) / this->itemSize; + + // Item pointers should always be in range + ASSERT(result < this->listSize); + + FUNCTION_TEST_RETURN((unsigned int)result); } /*********************************************************************************************************************************** @@ -248,6 +311,7 @@ lstInsert(List *this, unsigned int listIdx, const void *item) memmove(this->list + ((listIdx + 1) * this->itemSize), itemPtr, (lstSize(this) - listIdx) * this->itemSize); // Copy item into the list + this->sortOrder = sortOrderNone; memcpy(itemPtr, item, this->itemSize); this->listSize++; @@ -332,14 +396,6 @@ lstSize(const List *this) /*********************************************************************************************************************************** List sort ***********************************************************************************************************************************/ -static List *lstSortList = NULL; - -static int -lstSortDescComparator(const void *item1, const void *item2) -{ - return lstSortList->comparator(item2, item1); -} - List * lstSort(List *this, SortOrder sortOrder) { @@ -361,9 +417,9 @@ lstSort(List *this, SortOrder sortOrder) case sortOrderDesc: { // Assign the list that will be sorted for the comparator function to use - lstSortList = this; + comparatorDescList = this; - qsort(this->list, this->listSize, this->itemSize, lstSortDescComparator); + qsort(this->list, this->listSize, this->itemSize, lstComparatorDesc); break; } @@ -371,6 +427,8 @@ lstSort(List *this, SortOrder sortOrder) break; } + this->sortOrder = sortOrder; + FUNCTION_TEST_RETURN(this); } @@ -388,6 +446,7 @@ lstComparatorSet(List *this, ListComparator *comparator) ASSERT(this != NULL); this->comparator = comparator; + this->sortOrder = sortOrderNone; FUNCTION_TEST_RETURN(this); } diff --git a/src/common/type/list.h b/src/common/type/list.h index 811e27d82..aa456116d 100644 --- a/src/common/type/list.h +++ b/src/common/type/list.h @@ -54,6 +54,7 @@ List *lstNew(size_t itemSize); typedef struct ListParam { + SortOrder sortOrder; ListComparator *comparator; } ListParam; @@ -68,9 +69,11 @@ Functions void *lstAdd(List *this, const void *item); List *lstClear(List *this); void *lstGet(const List *this, unsigned int listIdx); +bool lstExists(const List *this, const void *item); void *lstFind(const List *this, const void *item); void *lstFindDefault(const List *this, const void *item, void *itemDefault); unsigned int lstFindIdx(const List *this, const void *item); +unsigned int lstIdx(const List *this, const void *item); void *lstInsert(List *this, unsigned int listIdx, const void *item); bool lstRemove(List *this, const void *item); List *lstRemoveIdx(List *this, unsigned int listIdx); diff --git a/src/common/type/stringList.c b/src/common/type/stringList.c index e7ae45264..646a4fa4b 100644 --- a/src/common/type/stringList.c +++ b/src/common/type/stringList.c @@ -281,18 +281,7 @@ strLstExists(const StringList *this, const String *string) ASSERT(this != NULL); - bool result = false; - - for (unsigned int listIdx = 0; listIdx < strLstSize(this); listIdx++) - { - if (strEq(strLstGet(this, listIdx), string)) - { - result = true; - break; - } - } - - FUNCTION_TEST_RETURN(result); + FUNCTION_TEST_RETURN(lstExists((List *)this, &string)); } bool @@ -305,18 +294,9 @@ strLstExistsZ(const StringList *this, const char *cstring) ASSERT(this != NULL); - bool result = false; + const String *string = cstring == NULL ? NULL : STR(cstring); - for (unsigned int listIdx = 0; listIdx < strLstSize(this); listIdx++) - { - if (strEqZ(strLstGet(this, listIdx), cstring)) - { - result = true; - break; - } - } - - FUNCTION_TEST_RETURN(result); + FUNCTION_TEST_RETURN(lstExists((List *)this, &string)); } /*********************************************************************************************************************************** diff --git a/test/define.yaml b/test/define.yaml index 928ecbed3..9ea0c7c99 100644 --- a/test/define.yaml +++ b/test/define.yaml @@ -157,7 +157,7 @@ unit: # ---------------------------------------------------------------------------------------------------------------------------- - name: type-list - total: 3 + total: 4 coverage: common/type/list: full diff --git a/test/src/module/common/typeListTest.c b/test/src/module/common/typeListTest.c index 88f81b098..c3db22253 100644 --- a/test/src/module/common/typeListTest.c +++ b/test/src/module/common/typeListTest.c @@ -1,6 +1,7 @@ /*********************************************************************************************************************************** Test Lists ***********************************************************************************************************************************/ +#include "common/time.h" /*********************************************************************************************************************************** Test sort comparator @@ -59,7 +60,10 @@ testRun(void) String *string3 = strNew("string3"); TEST_RESULT_PTR(lstFindDefault(list, &string3, (void *)1), (void *)1, " find string3 returns default"); + TEST_RESULT_BOOL(lstExists(list, &string3), false, " string3 does not exist"); TEST_RESULT_STR(strPtr(*(String **)lstFind(list, &string2)), "string2", " find string2"); + TEST_RESULT_STR(strPtr(*(String **)lstFindDefault(list, &string2, NULL)), "string2", " find string2 no default"); + TEST_RESULT_BOOL(lstExists(list, &string2), true, " string2 exists"); TEST_RESULT_BOOL(lstRemove(list, &string2), true, " remove string2"); TEST_RESULT_BOOL(lstRemove(list, &string2), false, " unable to remove string2"); @@ -155,5 +159,38 @@ testRun(void) TEST_RESULT_INT(*((int *)lstGet(list, 3)), 2, "sort value 3"); } + // ***************************************************************************************************************************** + if (testBegin("performance")) + { + int testMax = 2000; + List *list = lstNewP(sizeof(int), .comparator = testComparator); + + // Generate a large list of values (use int instead of string so there fewer allocations) + for (int listIdx = 0; listIdx < testMax; listIdx++) + lstAdd(list, &listIdx); + + TEST_RESULT_UINT(lstSize(list), testMax, "check list total"); + + // Search for all values with an ascending sort + lstSort(list, sortOrderAsc); + + TimeMSec timeBegin = timeMSec(); + + for (int listIdx = 0; listIdx < testMax; listIdx++) + CHECK(*(int *)lstFind(list, &listIdx) == listIdx); + + TEST_LOG_FMT("asc search completed in %ums", (unsigned int)(timeMSec() - timeBegin)); + + // Search for all values with an descending sort + lstSort(list, sortOrderDesc); + + timeBegin = timeMSec(); + + for (int listIdx = 0; listIdx < testMax; listIdx++) + CHECK(*(int *)lstFind(list, &listIdx) == listIdx); + + TEST_LOG_FMT("desc search completed in %ums", (unsigned int)(timeMSec() - timeBegin)); + } + FUNCTION_HARNESS_RESULT_VOID(); } diff --git a/test/src/module/common/typeStringTest.c b/test/src/module/common/typeStringTest.c index 65a9df394..b61043651 100644 --- a/test/src/module/common/typeStringTest.c +++ b/test/src/module/common/typeStringTest.c @@ -312,6 +312,7 @@ testRun(void) TEST_RESULT_VOID(strLstAddIfMissing(list, STRDEF("item1")), "add item 1"); TEST_RESULT_UINT(strLstSize(list), 1, "check size"); TEST_RESULT_BOOL(strLstExistsZ(list, "item1"), true, "check exists"); + TEST_RESULT_BOOL(strLstExistsZ(list, NULL), false, "check null exists"); TEST_RESULT_VOID(strLstAddIfMissing(list, STRDEF("item1")), "add item 1 again"); TEST_RESULT_UINT(strLstSize(list), 1, "check size");