Skip to content

Commit

Permalink
Refactor datatype conversion code to use pointers rather than IDs (#4104
Browse files Browse the repository at this point in the history 
)

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
  • Loading branch information
@jhendersonHDF
jhendersonHDF authored Mar 10, 2024
1 parent 28aaeb9 commit ef401a5
Show file tree
Hide file tree
Showing 26 changed files with 2,110 additions and 1,711 deletions.
120 changes: 51 additions & 69 deletions src/H5Aint.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -677,15 +677,16 @@ H5A__open_by_name(const H5G_loc_t *loc, const char *obj_name, const char *attr_n
herr_t
H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
{
uint8_t *tconv_buf = NULL; /* datatype conv buffer*/
uint8_t *bkg_buf = NULL; /* background buffer */
hssize_t snelmts; /* elements in attribute */
size_t nelmts; /* elements in attribute*/
H5T_path_t *tpath = NULL; /* type conversion info */
hid_t src_id = -1, dst_id = -1; /* temporary type IDs*/
size_t src_type_size; /* size of source type */
size_t dst_type_size; /* size of destination type */
size_t buf_size; /* desired buffer size */
uint8_t *tconv_buf = NULL; /* datatype conv buffer*/
uint8_t *bkg_buf = NULL; /* background buffer */
hssize_t snelmts; /* elements in attribute */
size_t nelmts; /* elements in attribute*/
H5T_path_t *tpath = NULL; /* type conversion info */
H5T_t *src_type = NULL; /* temporary datatype */
H5T_t *dst_type = NULL; /* temporary datatype */
size_t src_type_size; /* size of source type */
size_t dst_type_size; /* size of destination type */
size_t buf_size; /* desired buffer size */
herr_t ret_value = SUCCEED;

FUNC_ENTER_PACKAGE_TAG(attr->oloc.addr)
Expand Down Expand Up @@ -722,10 +723,10 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
if (!H5T_path_noop(tpath)) {
H5T_bkg_t need_bkg; /* Background buffer type */

if ((src_id = H5I_register(H5I_DATATYPE, H5T_copy(attr->shared->dt, H5T_COPY_ALL), false)) <
0 ||
(dst_id = H5I_register(H5I_DATATYPE, H5T_copy(mem_type, H5T_COPY_ALL), false)) < 0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTREGISTER, FAIL, "unable to register types for conversion");
if (NULL == (src_type = H5T_copy(attr->shared->dt, H5T_COPY_ALL)))
HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy attribute datatype");
if (NULL == (dst_type = H5T_copy(mem_type, H5T_COPY_ALL)))
HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy memory datatype");

/* Get the maximum buffer size needed and allocate it */
buf_size = nelmts * MAX(src_type_size, dst_type_size);
Expand All @@ -751,8 +752,9 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
}

/* Perform datatype conversion. */
if (H5T_convert(tpath, src_id, dst_id, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) < 0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTENCODE, FAIL, "datatype conversion failed");
if (H5T_convert(tpath, src_type, dst_type, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) <
0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTCONVERT, FAIL, "datatype conversion failed");

/* Copy the converted data into the user's buffer */
H5MM_memcpy(buf, tconv_buf, (dst_type_size * nelmts));
Expand All @@ -769,10 +771,10 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)

done:
/* Release resources */
if (src_id >= 0 && H5I_dec_ref(src_id) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
if (dst_id >= 0 && H5I_dec_ref(dst_id) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
if (src_type && H5T_close(src_type) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
if (dst_type && H5T_close(dst_type) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
if (tconv_buf)
tconv_buf = H5FL_BLK_FREE(attr_buf, tconv_buf);
if (bkg_buf)
Expand All @@ -799,15 +801,16 @@ H5A__read(const H5A_t *attr, const H5T_t *mem_type, void *buf)
herr_t
H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
{
uint8_t *tconv_buf = NULL; /* datatype conv buffer */
uint8_t *bkg_buf = NULL; /* temp conversion buffer */
hssize_t snelmts; /* elements in attribute */
size_t nelmts; /* elements in attribute */
H5T_path_t *tpath = NULL; /* conversion information*/
hid_t src_id = -1, dst_id = -1; /* temporary type IDs */
size_t src_type_size; /* size of source type */
size_t dst_type_size; /* size of destination type*/
size_t buf_size; /* desired buffer size */
uint8_t *tconv_buf = NULL; /* datatype conv buffer */
uint8_t *bkg_buf = NULL; /* temp conversion buffer */
hssize_t snelmts; /* elements in attribute */
size_t nelmts; /* elements in attribute */
H5T_path_t *tpath = NULL; /* conversion information*/
H5T_t *src_type = NULL; /* temporary datatype */
H5T_t *dst_type = NULL; /* temporary datatype */
size_t src_type_size; /* size of source type */
size_t dst_type_size; /* size of destination type*/
size_t buf_size; /* desired buffer size */
herr_t ret_value = SUCCEED;

FUNC_ENTER_PACKAGE_TAG(attr->oloc.addr)
Expand Down Expand Up @@ -840,9 +843,10 @@ H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
if (!H5T_path_noop(tpath)) {
H5T_bkg_t need_bkg; /* Background buffer type */

if ((src_id = H5I_register(H5I_DATATYPE, H5T_copy(mem_type, H5T_COPY_ALL), false)) < 0 ||
(dst_id = H5I_register(H5I_DATATYPE, H5T_copy(attr->shared->dt, H5T_COPY_ALL), false)) < 0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTREGISTER, FAIL, "unable to register types for conversion");
if (NULL == (src_type = H5T_copy(mem_type, H5T_COPY_ALL)))
HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy memory datatype");
if (NULL == (dst_type = H5T_copy(attr->shared->dt, H5T_COPY_ALL)))
HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy attribute datatype");

/* Get the maximum buffer size needed and allocate it */
buf_size = nelmts * MAX(src_type_size, dst_type_size);
Expand Down Expand Up @@ -876,8 +880,8 @@ H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)
}

/* Perform datatype conversion */
if (H5T_convert(tpath, src_id, dst_id, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) < 0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTENCODE, FAIL, "datatype conversion failed");
if (H5T_convert(tpath, src_type, dst_type, nelmts, (size_t)0, (size_t)0, tconv_buf, bkg_buf) < 0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTCONVERT, FAIL, "datatype conversion failed");

/* Free the previous attribute data buffer, if there is one */
if (attr->shared->data)
Expand Down Expand Up @@ -907,10 +911,10 @@ H5A__write(H5A_t *attr, const H5T_t *mem_type, const void *buf)

done:
/* Release resources */
if (src_id >= 0 && H5I_dec_ref(src_id) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
if (dst_id >= 0 && H5I_dec_ref(dst_id) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTDEC, FAIL, "unable to close temporary object");
if (src_type && H5T_close(src_type) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
if (dst_type && H5T_close(dst_type) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, FAIL, "unable to close temporary datatype");
if (tconv_buf)
tconv_buf = H5FL_BLK_FREE(attr_buf, tconv_buf);
if (bkg_buf)
Expand Down Expand Up @@ -2079,9 +2083,7 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
H5O_copy_t H5_ATTR_NDEBUG_UNUSED *cpy_info)
{
H5A_t *attr_dst = NULL; /* Destination attribute */
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 */
void *buf = NULL; /* Buffer for copying data */
void *reclaim_buf = NULL; /* Buffer for reclaiming data */
void *bkg_buf = NULL; /* Background buffer */
Expand Down Expand Up @@ -2195,7 +2197,6 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
/* Check if we need to convert data */
if (H5T_detect_class(attr_src->shared->dt, H5T_VLEN, false) > 0) {
H5T_path_t *tpath_src_mem, *tpath_mem_dst; /* Datatype conversion paths */
H5T_t *dt_mem; /* Memory datatype */
size_t src_dt_size; /* Source datatype size */
size_t tmp_dt_size; /* Temp. datatype size */
size_t max_dt_size; /* Max atatype size */
Expand All @@ -2204,20 +2205,9 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
size_t nelmts; /* Number of elements in buffer */
size_t buf_size; /* Size of copy buffer */

/* Create datatype ID for src datatype */
if ((tid_src = H5I_register(H5I_DATATYPE, attr_src->shared->dt, false)) < 0)
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, NULL, "unable to register source file datatype");

/* create a memory copy of the variable-length datatype */
if (NULL == (dt_mem = H5T_copy(attr_src->shared->dt, H5T_COPY_TRANSIENT)))
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "unable to copy");
if ((tid_mem = H5I_register(H5I_DATATYPE, dt_mem, false)) < 0)
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, NULL, "unable to register memory datatype");

/* create variable-length datatype at the destination file */
if ((tid_dst = H5I_register(H5I_DATATYPE, attr_dst->shared->dt, false)) < 0)
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTREGISTER, NULL,
"unable to register destination file datatype");

/* Set up the conversion functions */
if (NULL == (tpath_src_mem = H5T_path_find(attr_src->shared->dt, dt_mem)))
Expand Down Expand Up @@ -2273,7 +2263,8 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
HGOTO_ERROR(H5E_ATTR, H5E_CANTALLOC, NULL, "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_buf) < 0)
if (H5T_convert(tpath_src_mem, attr_src->shared->dt, dt_mem, nelmts, (size_t)0, (size_t)0, buf,
bkg_buf) < 0)
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "datatype conversion NULLed");

H5MM_memcpy(reclaim_buf, buf, buf_size);
Expand All @@ -2283,12 +2274,13 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size
memset(bkg_buf, 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_buf) < 0)
if (H5T_convert(tpath_mem_dst, dt_mem, attr_dst->shared->dt, nelmts, (size_t)0, (size_t)0, buf,
bkg_buf) < 0)
HGOTO_ERROR(H5E_DATATYPE, H5E_CANTINIT, NULL, "datatype conversion NULLed");

H5MM_memcpy(attr_dst->shared->data, buf, attr_dst->shared->data_size);

if (H5T_reclaim(tid_mem, buf_space, reclaim_buf) < 0)
if (H5T_reclaim(dt_mem, buf_space, reclaim_buf) < 0)
HGOTO_ERROR(H5E_DATASET, H5E_BADITER, NULL, "unable to reclaim variable-length data");
} /* end if */
else {
Expand All @@ -2313,19 +2305,9 @@ H5A__attr_copy_file(const H5A_t *attr_src, H5F_t *file_dst, bool *recompute_size

done:
if (buf_sid > 0 && H5I_dec_ref(buf_sid) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary dataspace ID");
if (tid_src > 0)
/* Don't decrement ID, we want to keep underlying datatype */
if (NULL == H5I_remove(tid_src))
HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary datatype ID");
if (tid_dst > 0)
/* Don't decrement ID, we want to keep underlying datatype */
if (NULL == H5I_remove(tid_dst))
HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary datatype ID");
if (tid_mem > 0)
/* Decrement the memory datatype ID, it's transient */
if (H5I_dec_ref(tid_mem) < 0)
HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "Can't decrement temporary datatype ID");
HDONE_ERROR(H5E_ATTR, H5E_CANTFREE, NULL, "can't decrement temporary dataspace ID");
if (dt_mem && (H5T_close(dt_mem) < 0))
HDONE_ERROR(H5E_ATTR, H5E_CANTCLOSEOBJ, NULL, "can't close temporary datatype");
if (buf)
buf = H5FL_BLK_FREE(attr_buf, buf);
if (reclaim_buf)
Expand Down Expand Up @@ -2416,7 +2398,7 @@ H5A__attr_post_copy_file(const H5O_loc_t *src_oloc, const H5A_t *attr_src, H5O_l
/* Check for expanding references */
if (cpy_info->expand_ref) {
/* Copy objects referenced in source buffer to destination file and set destination elements */
if (H5O_copy_expand_ref(file_src, H5I_INVALID_HID, attr_src->shared->dt, attr_src->shared->data,
if (H5O_copy_expand_ref(file_src, attr_src->shared->dt, attr_src->shared->data,
attr_src->shared->data_size, file_dst, attr_dst->shared->data,
cpy_info) < 0)
HGOTO_ERROR(H5E_ATTR, H5E_CANTCOPY, FAIL, "unable to copy reference attribute");
Expand Down
Loading

0 comments on commit ef401a5

Please sign in to comment.