Fixed u_arraylist to correct performance and missing calls.

Corrected poor memory management to not realloc() on each remove
call, use a normal approach to growth of the backing store, and
some general cleanup. Added calls for reserving space and for
requesting reduction of the backing store.

These were all issues called out in the intial code review.

Simple cleanup improved performance of the contains function
reducing its time by 40%-50% (~ 36ms down to 21ms-19ms).

With the backing store corrections, repeted add/remove testing was
improved with its time being cut by 75% (36.8ms down to 9.4ms).

(Times should be taking as very rough, since runs were done using
valgrind to slow things enough to actually measure.)

Change-Id: I81e42f2fdbec8fb7fb18f16f09cfa5008640a2d4
Signed-off-by: Jon A. Cruz <jonc@osg.samsung.com>
Reviewed-on: https://gerrit.iotivity.org/gerrit/2293
Reviewed-by: Habib Virji <habib.virji@samsung.com>
Tested-by: Habib Virji <habib.virji@samsung.com>
Tested-by: jenkins-iotivity <jenkins-iotivity@opendaylight.org>

diff --git a/resource/csdk/connectivity/common/inc/uarraylist.h b/resource/csdk/connectivity/common/inc/uarraylist.h
index 7e8403a..1457e31 100644 (file)
--- a/resource/csdk/connectivity/common/inc/uarraylist.h
+++ b/resource/csdk/connectivity/common/inc/uarraylist.h
@@ -58,6 +58,28 @@ u_arraylist_t *u_arraylist_create();
 void u_arraylist_free(u_arraylist_t **list);
 
 /**
+ * Request that the list prepare room for the specified number of entries.
+ * If count is greater than the current internal storage size then an
+ * an attempt will be made to reallocate room for at least count items.
+ *
+ * In other cases there will be no effect.
+ *
+ * This call will not affect the length used and cannot be used to remove
+ * entries.
+ * @param list the list to operate on.
+ * @param count the size to attempt to reserve room for.
+ */
+void u_arraylist_reserve(u_arraylist_t *list, size_t count);
+
+/**
+ * Request that the storage in the list be reduced to fit its current length.
+ *
+ * The request is non-binding, and may not affect the entries in the list.
+ * @param list the list to operate on.
+ */
+void u_arraylist_shrink_to_fit(u_arraylist_t *list);
+
+/**
  * Returns the data of the index from the array list.
  * @param[in] list         pointer of array list.
  * @param[in] index        index of array list.

diff --git a/resource/csdk/connectivity/common/src/uarraylist.c b/resource/csdk/connectivity/common/src/uarraylist.c
index 8cc88f4..d205aa5 100644 (file)
--- a/resource/csdk/connectivity/common/src/uarraylist.c
+++ b/resource/csdk/connectivity/common/src/uarraylist.c
@@ -33,18 +33,17 @@
 
 u_arraylist_t *u_arraylist_create()
 {
-    u_arraylist_t *list = NULL;
-
-    list = (u_arraylist_t *) OICMalloc(sizeof(u_arraylist_t));
+    u_arraylist_t *list = (u_arraylist_t *) OICMalloc(sizeof(u_arraylist_t));
     if (!list)
     {
+        OIC_LOG(DEBUG, TAG, "Out of memory");
         return NULL;
     }
 
     list->size = U_ARRAYLIST_DEFAULT_SIZE;
     list->length = 0;
 
-    list->data = (void *) OICMalloc(list->size * sizeof(void *));
+    list->data = (void **) OICMalloc(list->size * sizeof(list->data[0]));
     if (!list->data)
     {
         OIC_LOG(DEBUG, TAG, "Out of memory");
@@ -67,6 +66,49 @@ void u_arraylist_free(u_arraylist_t **list)
     *list = NULL;
 }
 
+void u_arraylist_reserve(u_arraylist_t *list, size_t count)
+{
+    if (list && (count > list->size))
+    {
+        void *tmp = OICRealloc(list->data, count * sizeof(list->data[0]));
+        if (!tmp)
+        {
+            OIC_LOG(DEBUG, TAG, "Memory reallocation failed.");
+            // Note that this is considered non-fatal.
+        }
+        else
+        {
+            list->data = (void **) tmp;
+            list->size = count;
+        }
+    }
+}
+
+void u_arraylist_shrink_to_fit(u_arraylist_t *list)
+{
+    if (!list)
+    {
+        return;
+    }
+
+    if ((list->size > list->length)
+        && (list->length >= U_ARRAYLIST_DEFAULT_SIZE))
+    {
+        void *tmp = OICRealloc(list->data,
+                               list->length * sizeof(list->data[0]));
+        if (!tmp)
+        {
+            OIC_LOG(DEBUG, TAG, "Memory reallocation failed.");
+            // Considered non-fatal as this call is non-binding.
+        }
+        else
+        {
+            list->data = (void **) tmp;
+            list->size = list->length;
+        }
+    }
+}
+
 void *u_arraylist_get(const u_arraylist_t *list, uint32_t index)
 {
     if (!list )
@@ -84,6 +126,7 @@ void *u_arraylist_get(const u_arraylist_t *list, uint32_t index)
 
 bool u_arraylist_add(u_arraylist_t *list, void *data)
 {
+    static const double GROWTH_FACTOR = 1.5;
     if (!list)
     {
         return false;
@@ -91,14 +134,18 @@ bool u_arraylist_add(u_arraylist_t *list, void *data)
 
     if (list->size <= list->length)
     {
-        uint32_t new_size = list->size + 1;
-        if (!(list->data = (void **) realloc(list->data, new_size * sizeof(void *))))
+        uint32_t new_size = (list->size * GROWTH_FACTOR) + 0.5;
+        // In case the re-alloc returns null, use a local variable to avoid
+        // losing the current block of memory.
+        void *tmp = OICRealloc(list->data, new_size * sizeof(list->data[0]));
+        if (!tmp)
         {
-            OIC_LOG(ERROR, TAG, "Failed to re-allocation memory");
+            OIC_LOG(DEBUG, TAG, "Memory reallocation failed.");
             return false;
         }
-
-        memset(list->data + list->size, 0, (new_size - list->size) * sizeof(void *));
+        list->data = (void **) tmp;
+        memset(list->data + list->size, 0,
+               (new_size - list->size) * sizeof(list->data[0]));
         list->size = new_size;
     }
 
@@ -112,12 +159,7 @@ void *u_arraylist_remove(u_arraylist_t *list, uint32_t index)
 {
     void *removed = NULL;
 
-    if (!list)
-    {
-        return NULL;
-    }
-
-    if (index >= list->length)
+    if (!list || (index >= list->length))
     {
         return NULL;
     }
@@ -126,21 +168,13 @@ void *u_arraylist_remove(u_arraylist_t *list, uint32_t index)
 
     if (index < list->length - 1)
     {
-        memmove(&list->data[index], &list->data[index + 1],
-                (list->length - index - 1) * sizeof(void *));
+        memmove(&list->data[index],
+                &list->data[index + 1],
+                (list->length - index - 1) * sizeof(list->data[0]));
     }
 
-    list->size--;
     list->length--;
 
-    // check minimum size.
-    list->size = (list->size <= U_ARRAYLIST_DEFAULT_SIZE) ? U_ARRAYLIST_DEFAULT_SIZE : list->size;
-
-    if (!(list->data = (void **) realloc(list->data, list->size * sizeof(void *))))
-    {
-        return NULL;
-    }
-
     return removed;
 }
 
@@ -154,20 +188,16 @@ uint32_t u_arraylist_length(const u_arraylist_t *list)
     return list->length;
 }
 
-bool u_arraylist_contains(const u_arraylist_t *list,const void *data)
+bool u_arraylist_contains(const u_arraylist_t *list, const void *data)
 {
-    uint32_t i = 0;
-
     if (!list)
     {
         return false;
     }
 
-    uint32_t length = u_arraylist_length(list);
-
-    for (i = 0; i < length; i++)
+    for (uint32_t i = 0; i < list->size; i++)
     {
-        if (data == u_arraylist_get(list, i))
+        if (data == list->data[i])
         {
             return true;
         }
@@ -183,10 +213,9 @@ void u_arraylist_destroy(u_arraylist_t *list)
     {
         return;
     }
-    uint32_t len = u_arraylist_length(list);
-    for (uint32_t i = 0; i < len; i++)
+    for (uint32_t i = 0; i < list->length; i++)
     {
-        OICFree(u_arraylist_get(list, i));
+        OICFree(list->data[i]);
     }
     (void)u_arraylist_free(&list);
 }

diff --git a/resource/csdk/connectivity/test/uarraylist_test.cpp b/resource/csdk/connectivity/test/uarraylist_test.cpp
index d99604d..167835c 100644 (file)
--- a/resource/csdk/connectivity/test/uarraylist_test.cpp
+++ b/resource/csdk/connectivity/test/uarraylist_test.cpp
@@ -107,6 +107,61 @@ TEST_F(UArrayListF, LengthMulti)
     ASSERT_EQ(static_cast<uint32_t>(1000), u_arraylist_length(list));
 }
 
+TEST_F(UArrayListF, NoReserve)
+{
+    static const int PAD_SIZE = 10000;
+
+    int dummy = 0;
+
+    //u_arraylist_reserve(list, PAD_SIZE);
+
+    for (int i = 0; i < PAD_SIZE; ++i)
+    {
+        bool rc = u_arraylist_add(list, &dummy);
+        ASSERT_TRUE(rc);
+    }
+}
+
+TEST_F(UArrayListF, Reserve)
+{
+    static const int PAD_SIZE = 10000;
+
+    int dummy = 0;
+
+    u_arraylist_reserve(list, PAD_SIZE);
+
+    for (int i = 0; i < PAD_SIZE; ++i)
+    {
+        bool rc = u_arraylist_add(list, &dummy);
+        ASSERT_TRUE(rc);
+    }
+}
+
+
+TEST_F(UArrayListF, ShrinkToFit)
+{
+    static const int PAD_SIZE = 100;
+
+    int dummy = 0;
+
+    u_arraylist_reserve(list, PAD_SIZE);
+
+    for (int i = 0; i < PAD_SIZE; ++i)
+    {
+        bool rc = u_arraylist_add(list, &dummy);
+        ASSERT_TRUE(rc);
+    }
+
+    for (int i = PAD_SIZE; i > 0; --i)
+    {
+        u_arraylist_remove(list, i);
+    }
+
+    EXPECT_GT(list->size, list->length);
+    u_arraylist_shrink_to_fit(list);
+    EXPECT_EQ(list->size, list->length);
+}
+
 TEST_F(UArrayListF, Get)
 {
     ASSERT_EQ(static_cast<uint32_t>(0), u_arraylist_length(list));
@@ -144,16 +199,13 @@ TEST_F(UArrayListF, Remove)
     ASSERT_EQ(static_cast<uint32_t>(1000), u_arraylist_length(list));
 
     // Remove walking forward so as to have a non-trivial case.
-    uint32_t idx = 0;
-    uint32_t old = 0;
-    while (idx < u_arraylist_length(list))
+    for (uint32_t idx = 0, old = 0;
+         idx < u_arraylist_length(list);
+         ++idx, old += 2)
     {
         void *value = u_arraylist_remove(list, idx);
         ASSERT_TRUE(value != NULL);
         ASSERT_EQ(value, &dummy[old]);
-
-        old += 2;
-        idx++; // remove call reduces by one, so this makes two.
     }
     ASSERT_EQ(static_cast<uint32_t>(500), u_arraylist_length(list));
 }
@@ -173,16 +225,13 @@ TEST_F(UArrayListF, Contains)
     ASSERT_EQ(static_cast<uint32_t>(1000), u_arraylist_length(list));
 
     // Remove walking forward so as to have a non-trivial case.
-    uint32_t idx = 0;
-    uint32_t old = 0;
-    while (idx < u_arraylist_length(list))
+    for (uint32_t idx = 0, old = 0;
+         idx < u_arraylist_length(list);
+         ++idx, old += 2)
     {
         void *value = u_arraylist_remove(list, idx);
         ASSERT_TRUE(value != NULL);
         ASSERT_EQ(value, &dummy[old]);
-
-        old += 2;
-        idx++; // remove call reduces by one, so this makes two.
     }
     ASSERT_EQ(static_cast<uint32_t>(500), u_arraylist_length(list));