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

Jump to bottom

rest_vol_dataset: (fix) Write hyperslab (memory) #50

Closed
wants to merge 1 commit into from
Closed

rest_vol_dataset: (fix) Write hyperslab (memory) #50

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 9 additions & 0 deletions src/rest_vol_dataset.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -829,6 +829,13 @@ RV_dataset_write(size_t count, void *dset[], hid_t mem_type_id[], hid_t mem_spac
FUNC_GOTO_ERROR(H5E_DATATYPE, H5E_BADVALUE, FAIL, "memory datatype is invalid");

write_body_len = (size_t)file_select_npoints * dtype_size;
if (NULL == (write_body = (char *)RV_malloc(write_body_len)))
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably worth only allocating this buffer and calling H5Dgather if the memory space wasn't set to H5S_ALL and has a non-contiguous selection in it. Otherwise, the H5Dgather call could potentially add overhead for no reason. Checking for H5S_ALL should be straightforward, but detecting a non-contiguous selection may take a little effort in the way of H5S calls.

I also noticed that the code below for point selections allocates the same write_body buffer. If I'm not mistaken, it looks like writing with a point selection could cause the memory allocated here to be leaked.

Copy link
Collaborator

@mattjala mattjala Aug 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Looks like point selection does cause a memory leak. From test_rest_vol:

Testing small write to dataset w/ point selection                         
==309263== Invalid read of size 2
==309263==    at 0x48529E0: memmove (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==309263==    by 0x497F43D: H5D__gather_mem (H5Dscatgath.c:417)
==309263==    by 0x496BB55: H5Dgather (H5D.c:1756)
==309263==    by 0x48F44E2: RV_dataset_write (rest_vol_dataset.c:835)
==309263==    by 0x4B1E977: H5VL__dataset_write (H5VLcallback.c:2236)
==309263==    by 0x4B24335: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309263==    by 0x496767B: H5D__write_api_common (H5D.c:1331)
==309263==    by 0x496A71F: H5Dwrite (H5D.c:1388)
==309263==    by 0x132BFE: test_write_dataset_small_point_selection (test_rest_vol.c:7535)
==309263==    by 0x10F869: main (test_rest_vol.c:18250)
==309263==  Address 0x777fcf0 is 32 bytes before a block of size 48 in arena "client"
==309263== 
==309263== Invalid read of size 1
==309263==    at 0x48528F1: memmove (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==309263==    by 0x497F43D: H5D__gather_mem (H5Dscatgath.c:417)
==309263==    by 0x496BB55: H5Dgather (H5D.c:1756)
==309263==    by 0x48F44E2: RV_dataset_write (rest_vol_dataset.c:835)
==309263==    by 0x4B1E977: H5VL__dataset_write (H5VLcallback.c:2236)
==309263==    by 0x4B24335: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309263==    by 0x496767B: H5D__write_api_common (H5D.c:1331)
==309263==    by 0x496A71F: H5Dwrite (H5D.c:1388)
==309263==    by 0x132BFE: test_write_dataset_small_point_selection (test_rest_vol.c:7535)
==309263==    by 0x10F869: main (test_rest_vol.c:18250)
==309263==  Address 0x777feac is 28 bytes after a block of size 48 in arena "client"
==309263== 
==309263== Use of uninitialised value of size 8
==309263==    at 0x48C8218: RV_base64_encode (rest_vol.c:3100)
==309263==    by 0x48F4A30: RV_dataset_write (rest_vol_dataset.c:900)
==309263==    by 0x4B1E977: H5VL__dataset_write (H5VLcallback.c:2236)
==309263==    by 0x4B24335: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309263==    by 0x496767B: H5D__write_api_common (H5D.c:1331)
==309263==    by 0x496A71F: H5Dwrite (H5D.c:1388)
==309263==    by 0x132BFE: test_write_dataset_small_point_selection (test_rest_vol.c:7535)
==309263==    by 0x10F869: main (test_rest_vol.c:18250)
==309263== 
==309263== Use of uninitialised value of size 8
==309263==    at 0x48C8298: RV_base64_encode (rest_vol.c:3105)
==309263==    by 0x48F4A30: RV_dataset_write (rest_vol_dataset.c:900)
==309263==    by 0x4B1E977: H5VL__dataset_write (H5VLcallback.c:2236)
==309263==    by 0x4B24335: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309263==    by 0x496767B: H5D__write_api_common (H5D.c:1331)
==309263==    by 0x496A71F: H5Dwrite (H5D.c:1388)
==309263==    by 0x132BFE: test_write_dataset_small_point_selection (test_rest_vol.c:7535)
==309263==    by 0x10F869: main (test_rest_vol.c:18250)
==309263== 
==309263== Use of uninitialised value of size 8
==309263==    at 0x48C82DB: RV_base64_encode (rest_vol.c:3111)
==309263==    by 0x48F4A30: RV_dataset_write (rest_vol_dataset.c:900)
==309263==    by 0x4B1E977: H5VL__dataset_write (H5VLcallback.c:2236)
==309263==    by 0x4B24335: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309263==    by 0x496767B: H5D__write_api_common (H5D.c:1331)
==309263==    by 0x496A71F: H5Dwrite (H5D.c:1388)
==309263==    by 0x132BFE: test_write_dataset_small_point_selection (test_rest_vol.c:7535)
==309263==    by 0x10F869: main (test_rest_vol.c:18250)
==309263== 
==309263== Use of uninitialised value of size 8
==309263==    at 0x48C8204: RV_base64_encode (rest_vol.c:3099)
==309263==    by 0x48F4A30: RV_dataset_write (rest_vol_dataset.c:900)
==309263==    by 0x4B1E977: H5VL__dataset_write (H5VLcallback.c:2236)
==309263==    by 0x4B24335: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309263==    by 0x496767B: H5D__write_api_common (H5D.c:1331)
==309263==    by 0x496A71F: H5Dwrite (H5D.c:1388)
==309263==    by 0x132BFE: test_write_dataset_small_point_selection (test_rest_vol.c:7535)
==309263==    by 0x10F869: main (test_rest_vol.c:18250)
==309263==

And the vol-tests:

==309052== 40 bytes in 1 blocks are definitely lost in loss record 2 of 7
==309052==    at 0x4848899: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==309052==    by 0x541B4B2: ???
==309052==    by 0x4B2EBA7: H5VL__dataset_write (H5VLcallback.c:2236)
==309052==    by 0x4B34565: H5VL_dataset_write_direct (H5VLcallback.c:2280)
==309052==    by 0x4906D3B: H5D__write_api_common (H5D.c:1331)
==309052==    by 0x4909DDF: H5Dwrite (H5D.c:1388)
==309052==    by 0x149AF1: test_write_dataset_small_point_selection (vol_dataset_test.c:5758)
==309052==    by 0x161B98: vol_dataset_test (vol_dataset_test.c:11257)
==309052==    by 0x10FC1E: vol_test_run (vol_test.c:118)
==309052==    by 0x10FC1E: main (vol_test.c:279)

Copy link
Contributor Author

@jwsblokland jwsblokland Aug 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@jhendersonHDF and @mattjala, thanks for the review and reminding me that I can use valgrind.

The mentioned memory leak of write_body has been address and also the strange behavior of test_write_dataset_small_point_selection test. For the test, it turns out I forgot the modify the H5Dwrite() call by including a memory dataspace.

The only thing left to implement is checking for H5S_ALL and non-contiguous selection. Currently, if H5S_ALL == mem_space_id and H5S_ALL != mem_space_id the selection of the file_space_id will be copied to mem_space_id. Is this the intended behavior. I assume it but I ask to make sure. This is not what I would naively expect. With this behavior, I think, you implicitly assume that the memory dataspace is exactly the same as the file dataspace.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you made a typo - did you mean to ask about the case where H5S_ALL == mem_space_id and H5S_ALL != file_space_id? If so, I believe that copying the selection from file space is the intended behavior of the library - see the second row of the mem_space_id/file_space_id table in the documentation for H5Dwrite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, you are right. Thanks for pointing me to the documentation of H5Dwrite().

FUNC_GOTO_ERROR(H5E_DATASPACE, H5E_CANTALLOC, FAIL,
"can't allocate space for the 'write_body' values");
if (H5Dgather(mem_space_id[0], buf[0], mem_type_id[0], write_body_len, write_body, NULL, write_body) <
0)
FUNC_GOTO_ERROR(H5E_DATASET, H5E_WRITEERROR, FAIL, "can't gather data to write buffer");
buf[0] = write_body;
} /* end if */
else {
if (H5T_STD_REF_OBJ == mem_type_id[0]) {
Expand Down Expand Up @@ -897,6 +904,8 @@ RV_dataset_write(size_t count, void *dset[], hid_t mem_type_id[], hid_t mem_spac
printf("-> Base64-encoded data buffer: %s\n\n", base64_encoded_value);
#endif

if (write_body)
RV_free(write_body);
write_body_len = (strlen(fmt_string) - 4) + selection_body_len + value_body_len;
if (NULL == (write_body = RV_malloc(write_body_len + 1)))
FUNC_GOTO_ERROR(H5E_DATASET, H5E_CANTALLOC, FAIL, "can't allocate space for write buffer");
Expand Down
39 changes: 35 additions & 4 deletions test/test_rest_vol.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7464,11 +7464,13 @@ test_write_dataset_small_point_selection(void)
hsize_t points[DATASET_SMALL_WRITE_TEST_POINT_SELECTION_NUM_POINTS *
DATASET_SMALL_WRITE_TEST_POINT_SELECTION_DSET_SPACE_RANK];
hsize_t dims[DATASET_SMALL_WRITE_TEST_POINT_SELECTION_DSET_SPACE_RANK] = {10, 10, 10};
hsize_t mdims[1];
size_t i, data_size;
hid_t file_id = -1, fapl_id = -1;
hid_t container_group = -1;
hid_t dset_id = -1;
hid_t fspace_id = -1;
hid_t mspace_id = -1;
void *data = NULL;

TESTING("small write to dataset w/ point selection")
Expand Down Expand Up @@ -7511,6 +7513,9 @@ test_write_dataset_small_point_selection(void)
if (NULL == (data = malloc(data_size)))
TEST_ERROR

mdims[0] = DATASET_SMALL_WRITE_TEST_POINT_SELECTION_NUM_POINTS;
if ((mspace_id = H5Screate_simple(1, mdims, NULL)) < 0)
TEST_ERROR
for (i = 0; i < data_size / DATASET_SMALL_WRITE_TEST_POINT_SELECTION_DSET_DTYPESIZE; i++)
((int *)data)[i] = (int)i;

Expand All @@ -7532,7 +7537,7 @@ test_write_dataset_small_point_selection(void)
puts("Writing a small amount of data to dataset using a point selection\n");
#endif

if (H5Dwrite(dset_id, DATASET_SMALL_WRITE_TEST_POINT_SELECTION_DSET_DTYPE, H5S_ALL, fspace_id,
if (H5Dwrite(dset_id, DATASET_SMALL_WRITE_TEST_POINT_SELECTION_DSET_DTYPE, mspace_id, fspace_id,
H5P_DEFAULT, data) < 0) {
H5_FAILED();
printf(" couldn't write to dataset\n");
Expand All @@ -7544,6 +7549,8 @@ test_write_dataset_small_point_selection(void)
data = NULL;
}

if (H5Sclose(mspace_id) < 0)
TEST_ERROR
if (H5Sclose(fspace_id) < 0)
TEST_ERROR
if (H5Dclose(dset_id) < 0)
Expand Down Expand Up @@ -8539,6 +8546,7 @@ test_write_dataset_data_verification(void)
{
hssize_t space_npoints;
hsize_t dims[DATASET_DATA_VERIFY_WRITE_TEST_DSET_SPACE_RANK] = {10, 10, 10};
hsize_t mdims[1];
hsize_t start[DATASET_DATA_VERIFY_WRITE_TEST_DSET_SPACE_RANK];
hsize_t stride[DATASET_DATA_VERIFY_WRITE_TEST_DSET_SPACE_RANK];
hsize_t count[DATASET_DATA_VERIFY_WRITE_TEST_DSET_SPACE_RANK];
Expand All @@ -8550,6 +8558,7 @@ test_write_dataset_data_verification(void)
hid_t container_group = -1;
hid_t dset_id = -1;
hid_t fspace_id = -1;
hid_t mspace_id = -1;
void *data = NULL;
void *write_buf = NULL;
void *read_buf = NULL;
Expand Down Expand Up @@ -8697,23 +8706,34 @@ test_write_dataset_data_verification(void)
}

/* Write to first two rows of dataset */
mdims[0] = dims[1] * 2;
start[0] = 0;
stride[0] = 1;
count[0] = dims[1] * 2;
block[0] = 1;
if ((mspace_id = H5Screate_simple(1, mdims, NULL)) < 0)
TEST_ERROR
if (H5Sselect_hyperslab(mspace_id, H5S_SELECT_SET, start, stride, count, block) < 0)
TEST_ERROR

start[0] = start[1] = start[2] = 0;
stride[0] = stride[1] = stride[2] = 1;
count[0] = 2;
count[1] = dims[1];
count[2] = 1;
block[0] = block[1] = block[2] = 1;

if (H5Sselect_hyperslab(fspace_id, H5S_SELECT_SET, start, stride, count, block) < 0)
TEST_ERROR

if (H5Dwrite(dset_id, DATASET_DATA_VERIFY_WRITE_TEST_DSET_DTYPE, H5S_ALL, fspace_id, H5P_DEFAULT,
if (H5Dwrite(dset_id, DATASET_DATA_VERIFY_WRITE_TEST_DSET_DTYPE, mspace_id, fspace_id, H5P_DEFAULT,
write_buf) < 0) {
H5_FAILED();
printf(" couldn't write to dataset\n");
goto error;
}

if (H5Sclose(mspace_id) < 0)
TEST_ERROR
if (H5Sclose(fspace_id) < 0)
TEST_ERROR
if (H5Dclose(dset_id) < 0)
Expand Down Expand Up @@ -8814,6 +8834,15 @@ test_write_dataset_data_verification(void)
}

/* Select a series of 10 points in the dataset */
mdims[0] = DATASET_DATA_VERIFY_WRITE_TEST_NUM_POINTS;
if ((mspace_id = H5Screate_simple(1, mdims, NULL)) < 0)
TEST_ERROR
for (i = 0; i < DATASET_DATA_VERIFY_WRITE_TEST_NUM_POINTS; i++) {
points[i] = i;
}
if (H5Sselect_elements(mspace_id, H5S_SELECT_SET, DATASET_DATA_VERIFY_WRITE_TEST_NUM_POINTS, points) < 0)
TEST_ERROR

for (i = 0; i < DATASET_DATA_VERIFY_WRITE_TEST_NUM_POINTS; i++) {
size_t j;

Expand All @@ -8824,13 +8853,15 @@ test_write_dataset_data_verification(void)
if (H5Sselect_elements(fspace_id, H5S_SELECT_SET, DATASET_DATA_VERIFY_WRITE_TEST_NUM_POINTS, points) < 0)
TEST_ERROR

if (H5Dwrite(dset_id, DATASET_DATA_VERIFY_WRITE_TEST_DSET_DTYPE, H5S_ALL, fspace_id, H5P_DEFAULT,
if (H5Dwrite(dset_id, DATASET_DATA_VERIFY_WRITE_TEST_DSET_DTYPE, mspace_id, fspace_id, H5P_DEFAULT,
write_buf) < 0) {
H5_FAILED();
printf(" couldn't write to dataset\n");
goto error;
}

if (H5Sclose(mspace_id) < 0)
TEST_ERROR
if (H5Sclose(fspace_id) < 0)
TEST_ERROR
if (H5Dclose(dset_id) < 0)
Expand Down