Switch ID code to use a hash table instead of a skip list (#52)

* Brings hash table ID code over from Bitbucket branch

* Includes reformatting via clang.

* Excludes uthash.h from reformatting.

* Still has the failing test issue in tid.c. This should only be a
  problem if a custom ID type is used and its free function deletes
  other IDs.

* Fixes munged H5_GCC_DIAG_ON/OFF macros in H5I.c

The H5_GCC_DIAG_ON/OFF macros used to turn off fallthrough warnings
in uthash.h (external code) were munged when formatting with clang
due to their lack of quotes.

e.g.;

    H5_GCC_DIAG_OFF(implicit-fallthrough)

was munged to:

    H5_GCC_DIAG_OFF(implicit - fallthrough)

which compiles, but is useless. So, with quotes, this is now:

    H5_GCC_DIAG_OFF("implicit-fallthrough")

which survives reformatting with clang.

* Fixes issues with user callbacks in the ID hash tables

The skip lists (previously) used to handle IDs use a mark-and-sweep
scheme to deal with user-defined ID delete callbacks which themselves delete
other IDs in the list. The uthash hash table implementation used to manage
the IDs in this feature branch does not have this ability.

This commit restores the skip lists for non-library ID types in lieu of
significantly modifying the uthash code. The hash tables are used to
manage the library IDs as those do not delete other IDs when they are
closed.

* Adds uthash.h to MANIFEST

* Removes implicit-fallthrough diagnostic disable

Removing -Wimplicit-fallthrough=5 means that the uthash code
no longer raises warnings so the H5_GCC_DIAG_OFF/ON macros
that disabled those warnings have been removed from H5I.c.

* Adds a test to ensure you can delete IDs in the H5Iiterate() callback

.github/workflows/clang-format-check.yml

@@ -14,4 +14,4 @@ jobs:
         extensions: 'c,h,cpp,hpp'
         clangFormatVersion: 10
         style: file
-        exclude: './config ./hl/src/H5LTanalyze.c ./hl/src/H5LTparse.c ./hl/src/H5LTparse.h ./src/H5Epubgen.h ./src/H5Einit.h ./src/H5Eterm.h ./src/H5Edefin.h ./src/H5version.h ./src/H5overflow.h'
+        exclude: './config ./hl/src/H5LTanalyze.c ./hl/src/H5LTparse.c ./hl/src/H5LTparse.h ./src/H5Epubgen.h ./src/H5Einit.h ./src/H5Eterm.h ./src/H5Edefin.h ./src/H5version.h ./src/H5overflow.h ./src/uthash.h'

MANIFEST

@@ -1043,10 +1043,11 @@
 ./src/H5Zshuffle.c
 ./src/H5Zszip.c
 ./src/H5Ztrans.c
+./src/H5win32defs.h
 ./src/Makefile.am
 ./src/hdf5.h
 ./src/libhdf5.settings.in
-./src/H5win32defs.h
+./src/uthash.h
 
 ./test/AtomicWriterReader.txt
 ./test/COPYING

bin/format_source

@@ -19,6 +19,7 @@ find . -type d \( -path ./config \) -prune \
         -or -name H5Edefin.h \
         -or -name H5version.h \
         -or -name H5overflow.h \
+        -or -name uthash.h \
         \) \) \
     -and \( -iname *.h -or -iname *.c -or -iname *.cpp -or -iname *.hpp \) \) \
     | xargs clang-format -style=file -i -fallback-style=none

bin/format_source_patch

@@ -19,6 +19,7 @@ find . -type d \( -path ./config \) -prune \
         -or -name H5Edefin.h \
         -or -name H5version.h \
         -or -name H5overflow.h \
+        -or -name uthash.h \
         \) \) \
     -and \( -iname *.h -or -iname *.c -or -iname *.cpp -or -iname *.hpp \) \) \
     | xargs clang-format -style=file -i -fallback-style=none

src/H5I.c

@@ -43,6 +43,8 @@
 #include "H5Tpkg.h"      /* Datatypes                                */
 #include "H5VLprivate.h" /* Virtual Object Layer                     */
 
+#include "uthash.h" /* Hash table functionality                 */
+
 /* Local Macros */
 
 /* Combine a Type number and an atom index into an atom */
@@ -52,10 +54,11 @@
 
 /* Atom information structure used */
 typedef struct H5I_id_info_t {
-    hid_t       id;        /* ID for this info                */
-    unsigned    count;     /* ref. count for this atom            */
-    unsigned    app_count; /* ref. count of application visible atoms  */
-    const void *obj_ptr;   /* pointer associated with the atom        */
+    hid_t          id;        /* ID for this info                */
+    unsigned       count;     /* ref. count for this atom            */
+    unsigned       app_count; /* ref. count of application visible atoms  */
+    const void *   obj_ptr;   /* pointer associated with the atom        */
+    UT_hash_handle hh;        /* Hash table handle (must be LAST) */
 } H5I_id_info_t;
 
 /* ID type structure used */
@@ -65,7 +68,14 @@ typedef struct {
     uint64_t           id_count;   /* Current number of IDs held            */
     uint64_t           nextid;     /* ID to use for the next atom            */
     H5I_id_info_t *    last_info;  /* Info for most recent ID looked up        */
-    H5SL_t *           ids;        /* Pointer to skip list that stores IDs     */
+    /* Library IDs are stored in a hash table, user IDs are stored in a skip
+     * list, which is slower but handles the case where ID delete callbacks
+     * delete other IDs via a mark-and-sweep scheme.
+     */
+    union {
+        H5I_id_info_t *hash_table; /* Hash table pointer for this ID type */
+        H5SL_t *       skip_list;  /* Pointer to skip list that stores IDs */
+    } ids;
 } H5I_id_type_t;
 
 typedef struct {
@@ -169,15 +179,25 @@ H5I_term_package(void)
 
         /* How many types are still being used? */
         for (type = 0; type < H5I_next_type; type++)
-            if ((type_ptr = H5I_id_type_list_g[type]) && type_ptr->ids)
-                n++;
+            if (H5I_IS_LIB_TYPE(type)) {
+                if ((type_ptr = H5I_id_type_list_g[type]) && type_ptr->ids.hash_table)
+                    n++;
+            }
+            else {
+                if ((type_ptr = H5I_id_type_list_g[type]) && type_ptr->ids.skip_list)
+                    n++;
+            }
 
         /* If no types are used then clean up */
         if (0 == n) {
             for (type = 0; type < H5I_next_type; type++) {
                 type_ptr = H5I_id_type_list_g[type];
                 if (type_ptr) {
-                    HDassert(NULL == type_ptr->ids);
+                    if (H5I_IS_LIB_TYPE(type))
+                        HDassert(NULL == type_ptr->ids.hash_table);
+                    else
+                        HDassert(NULL == type_ptr->ids.skip_list);
+
                     type_ptr                 = H5FL_FREE(H5I_id_type_t, type_ptr);
                     H5I_id_type_list_g[type] = NULL;
                     n++;
@@ -313,8 +333,12 @@ H5I_register_type(const H5I_class_t *cls)
         type_ptr->id_count  = 0;
         type_ptr->nextid    = cls->reserved;
         type_ptr->last_info = NULL;
-        if (NULL == (type_ptr->ids = H5SL_create(H5SL_TYPE_HID, NULL)))
-            HGOTO_ERROR(H5E_ATOM, H5E_CANTCREATE, FAIL, "skip list creation failed")
+        if (H5I_IS_LIB_TYPE(cls->type_id))
+            type_ptr->ids.hash_table = NULL;
+        else {
+            if (NULL == (type_ptr->ids.skip_list = H5SL_create(H5SL_TYPE_HID, NULL)))
+                HGOTO_ERROR(H5E_ATOM, H5E_CANTCREATE, FAIL, "skip list creation failed")
+        }
     } /* end if */
 
     /* Increment the count of the times this type has been initialized */
@@ -323,9 +347,10 @@ H5I_register_type(const H5I_class_t *cls)
 done:
     if (ret_value < 0) { /* Clean up on error */
         if (type_ptr) {
-            if (type_ptr->ids)
-                H5SL_close(type_ptr->ids);
-            (void)H5FL_FREE(H5I_id_type_t, type_ptr);
+            if (FALSE == H5I_IS_LIB_TYPE(cls->type_id))
+                if (type_ptr->ids.skip_list)
+                    H5SL_close(type_ptr->ids.skip_list);
+            H5FL_FREE(H5I_id_type_t, type_ptr);
         } /* end if */
     }     /* end if */
 
@@ -556,8 +581,23 @@ H5I_clear_type(H5I_type_t type, hbool_t force, hbool_t app_ref)
     udata.force   = force;
     udata.app_ref = app_ref;
 
-    /* Attempt to free all ids in the type */
-    if (H5SL_try_free_safe(udata.type_ptr->ids, H5I__clear_type_cb, &udata) < 0)
+    if (H5I_IS_LIB_TYPE(type)) {
+        H5I_id_info_t *item = NULL;
+        H5I_id_info_t *tmp  = NULL;
+
+        /* This is a "delete-safe" iteration */
+        HASH_ITER(hh, udata.type_ptr->ids.hash_table, item, tmp)
+        {
+            htri_t ret = H5I__clear_type_cb((void *)item, NULL, (void *)&udata);
+            if (FAIL == ret)
+                HGOTO_ERROR(H5E_ATOM, H5E_BADITER, FAIL, "iteration failed")
+            if (TRUE == ret)
+                HASH_DELETE(hh, udata.type_ptr->ids.hash_table, item);
+        }
+    }
+    else
+        /* Attempt to free all ids in the type */
+        if (H5SL_try_free_safe(udata.type_ptr->ids.skip_list, H5I__clear_type_cb, &udata) < 0)
         HGOTO_ERROR(H5E_ATOM, H5E_CANTDELETE, FAIL, "can't free ids in type")
 
 done:
@@ -702,9 +742,15 @@ H5I__destroy_type(H5I_type_t type)
         if (type_ptr->cls->flags & H5I_CLASS_IS_APPLICATION)
             type_ptr->cls = H5FL_FREE(H5I_class_t, (void *)type_ptr->cls);
 
-    if (H5SL_close(type_ptr->ids) < 0)
-        HGOTO_ERROR(H5E_ATOM, H5E_CANTCLOSEOBJ, FAIL, "can't close skip list")
-    type_ptr->ids = NULL;
+    if (H5I_IS_LIB_TYPE(type)) {
+        HASH_CLEAR(hh, type_ptr->ids.hash_table);
+        type_ptr->ids.hash_table = NULL;
+    }
+    else {
+        if (H5SL_close(type_ptr->ids.skip_list) < 0)
+            HGOTO_ERROR(H5E_ATOM, H5E_CANTCLOSEOBJ, FAIL, "can't close skip list")
+        type_ptr->ids.skip_list = NULL;
+    }
 
     type_ptr                 = H5FL_FREE(H5I_id_type_t, type_ptr);
     H5I_id_type_list_g[type] = NULL;
@@ -783,7 +829,10 @@ H5I_register(H5I_type_t type, const void *object, hbool_t app_ref)
     id_ptr->obj_ptr   = object;
 
     /* Insert into the type */
-    if (H5SL_insert(type_ptr->ids, id_ptr, &id_ptr->id) < 0)
+    if (H5I_IS_LIB_TYPE(type))
+        /* Insert into the hash table */
+        HASH_ADD(hh, type_ptr->ids.hash_table, id, sizeof(hid_t), id_ptr);
+    else if (H5SL_insert(type_ptr->ids.skip_list, id_ptr, &id_ptr->id) < 0)
         HGOTO_ERROR(H5E_ATOM, H5E_CANTINSERT, H5I_INVALID_HID, "can't insert ID node into skip list")
     type_ptr->id_count++;
     type_ptr->nextid++;
@@ -860,7 +909,10 @@ H5I_register_using_existing_id(H5I_type_t type, void *object, hbool_t app_ref, h
     id_ptr->obj_ptr   = object;
 
     /* Insert into the type */
-    if (H5SL_insert(type_ptr->ids, id_ptr, &id_ptr->id) < 0)
+    if (H5I_IS_LIB_TYPE(type))
+        /* Insert into the hash table */
+        HASH_ADD(hh, type_ptr->ids.hash_table, id, sizeof(hid_t), id_ptr);
+    else if (H5SL_insert(type_ptr->ids.skip_list, id_ptr, &id_ptr->id) < 0)
         HGOTO_ERROR(H5E_ATOM, H5E_CANTINSERT, FAIL, "can't insert ID node into skip list")
     type_ptr->id_count++;
 
@@ -1201,7 +1253,7 @@ H5I__remove_verify(hid_t id, H5I_type_t id_type)
 static void *
 H5I__remove_common(H5I_id_type_t *type_ptr, hid_t id)
 {
-    H5I_id_info_t *curr_id;          /* Pointer to the current atom */
+    H5I_id_info_t *curr_id   = NULL; /* Pointer to the current atom */
     void *         ret_value = NULL; /* Return value */
 
     FUNC_ENTER_STATIC
@@ -1209,8 +1261,16 @@ H5I__remove_common(H5I_id_type_t *type_ptr, hid_t id)
     /* Sanity check */
     HDassert(type_ptr);
 
-    /* Get the ID node for the ID */
-    if (NULL == (curr_id = (H5I_id_info_t *)H5SL_remove(type_ptr->ids, &id)))
+    /* Delete the node */
+    if (H5I_IS_LIB_TYPE(type_ptr->cls->type_id)) {
+        /* Remove the node from the hash table */
+        HASH_FIND(hh, type_ptr->ids.hash_table, &id, sizeof(hid_t), curr_id);
+        if (curr_id)
+            HASH_DELETE(hh, type_ptr->ids.hash_table, curr_id);
+        else
+            HGOTO_ERROR(H5E_ATOM, H5E_CANTDELETE, NULL, "can't remove ID node from hash table")
+    }
+    else if (NULL == (curr_id = (H5I_id_info_t *)H5SL_remove(type_ptr->ids.skip_list, &id)))
         HGOTO_ERROR(H5E_ATOM, H5E_CANTDELETE, NULL, "can't remove ID node from skip list")
 
     /* Check if this ID was the last one accessed */
@@ -1915,7 +1975,7 @@ H5Isearch(H5I_type_t type, H5I_search_func_t func, void *key)
     /* Note that H5I_iterate returns an error code.  We ignore it
      * here, as we can't do anything with it without revising the API.
      */
-    (void)H5I_iterate(type, H5I__search_cb, &udata, TRUE);
+    H5I_iterate(type, H5I__search_cb, &udata, TRUE);
 
     /* Set return value */
     ret_value = udata.ret_obj;
@@ -2107,7 +2167,20 @@ H5I_iterate(H5I_type_t type, H5I_search_func_t func, void *udata, hbool_t app_re
         iter_udata.obj_type   = type;
 
         /* Iterate over IDs */
-        if ((iter_status = H5SL_iterate(type_ptr->ids, H5I__iterate_cb, &iter_udata)) < 0)
+        if (H5I_IS_LIB_TYPE(type)) {
+            H5I_id_info_t *item = NULL;
+            H5I_id_info_t *tmp  = NULL;
+
+            HASH_ITER(hh, type_ptr->ids.hash_table, item, tmp)
+            {
+                int ret = H5I__iterate_cb((void *)item, NULL, (void *)&iter_udata);
+                if (H5_ITER_ERROR == ret)
+                    HGOTO_ERROR(H5E_ATOM, H5E_BADITER, FAIL, "iteration failed")
+                if (H5_ITER_STOP == ret)
+                    break;
+            }
+        }
+        else if ((iter_status = H5SL_iterate(type_ptr->ids.skip_list, H5I__iterate_cb, &iter_udata)) < 0)
             HGOTO_ERROR(H5E_ATOM, H5E_BADITER, FAIL, "iteration failed")
     } /* end if */
 
@@ -2149,7 +2222,10 @@ H5I__find_id(hid_t id)
         ret_value = type_ptr->last_info;
     else {
         /* Locate the ID node for the ID */
-        ret_value = (H5I_id_info_t *)H5SL_search(type_ptr->ids, &id);
+        if (H5I_IS_LIB_TYPE(H5I_TYPE(id)))
+            HASH_FIND(hh, type_ptr->ids.hash_table, &id, sizeof(hid_t), ret_value);
+        else
+            ret_value = (H5I_id_info_t *)H5SL_search(type_ptr->ids.skip_list, &id);
 
         /* Remember this ID */
         type_ptr->last_info = ret_value;
@@ -2329,7 +2405,20 @@ H5I_find_id(const void *object, H5I_type_t type, hid_t *id)
         udata.ret_id   = H5I_INVALID_HID;
 
         /* Iterate over IDs for the ID type */
-        if ((iter_status = H5SL_iterate(type_ptr->ids, H5I__find_id_cb, &udata)) < 0)
+        if (H5I_IS_LIB_TYPE(type)) {
+            H5I_id_info_t *item = NULL;
+            H5I_id_info_t *tmp  = NULL;
+
+            HASH_ITER(hh, type_ptr->ids.hash_table, item, tmp)
+            {
+                int ret = H5I__find_id_cb((void *)item, NULL, (void *)&udata);
+                if (H5_ITER_ERROR == ret)
+                    HGOTO_ERROR(H5E_ATOM, H5E_BADITER, FAIL, "iteration failed")
+                if (H5_ITER_STOP == ret)
+                    break;
+            }
+        }
+        else if ((iter_status = H5SL_iterate(type_ptr->ids.skip_list, H5I__find_id_cb, &udata)) < 0)
             HGOTO_ERROR(H5E_ATOM, H5E_BADITER, FAIL, "iteration failed")
 
         *id = udata.ret_id;
@@ -2453,7 +2542,27 @@ H5I_dump_ids_for_type(H5I_type_t type)
         /* List */
         if (type_ptr->id_count > 0) {
             HDfprintf(stderr, "     List:\n");
-            H5SL_iterate(type_ptr->ids, H5I__id_dump_cb, &type);
+
+            if (H5I_IS_LIB_TYPE(type)) {
+                H5I_id_info_t *item = NULL;
+                H5I_id_info_t *tmp  = NULL;
+
+                /* Normally we care about the callback's return value
+                 * (H5I_ITER_CONT, etc.), but this is an iteration over all
+                 * the IDs so we don't care.
+                 *
+                 * XXX: Update this to emit an error message on errors?
+                 */
+                HDfprintf(stderr, "     (HASH TABLE)\n");
+                HASH_ITER(hh, type_ptr->ids.hash_table, item, tmp)
+                {
+                    H5I__id_dump_cb((void *)item, NULL, (void *)&type);
+                }
+            }
+            else {
+                HDfprintf(stderr, "     (SKIP LIST)\n");
+                H5SL_iterate(type_ptr->ids.skip_list, H5I__id_dump_cb, &type);
+            }
         }
     }
     else

test/tid.c

@@ -763,6 +763,79 @@ out:
     return -1;
 } /* end test_remove_clear_type() */
 
+
+/* Test that IDs can be deleted while iterating */
+
+#define N_ITERATE_IDS   128
+
+/* A callback that (maybe) closes a random ID when invoked */
+static herr_t
+iterate_delete_op(hid_t H5_ATTR_UNUSED id, void *udata)
+{
+    hid_t           *ids = (hid_t *)udata;
+    int             closed_index;
+    hid_t           closed_id;
+
+    /* Maybe close an ID
+     *
+     * Do nothing when an ID has already been closed so we don't
+     * close ALL the IDs.
+     *
+     * Closing the ID we're currently iterating on is also acceptable.
+     */
+    closed_index = HDrandom() % N_ITERATE_IDS;
+    closed_id = ids[closed_index];
+
+    if (closed_id != H5I_INVALID_HID) {
+        if (H5Sclose(closed_id) < 0)
+            return -1;
+        ids[closed_index] = H5I_INVALID_HID;
+    }
+
+    return 0;
+} /* end iterate_delete_op() */
+
+static int
+test_iteration_remove(void)
+{
+    hid_t           *ids = NULL;
+    int             i;
+
+    /* Create a bunch of IDs of a single library type */
+    if (NULL == (ids = malloc(N_ITERATE_IDS * sizeof(hid_t))))
+        goto error;
+    for (i = 0; i < N_ITERATE_IDS; i++)
+        if ((ids[i] = H5Screate(H5S_NULL)) == H5I_INVALID_HID)
+            goto error;
+
+    /* Iterate over the IDs, (maybe) closing a random one in the callback */
+    if (H5Iiterate(H5I_DATASPACE, iterate_delete_op, ids) < 0)
+        goto error;
+
+    /* Close and free */
+    for (i = 0; i < N_ITERATE_IDS; i++) {
+        if (ids[i] == H5I_INVALID_HID)
+            continue;
+        if (H5Sclose(ids[i]) < 0)
+            goto error;
+        ids[i] = H5I_INVALID_HID;
+    }
+    HDfree(ids);
+
+    return 0;
+
+error:
+    H5E_BEGIN_TRY
+        if (ids != NULL)
+            for (i = 0; i < N_ITERATE_IDS; i++)
+                H5Sclose(ids[i]);
+    H5E_END_TRY
+
+    HDfree(ids);
+
+    return -1;
+} /* end test_iteration_remove() */
+
 void
 test_ids(void)
 {
@@ -781,4 +854,6 @@ test_ids(void)
         TestErrPrintf("ID type list test failed\n");
     if (test_remove_clear_type() < 0)
         TestErrPrintf("ID remove during H5Iclear_type test failed\n");
+    if (test_iteration_remove() < 0)
+        TestErrPrintf("Removing random IDs while iterating failed\n");
 }