Remove cached datatype conversion path table entries on file close (#3942)

Remove cached datatype conversion path table entries on file close

When performing datatype conversions during I/O, the library
checks to see whether it can re-use a cached datatype conversion
pathway by performing comparisons between the source and destination
datatypes of the current operation and the source and destination
datatypes associated with each cached datatype conversion pathway.
For variable-length and reference datatypes, a comparison is made
between the VOL object for the file associated with these datatypes,
which may change as a file is closed and reopened. In workflows
involving a loop that opens a file, performs I/O on an object with a
variable-length or reference datatype and then closes the file, this
can lead to constant memory usage growth as the library compares the
file VOL objects between the datatypes as different and adds a new
cached conversion pathway entry on each iteration during I/O. This is
now fixed by clearing out any cached conversion pathway entries for
variable-length or reference datatypes associated with a particular
file when that file is closed.

4 files changed, 117 insertions, 12 deletions

diff --git a/src/H5Fint.c b/src/H5Fint.c
index 8738026..1feada6 100644
--- a/src/H5Fint.c
+++ b/src/H5Fint.c
@@ -1615,6 +1615,18 @@ H5F__dest(H5F_t *f, bool flush, bool free_on_failure)
         if (vol_wrap_ctx && (NULL == H5VL_object_unwrap(f->vol_obj)))
             HDONE_ERROR(H5E_FILE, H5E_CANTGET, FAIL, "can't unwrap VOL object");
 
+        /*
+         * Clean up any cached type conversion path table entries that
+         * may have been keeping a reference to the file's VOL object
+         * in order to prevent the file from being closed out from
+         * underneath other places that may access the conversion path
+         * or its src/dst datatypes later on (currently, conversions on
+         * variable-length and reference datatypes involve this)
+         */
+        if (H5T_unregister(H5T_PERS_SOFT, NULL, NULL, NULL, f->vol_obj, NULL) < 0)
+            HDONE_ERROR(H5E_FILE, H5E_CANTRELEASE, FAIL,
+                        "unable to free cached type conversion path table entries");
+
         if (H5VL_free_object(f->vol_obj) < 0)
             HDONE_ERROR(H5E_FILE, H5E_CANTDEC, FAIL, "unable to free VOL object");
         f->vol_obj = NULL;
diff --git a/src/H5T.c b/src/H5T.c
index 4a5c7cf..d665566 100644
--- a/src/H5T.c
+++ b/src/H5T.c
@@ -343,12 +343,13 @@ typedef H5T_t *(*H5T_copy_func_t)(H5T_t *old_dt);
 static herr_t H5T__register_int(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst,
                                 H5T_lib_conv_t func);
 static herr_t H5T__register(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5T_conv_func_t *conv);
-static herr_t H5T__unregister(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5T_conv_t func);
 static htri_t H5T__compiler_conv(H5T_t *src, H5T_t *dst);
 static herr_t H5T__set_size(H5T_t *dt, size_t size);
 static herr_t H5T__close_cb(H5T_t *dt, void **request);
 static H5T_path_t *H5T__path_find_real(const H5T_t *src, const H5T_t *dst, const char *name,
                                        H5T_conv_func_t *conv);
+static bool        H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst,
+                                  H5VL_object_t *owned_vol_obj, H5T_conv_t func);
 static bool        H5T__detect_vlen_ref(const H5T_t *dt);
 static H5T_t      *H5T__initiate_copy(const H5T_t *old_dt);
 static H5T_t      *H5T__copy_transient(H5T_t *old_dt);
@@ -2671,7 +2672,7 @@ done:
 } /* end H5Tregister() */
 
 /*-------------------------------------------------------------------------
- * Function:   H5T__unregister
+ * Function:   H5T_unregister
  *
  * Purpose:    Removes conversion paths that match the specified criteria.
  *        All arguments are optional. Missing arguments are wild cards.
@@ -2682,18 +2683,33 @@ done:
  *
  *-------------------------------------------------------------------------
  */
-static herr_t
-H5T__unregister(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5T_conv_t func)
+herr_t
+H5T_unregister(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5VL_object_t *owned_vol_obj,
+               H5T_conv_t func)
 {
     H5T_path_t *path   = NULL; /*conversion path             */
     H5T_soft_t *soft   = NULL; /*soft conversion information */
     int         nprint = 0;    /*number of paths shut down   */
     int         i;             /*counter                     */
 
-    FUNC_ENTER_PACKAGE_NOERR
+    FUNC_ENTER_NOAPI_NOERR
 
-    /* Remove matching entries from the soft list */
-    if (H5T_PERS_DONTCARE == pers || H5T_PERS_SOFT == pers) {
+    /*
+     * Remove matching entries from the soft list if:
+     *
+     *  - The caller didn't specify a particular type (soft or hard)
+     *    of conversion path to match against or specified that soft
+     *    conversion paths should be matched against
+     *
+     *  AND
+     *
+     *  - The caller didn't provide the `owned_vol_obj` parameter;
+     *    if this parameter is provided, we want to leave the soft
+     *    list untouched and only remove cached conversion paths
+     *    below where the file VOL object associated with the path's
+     *    source or destination types matches the given VOL object.
+     */
+    if ((H5T_PERS_DONTCARE == pers || H5T_PERS_SOFT == pers) && !owned_vol_obj) {
         for (i = H5T_g.nsoft - 1; i >= 0; --i) {
             soft = H5T_g.soft + i;
             assert(soft);
@@ -2713,13 +2729,15 @@ H5T__unregister(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5T_c
 
     /* Remove matching conversion paths, except no-op path */
     for (i = H5T_g.npaths - 1; i > 0; --i) {
+        bool nomatch;
+
         path = H5T_g.path[i];
         assert(path);
 
+        nomatch = !H5T_path_match(path, pers, name, src, dst, owned_vol_obj, func);
+
         /* Not a match */
-        if (((H5T_PERS_SOFT == pers && path->is_hard) || (H5T_PERS_HARD == pers && !path->is_hard)) ||
-            (name && *name && strcmp(name, path->name) != 0) || (src && H5T_cmp(src, path->src, false)) ||
-            (dst && H5T_cmp(dst, path->dst, false)) || (func && func != path->conv.u.app_func)) {
+        if (nomatch) {
             /*
              * Notify all other functions to recalculate private data since some
              * functions might cache a list of conversion functions.  For
@@ -2768,7 +2786,7 @@ H5T__unregister(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst, H5T_c
     }                              /* end for */
 
     FUNC_LEAVE_NOAPI(SUCCEED)
-} /* end H5T__unregister() */
+} /* end H5T_unregister() */
 
 /*-------------------------------------------------------------------------
  * Function:  H5Tunregister
@@ -2798,7 +2816,7 @@ H5Tunregister(H5T_pers_t pers, const char *name, hid_t src_id, hid_t dst_id, H5T
     if (dst_id > 0 && (NULL == (dst = (H5T_t *)H5I_object_verify(dst_id, H5I_DATATYPE))))
         HGOTO_ERROR(H5E_ARGS, H5E_BADTYPE, FAIL, "dst is not a data type");
 
-    if (H5T__unregister(pers, name, src, dst, func) < 0)
+    if (H5T_unregister(pers, name, src, dst, NULL, func) < 0)
         HGOTO_ERROR(H5E_DATATYPE, H5E_CANTDELETE, FAIL, "internal unregister function failed");
 
 done:
@@ -5149,6 +5167,53 @@ done:
 } /* end H5T__path_find_real() */
 
 /*-------------------------------------------------------------------------
+ * Function:  H5T_path_match
+ *
+ * Purpose:   Helper function to determine whether a datatype conversion
+ *            path object matches against a given set of criteria.
+ *
+ * Return:    true/false (can't fail)
+ *
+ *-------------------------------------------------------------------------
+ */
+static bool
+H5T_path_match(H5T_path_t *path, H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst,
+               H5VL_object_t *owned_vol_obj, H5T_conv_t func)
+{
+    bool ret_value = true;
+
+    assert(path);
+
+    FUNC_ENTER_NOAPI_NOINIT_NOERR
+
+    if (
+        /* Check that the specified conversion function persistence matches */
+        ((H5T_PERS_SOFT == pers && path->is_hard) || (H5T_PERS_HARD == pers && !path->is_hard)) ||
+
+        /* Check that the specified conversion path name matches */
+        (name && *name && strcmp(name, path->name) != 0) ||
+
+        /*
+         * Check that the specified source and destination datatypes match
+         * the source and destination datatypes in the conversion path
+         */
+        (src && H5T_cmp(src, path->src, false)) || (dst && H5T_cmp(dst, path->dst, false)) ||
+
+        /*
+         * Check that the specified VOL object matches the VOL object
+         * in the conversion path
+         */
+        (owned_vol_obj && (owned_vol_obj != path->src->shared->owned_vol_obj) &&
+         (owned_vol_obj != path->dst->shared->owned_vol_obj)) ||
+
+        /* Check that the specified conversion function matches */
+        (func && func != path->conv.u.app_func))
+        ret_value = false;
+
+    FUNC_LEAVE_NOAPI(ret_value)
+} /* H5T_path_match() */
+
+/*-------------------------------------------------------------------------
  * Function:  H5T_path_noop
  *
  * Purpose:   Is the path the special no-op path? The no-op function can be
@@ -6095,3 +6160,26 @@ H5T_own_vol_obj(H5T_t *dt, H5VL_object_t *vol_obj)
 done:
     FUNC_LEAVE_NOAPI(ret_value)
 } /* end H5T_own_vol_obj() */
+
+/*-------------------------------------------------------------------------
+ * Function:    H5T__get_path_table_npaths
+ *
+ * Purpose:     Testing function to return the number of type conversion
+ *              paths currently stored in the type conversion path table
+ *              cache.
+ *
+ * Return:      Number of type conversion paths (can't fail)
+ *
+ *-------------------------------------------------------------------------
+ */
+int
+H5T__get_path_table_npaths(void)
+{
+    int ret_value = 0;
+
+    FUNC_ENTER_PACKAGE_NOERR
+
+    ret_value = H5T_g.npaths;
+
+    FUNC_LEAVE_NOAPI(ret_value)
+}
diff --git a/src/H5Tpkg.h b/src/H5Tpkg.h
index b9e24be..ef5ba36 100644
--- a/src/H5Tpkg.h
+++ b/src/H5Tpkg.h
@@ -877,4 +877,7 @@ H5_DLL herr_t H5T__sort_name(const H5T_t *dt, int *map);
 /* Debugging functions */
 H5_DLL herr_t H5T__print_stats(H5T_path_t *path, int *nprint /*in,out*/);
 
+/* Testing functions */
+H5_DLL int H5T__get_path_table_npaths(void);
+
 #endif /* H5Tpkg_H */
diff --git a/src/H5Tprivate.h b/src/H5Tprivate.h
index 0332679..3120053 100644
--- a/src/H5Tprivate.h
+++ b/src/H5Tprivate.h
@@ -133,6 +133,8 @@ H5_DLL H5T_path_t        *H5T_path_find(const H5T_t *src, const H5T_t *dst);
 H5_DLL bool               H5T_path_noop(const H5T_path_t *p);
 H5_DLL H5T_bkg_t          H5T_path_bkg(const H5T_path_t *p);
 H5_DLL H5T_subset_info_t *H5T_path_compound_subset(const H5T_path_t *p);
+H5_DLL herr_t             H5T_unregister(H5T_pers_t pers, const char *name, H5T_t *src, H5T_t *dst,
+                                         H5VL_object_t *owned_vol_obj, H5T_conv_t func);
 H5_DLL herr_t H5T_convert(H5T_path_t *tpath, hid_t src_id, hid_t dst_id, size_t nelmts, size_t buf_stride,
                           size_t bkg_stride, void *buf, void *bkg);
 H5_DLL herr_t H5T_reclaim(hid_t type_id, struct H5S_t *space, void *buf);