Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

H5Dchunk_iter now passes offsets in units of dataset elements, fix #1419 #1969

Merged
merged 2 commits into from
Aug 7, 2022
Merged

H5Dchunk_iter now passes offsets in units of dataset elements, fix #1419 #1969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5af3dd5
H5Dchunk_iter now passes chunk dimension scaled offsets, fix #1419
mkitti Aug 5, 2022
52381d0
Update docs for H5Dchunk_iter, H5Dget_chunk_info*
mkitti Aug 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/H5Dchunk.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,6 +254,7 @@ typedef struct H5D_chunk_coll_fill_info_t {
typedef struct H5D_chunk_iter_ud_t {
H5D_chunk_iter_op_t op; /* User defined callback */
void *op_data; /* User data for user defined callback */
H5O_layout_chunk_t *chunk; /* Chunk layout */
} H5D_chunk_iter_ud_t;

/********************/
Expand Down Expand Up @@ -8066,13 +8067,20 @@ static int
H5D__chunk_iter_cb(const H5D_chunk_rec_t *chunk_rec, void *udata)
{
const H5D_chunk_iter_ud_t *data = (H5D_chunk_iter_ud_t *)udata;
const H5O_layout_chunk_t *chunk = data->chunk;
int ret_value = H5_ITER_CONT;
hsize_t offset[H5O_LAYOUT_NDIMS];
unsigned ii; /* Match H5O_layout_chunk_t.ndims */

/* Similar to H5D__get_chunk_info */
for (ii = 0; ii < chunk->ndims; ii++)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is off by one? The ndims here is 1 larger than the rank I set. I also see in other places, layout->ndims is decreased by 1 to determine the number of dimensions.

hdf5/src/H5Dearray.c

Lines 1095 to 1100 in 509fe96

unsigned ndims = (idx_info->layout->ndims - 1); /* Number of dimensions */
unsigned u;
/* Compute coordinate offset from scaled offset */
for (u = 0; u < ndims; u++)
swizzled_coords[u] = udata->common.scaled[u] * idx_info->layout->dim[u];

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you add an assertion here and run this test, it's easy to see that chunk->ndims is 3 but we clearly expect the offset array to have two valid elements.

Copy link
Contributor Author

@mkitti mkitti Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct that there are ndims - 1 dimensions, but this is an internal implementation detail of the library. That specific loop comes from H5D__get_chunk_info.

What is the actual problem though? At worse, we do one more multiplication than needed while avoiding a subtraction and another variable on the stack.

offset[ii] = chunk_rec->scaled[ii] * chunk->dim[ii];

FUNC_ENTER_PACKAGE_NOERR

/* Check for callback failure and pass along return value */
if ((ret_value = (data->op)(chunk_rec->scaled, chunk_rec->filter_mask, chunk_rec->chunk_addr,
chunk_rec->nbytes, data->op_data)) < 0)
if ((ret_value = (data->op)(offset, chunk_rec->filter_mask, chunk_rec->chunk_addr, chunk_rec->nbytes,
data->op_data)) < 0)
HERROR(H5E_DATASET, H5E_CANTNEXT, "iteration operator failed");

FUNC_LEAVE_NOAPI(ret_value)
Expand Down Expand Up @@ -8132,6 +8140,7 @@ H5D__chunk_iter(H5D_t *dset, H5D_chunk_iter_op_t op, void *op_data)
/* Set up info for iteration callback */
ud.op = op;
ud.op_data = op_data;
ud.chunk = &dset->shared->layout.u.chunk;

/* Iterate over the allocated chunks calling the iterator callback */
if ((ret_value = (layout->storage.u.chunk.ops->iterate)(&idx_info, H5D__chunk_iter_cb, &ud)) < 0)
Expand Down
24 changes: 12 additions & 12 deletions src/H5Dpublic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,10 @@ typedef herr_t (*H5D_gather_func_t)(const void *dst_buf, size_t dst_buf_bytes_us
/**
* \brief Callback for H5Dchunk_iter()
*
* \param[in] offset Array of starting logical coordinates of chunk.
* \param[in] filter_mask Filter mask of chunk.
* \param[in] addr Offset in file of chunk data.
* \param[in] nbytes Size in bytes of chunk data in file.
* \param[in] offset Logical position of the chunk’s first element in units of dataset elements
* \param[in] filter_mask Bitmask indicating the filters used when the chunk was written
* \param[in] addr Chunk address in the file
* \param[in] size Chunk size in bytes, 0 if the chunk does not exist
* \param[in,out] op_data Pointer to any user-defined data associated with
* the operation.
* \returns \li Zero (#H5_ITER_CONT) causes the iterator to continue, returning
Expand All @@ -239,7 +239,7 @@ typedef herr_t (*H5D_gather_func_t)(const void *dst_buf, size_t dst_buf_bytes_us
* \li A negative (#H5_ITER_ERROR) causes the iterator to immediately
* return that value, indicating failure.
*/
typedef int (*H5D_chunk_iter_op_t)(const hsize_t *offset, uint32_t filter_mask, haddr_t addr, uint32_t nbytes,
typedef int (*H5D_chunk_iter_op_t)(const hsize_t *offset, uint32_t filter_mask, haddr_t addr, uint32_t size,
void *op_data);
//! <!-- [H5D_chunk_iter_op_t_snip] -->

Expand Down Expand Up @@ -634,10 +634,10 @@ H5_DLL herr_t H5Dget_num_chunks(hid_t dset_id, hid_t fspace_id, hsize_t *nchunks
* \brief Retrieves information about a chunk specified by its coordinates
*
* \dset_id
* \param[in] offset Logical position of the chunk’s first element
* \param[out] filter_mask Indicating filters used with the chunk when written
* \param[in] offset Logical position of the chunk’s first element in units of dataset elements
* \param[out] filter_mask Bitmask indicating the filters used when the chunk was written
* \param[out] addr Chunk address in the file
* \param[out] size Chunk size in bytes, 0 if chunk doesn’t exist
* \param[out] size Chunk size in bytes, 0 if the chunk does not exist
*
* \return \herr_t
*
Expand Down Expand Up @@ -672,7 +672,7 @@ H5_DLL herr_t H5Dget_chunk_info_by_coord(hid_t dset_id, const hsize_t *offset, u
*
* \return \herr_t
*
* \details H5Dget_chunk_iter iterates over all chunks in the dataset, calling the
* \details H5Dchunk_iter iterates over all chunks in the dataset, calling the
* user supplied callback with the details of the chunk and the supplied
* context \p op_data.
*
Expand All @@ -696,10 +696,10 @@ H5_DLL herr_t H5Dchunk_iter(hid_t dset_id, hid_t dxpl_id, H5D_chunk_iter_op_t cb
* \dset_id
* \param[in] fspace_id File dataspace selection identifier (See Note below)
* \param[in] chk_idx Index of the chunk
* \param[out] offset Logical position of the chunk’s first element
* \param[out] filter_mask Indicating filters used with the chunk when written
* \param[out] offset Logical position of the chunk’s first element in units of dataset elements
* \param[out] filter_mask Bitmask indicating the filters used when the chunk was written
* \param[out] addr Chunk address in the file
* \param[out] size Chunk size in bytes, 0 if chunk doesn’t exist
* \param[out] size Chunk size in bytes, 0 if the chunk does not exist
*
* \return \herr_t
*
Expand Down
4 changes: 2 additions & 2 deletions test/chunk_info.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1722,9 +1722,9 @@ test_basic_query(hid_t fapl)
if (chunk_infos[0].nbytes != 96)
FAIL_PUTS_ERROR("size mismatch");

if (chunk_infos[1].offset[0] != 1)
if (chunk_infos[1].offset[0] != CHUNK_NX)
FAIL_PUTS_ERROR("offset[0] mismatch");
if (chunk_infos[1].offset[1] != 1)
if (chunk_infos[1].offset[1] != CHUNK_NY)
FAIL_PUTS_ERROR("offset[1] mismatch");

/* Iterate and stop after one iteration */
Expand Down