type conversions moved outside of HDF5 to improve performance - #284
Open
reguly wants to merge 1 commit into
Open
type conversions moved outside of HDF5 to improve performance#284reguly wants to merge 1 commit into
reguly wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new conversion helpers and diagnostics introduce confirmed undefined-behavior/overflow risks (printf format mismatch and unchecked size multiplications) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR introduces explicit, pre-/post-HDF5 datatype conversion helpers so that parallel HDF5 can keep using collective MPI-IO (avoiding the documented fallback to independent I/O when HDF5 must do implicit conversion internally).
Changes:
- Added
H5Dwrite_matching_types/H5Dread_matching_typeswrappers to convert buffers outsideH5Dwrite/H5Dreadwhen dataset and memory types differ. - Updated MPI and non-MPI HDF5 I/O paths to use the new wrappers and pass explicit element counts (
nelem). - Simplified/centralized real-precision storage selection via
h5_storage_type, and added optional MPI-IO mode diagnostics in the MPI writer.
File summaries
| File | Description |
|---|---|
| ops/c/src/mpi/ops_mpi_hdf5.cpp | Switches MPI HDF5 reads/writes to matching-type wrappers, adds MPI-IO mode reporting. |
| ops/c/src/externlib/ops_hdf5.cpp | Switches non-MPI HDF5 reads/writes to matching-type wrappers and passes element counts. |
| ops/c/src/externlib/ops_hdf5_common.cpp | Adds type conversion helpers and factors out storage-type selection for precision control. |
| ops/c/include/ops_hdf5_common.h | Exposes new helpers (h5_storage_type, matching-type read/write wrappers). |
Review details
Suppressed comments (1)
ops/c/src/externlib/ops_hdf5_common.cpp:272
nbytesfor the temporary read buffer is computed asnelem * max(src_sz, dst_sz)without an overflow check. For very large selections this can wrap and allocate too small a buffer, risking memory corruption inH5Dread/H5Tconvert. Guard the multiplication before allocating.
size_t src_sz = H5Tget_size(dset_type);
size_t dst_sz = H5Tget_size(mem_type);
size_t nbytes = nelem * (src_sz > dst_sz ? src_sz : dst_sz);
if (nbytes == 0)
nbytes = 1;
char *tmp = (char *)malloc(nbytes);
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+209
to
+213
| if (equal == 0 && nelem > 0 && buf != nullptr) { | ||
| size_t src_sz = H5Tget_size(mem_type); | ||
| size_t dst_sz = H5Tget_size(dset_type); | ||
| size_t nbytes = nelem * (src_sz > dst_sz ? src_sz : dst_sz); | ||
| converted = (char *)malloc(nbytes); |
Comment on lines
+77
to
+80
| ops_printf("HDF5 %s actual_io_mode=%d (4=contiguous collective, 0=none) " | ||
| "no_collective_cause local=0x%x global=0x%x " | ||
| "(0x2=DATATYPE_CONVERSION) get_status=%d/%d\n", | ||
| tag, (int)mode, local_cause, global_cause, (int)e1, (int)e2); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug reported by the OpenSBLI team - turns out when implicit type conversion was needed, HDF5 serialized and didn't use MPI I/O