Refactor datatype conversion code to use pointers rather than IDs (#4104)

The datatype conversion code previously used IDs for the source and
destination datatypes rather than pointers to the internal structures
for those datatypes. This was mostly due to the need for an ID for these
datatypes that can be passed to an application-registered datatype
conversion function or datatype conversion exception function. However,
using IDs internally caused a lot of unnecessary ID lookups and hurt
performance of datatype conversions in general. This was especially
problematic for compound datatype conversions, where the ID lookups were
occuring on every member of every compound element of a dataset. The
code has now been refactored to use pointers internally and only create
IDs for datatypes when necessary.

Fixed a test issue in dt_arith where a library datatype conversion
function was being cast to an application conversion function. Since the
two have different prototypes, this started failing after the parameters
for a library conversion function changed from hid_t to H5T_t * and an
extra parameter was added. This appears to have worked coincidentally in
the past since the only different between a library conversion function
and application conversion function was an extra DXPL parameter at the
end of an application conversion function

Fixed an issue where memory wasn't being freed in the h5fc_chk_idx test
program. Even though the program exits quickly after allocating the
memory, it still causes failures when testing with -fsanitize=address

1 files changed, 17 insertions, 31 deletions

diff --git a/src/H5Dcompact.c b/src/H5Dcompact.c
index 4a8fa0d..66acfd5 100644
--- a/src/H5Dcompact.c
+++ b/src/H5Dcompact.c
@@ -127,7 +127,7 @@ H5D__compact_fill(const H5D_t *dset)
     /* Initialize the fill value buffer */
     /* (use the compact dataset storage buffer as the fill value buffer) */
     if (H5D__fill_init(&fb_info, dset->shared->layout.storage.u.compact.buf, NULL, NULL, NULL, NULL,
-                       &dset->shared->dcpl_cache.fill, dset->shared->type, dset->shared->type_id, (size_t)0,
+                       &dset->shared->dcpl_cache.fill, dset->shared->type, (size_t)0,
                        dset->shared->layout.storage.u.compact.size) < 0)
         HGOTO_ERROR(H5E_DATASET, H5E_CANTINIT, FAIL, "can't initialize fill buffer info");
     fb_info_init = true;
@@ -470,9 +470,8 @@ herr_t
 H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_dst,
                   H5O_storage_compact_t *storage_dst, H5T_t *dt_src, H5O_copy_t *cpy_info)
 {
-    hid_t         tid_src     = -1;   /* Datatype ID for source datatype */
-    hid_t         tid_dst     = -1;   /* Datatype ID for destination datatype */
-    hid_t         tid_mem     = -1;   /* Datatype ID for memory datatype */
+    H5T_t        *dt_mem      = NULL; /* Memory datatype */
+    H5T_t        *dt_dst      = NULL; /* Destination datatype */
     void         *buf         = NULL; /* Buffer for copying data */
     void         *bkg         = NULL; /* Temporary buffer for copying data */
     void         *reclaim_buf = NULL; /* Buffer for reclaiming data */
@@ -496,15 +495,9 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
     if (shared_fo != NULL)
         storage_src = &(shared_fo->layout.storage.u.compact);
 
-    /* Create datatype ID for src datatype, so it gets freed */
-    if ((tid_src = H5I_register(H5I_DATATYPE, dt_src, false)) < 0)
-        HGOTO_ERROR(H5E_DATASET, H5E_CANTREGISTER, FAIL, "unable to register source file datatype");
-
     /* If there's a VLEN source datatype, do type conversion information */
     if (H5T_detect_class(dt_src, H5T_VLEN, false) > 0) {
         H5T_path_t *tpath_src_mem, *tpath_mem_dst; /* Datatype conversion paths */
-        H5T_t      *dt_dst;                        /* Destination datatype */
-        H5T_t      *dt_mem;                        /* Memory datatype */
         H5S_t      *buf_space;                     /* Dataspace describing buffer */
         size_t      buf_size;                      /* Size of copy buffer */
         size_t      nelmts;                        /* Number of elements in buffer */
@@ -516,10 +509,6 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
         /* create a memory copy of the variable-length datatype */
         if (NULL == (dt_mem = H5T_copy(dt_src, H5T_COPY_TRANSIENT)))
             HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "unable to copy");
-        if ((tid_mem = H5I_register(H5I_DATATYPE, dt_mem, false)) < 0) {
-            (void)H5T_close_real(dt_mem);
-            HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL, "unable to register memory datatype");
-        } /* end if */
 
         /* create variable-length datatype at the destination file */
         if (NULL == (dt_dst = H5T_copy(dt_src, H5T_COPY_TRANSIENT)))
@@ -528,10 +517,6 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
             (void)H5T_close_real(dt_dst);
             HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "cannot mark datatype on disk");
         } /* end if */
-        if ((tid_dst = H5I_register(H5I_DATATYPE, dt_dst, false)) < 0) {
-            (void)H5T_close_real(dt_dst);
-            HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, FAIL, "unable to register destination file datatype");
-        } /* end if */
 
         /* Set up the conversion functions */
         if (NULL == (tpath_src_mem = H5T_path_find(dt_src, dt_mem)))
@@ -584,8 +569,8 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
             HGOTO_ERROR(H5E_RESOURCE, H5E_NOSPACE, FAIL, "memory allocation failed");
 
         /* Convert from source file to memory */
-        if (H5T_convert(tpath_src_mem, tid_src, tid_mem, nelmts, (size_t)0, (size_t)0, buf, bkg) < 0)
-            HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "datatype conversion failed");
+        if (H5T_convert(tpath_src_mem, dt_src, dt_mem, nelmts, (size_t)0, (size_t)0, buf, bkg) < 0)
+            HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "datatype conversion failed");
 
         /* Copy into another buffer, to reclaim memory later */
         H5MM_memcpy(reclaim_buf, buf, buf_size);
@@ -594,13 +579,13 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
         memset(bkg, 0, buf_size);
 
         /* Convert from memory to destination file */
-        if (H5T_convert(tpath_mem_dst, tid_mem, tid_dst, nelmts, (size_t)0, (size_t)0, buf, bkg) < 0)
-            HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, FAIL, "datatype conversion failed");
+        if (H5T_convert(tpath_mem_dst, dt_mem, dt_dst, nelmts, (size_t)0, (size_t)0, buf, bkg) < 0)
+            HGOTO_ERROR(H5E_DATATYPE, H5E_CANTCONVERT, FAIL, "datatype conversion failed");
 
         H5MM_memcpy(storage_dst->buf, buf, storage_dst->size);
 
-        if (H5T_reclaim(tid_mem, buf_space, reclaim_buf) < 0)
-            HGOTO_ERROR(H5E_DATASET, H5E_BADITER, FAIL, "unable to reclaim variable-length data");
+        if (H5T_reclaim(dt_mem, buf_space, reclaim_buf) < 0)
+            HGOTO_ERROR(H5E_DATASET, H5E_CANTFREE, FAIL, "unable to reclaim variable-length data");
     } /* end if */
     else if (H5T_get_class(dt_src, false) == H5T_REFERENCE) {
         if (f_src != f_dst) {
@@ -608,7 +593,7 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
             if (cpy_info->expand_ref) {
                 /* Copy objects referenced in source buffer to destination file and set destination elements
                  */
-                if (H5O_copy_expand_ref(f_src, tid_src, dt_src, storage_src->buf, storage_src->size, f_dst,
+                if (H5O_copy_expand_ref(f_src, dt_src, storage_src->buf, storage_src->size, f_dst,
                                         storage_dst->buf, cpy_info) < 0)
                     HGOTO_ERROR(H5E_DATASET, H5E_CANTCOPY, FAIL, "unable to copy reference attribute");
             } /* end if */
@@ -630,12 +615,13 @@ H5D__compact_copy(H5F_t *f_src, H5O_storage_compact_t *_storage_src, H5F_t *f_ds
 done:
     if (buf_sid > 0 && H5I_dec_ref(buf_sid) < 0)
         HDONE_ERROR(H5E_DATASET, H5E_CANTFREE, FAIL, "can't decrement temporary dataspace ID");
-    if (tid_src > 0 && H5I_dec_ref(tid_src) < 0)
-        HDONE_ERROR(H5E_DATASET, H5E_CANTFREE, FAIL, "Can't decrement temporary datatype ID");
-    if (tid_dst > 0 && H5I_dec_ref(tid_dst) < 0)
-        HDONE_ERROR(H5E_DATASET, H5E_CANTFREE, FAIL, "Can't decrement temporary datatype ID");
-    if (tid_mem > 0 && H5I_dec_ref(tid_mem) < 0)
-        HDONE_ERROR(H5E_DATASET, H5E_CANTFREE, FAIL, "Can't decrement temporary datatype ID");
+    /* Caller expects that source datatype will be freed */
+    if (dt_src && (H5T_close(dt_src) < 0))
+        HDONE_ERROR(H5E_DATASET, H5E_CANTCLOSEOBJ, FAIL, "can't close temporary datatype");
+    if (dt_dst && (H5T_close(dt_dst) < 0))
+        HDONE_ERROR(H5E_DATASET, H5E_CANTCLOSEOBJ, FAIL, "can't close temporary datatype");
+    if (dt_mem && (H5T_close(dt_mem) < 0))
+        HDONE_ERROR(H5E_DATASET, H5E_CANTCLOSEOBJ, FAIL, "can't close temporary datatype");
     if (buf)
         buf = H5FL_BLK_FREE(type_conv, buf);
     if (reclaim_buf)