Introduce more flexible storeChunk() syntax, use to add ADIOS2 memory selection - #1620
franzpoeschel wants to merge 74 commits into
Conversation
727cbbc to
ddcdfc9
Compare
c05d535 to
ba7c582
Compare
ba7c582 to
d7eb891
Compare
332932f to
efb0876
Compare
efb0876 to
39e3d71
Compare
47d497b to
b2bc355
Compare
b699520 to
3430b6d
Compare
9b5acff to
bd7b378
Compare
208c381 to
e51e635
Compare
c8be94b to
8e455b4
Compare
8e455b4 to
430fe3b
Compare
430fe3b to
e98c840
Compare
190674f to
e04f76d
Compare
b99bb95 to
d05e029
Compare
| [deleter = std::move(this->get_deleter()), original_ptr](other_type *) { | ||
| deleter(original_ptr); |
There was a problem hiding this comment.
Codex:
[P2] Apply the deleter to its argument after resetting a converted pointer
Capturing original_ptr makes the deleter ignore the pointer currently owned by the resulting UniquePtrWithLambda. This class publicly inherits std::unique_ptr, so auto q = std::move(p).static_cast_<void>(); q.reset(replacement); q.reset(); is valid usage: the first reset should delete the original allocation and the second should delete the replacement. A tracking-deleter reproducer instead receives the original address twice. With a normal deleting deleter this becomes a double free, while the replacement leaks. Please retain deletion of the supplied pointer, with the necessary type/const conversion, instead of binding deletion permanently to the original address.
There was a problem hiding this comment.
The only safe thing to do here is to detect this condition and throw (the alternative would be to assume that the new pointer has the same provenance and can hence be reinterpret_casted back to the original type for applying the original deleter…).
Note that .swap() remains safe as that also swaps deleters.
| template <typename T> | ||
| void loadChunk(std::shared_ptr<T> data, Offset offset, Extent extent); |
There was a problem hiding this comment.
Claude:
Source-compatibility regression: the std::shared_ptr<T[]> overloads of loadChunk() / storeChunk() were removed. Calls without explicit template arguments still work (T deduces to double[]), but calls with explicit template arguments no longer compile:
std::shared_ptr<double[]> p(new double[10]);
rc.loadChunk<double>(p, {0}, {10}); // dev: OK, PR: no matching function
rc.storeChunk<double>(p, {0}, {10}); // dev: OK, PR: no matching function(Verified with GCC 13 against the dev and PR headers.) Suggest keeping them as thin, header-only forwarding overloads. No extra explicit instantiations are needed, partial ordering still selects them for the implicit call, and inside the forwarder loadChunk<T[]> SFINAE-drops the T[][] candidate. With this and the storeChunk counterpart below, explicit, implicit and const[] calls compile and link again:
| template <typename T> | |
| void loadChunk(std::shared_ptr<T> data, Offset offset, Extent extent); | |
| template <typename T> | |
| void loadChunk(std::shared_ptr<T> data, Offset offset, Extent extent); | |
| /** Load a chunk of data into pre-allocated memory, array version. | |
| * | |
| * Kept for source compatibility with explicit template arguments, | |
| * e.g. loadChunk<double>(std::shared_ptr<double[]>, ...). | |
| */ | |
| template <typename T> | |
| void loadChunk(std::shared_ptr<T[]> data, Offset offset, Extent extent) | |
| { | |
| loadChunk<T[]>(std::move(data), std::move(offset), std::move(extent)); | |
| } |
There was a problem hiding this comment.
Will not fix. These overloads were only needed because the array types required special handling previously, and don't need special handling any longer with this PR. Assume the following snippet:
std::shared_ptr<double[]> p(new double[10]);
rc.loadChunk(p, {0}, {10});We previously needed a special overload to handle this, i.e. this would resolve to:
std::shared_ptr<double[]> p(new double[10]);
rc.loadChunk<double>(p, {0}, {10});This is no longer needed, the base template works for this use case:
std::shared_ptr<double[]> p(new double[10]);
rc.loadChunk<double[]>(p, {0}, {10}); // note the different type inside <>Claude recognized this changed template resolution as an API break (which it technically is, if specifying template arguments explicitly, the old usage will fail.) Template arguments never needed to be explicitly specified here.
| template <typename T> | ||
| void storeChunk(std::shared_ptr<T> data, Offset offset, Extent extent); |
There was a problem hiding this comment.
Claude:
Same source-compatibility regression for storeChunk<double>(std::shared_ptr<double[]>, ...) (see the loadChunk comment above):
| template <typename T> | |
| void storeChunk(std::shared_ptr<T> data, Offset offset, Extent extent); | |
| template <typename T> | |
| void storeChunk(std::shared_ptr<T> data, Offset offset, Extent extent); | |
| /** Store a chunk of data from a chunk of memory, array version. | |
| * | |
| * Kept for source compatibility with explicit template arguments, | |
| * e.g. storeChunk<double>(std::shared_ptr<double[]>, ...). | |
| */ | |
| template <typename T> | |
| void storeChunk(std::shared_ptr<T[]> data, Offset offset, Extent extent) | |
| { | |
| storeChunk<T[]>(std::move(data), std::move(offset), std::move(extent)); | |
| } |
The 'Remember buffer sizes' change added ConfigureLoadStoreFromBuffer:: bufferSize() and ConfigureStoreChunkFromBuffer::bufferSize(), as well as buffer-size handling in withContiguousContainer(). Add Doxygen for these and update the IOTask enum literalinclude range in the design notes to cover the new INCREASE_FLUSH_COUNTER operation.
A pending deferred load must not be treated as complete when an unrelated backend operation (here a rank-table query) flushes the I/O handler. Before the per-record-component flush counter, such a flush advanced the handler-wide counter, the deferred load's own flush was skipped, and the buffer kept its initial contents. Covers both pre-allocated (withRawPtr) and allocating loads.
loadChunkRaw() passed its offset/extent directly to the new builder, so the
sentinel values offset {0u} and extent {-1u} were treated as literal
selections. On a 2D dataset the documented call
rc.loadChunkRaw(buffer, {0u}, {-1u}) then threw a dimensionality mismatch,
and on a 1D dataset the sentinel extent was taken as an oversized chunk.
Forward through the shared-pointer loadChunk() overload, which performs the
required expansion, preserving the existing API contract.
The size of a contiguous container buffer is now checked against the final operation extent (known only at store()/load() time) to prevent out-of-bounds reads and writes: - N-D datasets (or an explicit extent) with a too-small container throw error::WrongAPIUsage instead of reading/writing past the buffer end. - A 1-D default (full) extent is downsized to the buffer size. - An exactly-sized container loads without error. Before the buffer-size was remembered, the N-D default extent fell back to the full dataset extent, causing a heap buffer over-read/over-write.
7700227 to
0e49f62
Compare
| { | ||
| std::string const nd_name = "../samples/issue3_nd_buffer_size.json"; | ||
|
|
||
| void write_nd_dataset() |
Check notice
Code scanning / CodeQL
Unused static function Note test
| } | ||
| } // namespace | ||
|
|
||
| TEST_CASE("container_buffer_size_too_small_throws", "[serial][json]") |
Check notice
Code scanning / CodeQL
Unused static function Note test
| s.flush(); | ||
| } | ||
|
|
||
| TEST_CASE("container_buffer_size_1d_downsizes", "[serial][json]") |
Check notice
Code scanning / CodeQL
Unused static function Note test
| REQUIRE(buf == std::vector<int>(4, 7)); | ||
| } | ||
|
|
||
| TEST_CASE("container_buffer_size_exact_loads", "[serial][json]") |
Check notice
Code scanning / CodeQL
Unused static function Note test
|
|
||
| using namespace openPMD; | ||
|
|
||
| TEST_CASE("deferred_load_unrelated_flush", "[serial][json]") |
Check notice
Code scanning / CodeQL
Unused static function Note test
The "clean up joined dim logic" change centralized the joined-dimension
offset rule in computeOffset() (empty offset for joined dims, throw on a
non-empty one) but only updated the container/allocating overloads. The
buffer-based overloads were left inconsistent:
- loadChunk(shared_ptr) still forwarded a {0} offset (stale `dim <= 1u`
condition), which computeOffset() then rejected for a joined dimension.
- loadChunk_impl() lacked the joined-dimension branch that verifyChunk()
(the store path) has, so it rejected the empty offset that joined
dimensions require with a dimensionality error. A joined dimension
therefore could not be loaded through the buffer-based load overloads
at all.
- The buffer-based storeChunk overloads and the span-based allocating
storeChunk forwarded a {0} offset / {-1} extent unconditionally, so a
joined-dimension store with the default {0} offset threw.
Make all buffer-based and span-based loadChunk/storeChunk overloads
detect the default {0} offset and {-1} extent (leaving joined-dimension
handling to computeOffset/computeExtent), and add the joined-dimension
branch to loadChunk_impl() mirroring verifyChunk().
Add a regression test (joined_dim_buffer_api) validating the frontend
offset/extent handling for a joined dimension through the buffer-based
overloads on the JSON backend; the argument handling is backend agnostic
(a joined dimension is only readable back on ADIOS2).
The handles returned by RecordComponent::prepareLoadStore().store()/load() perform the data operation and, by default, an automatic Series flush. Since Series::flush() is MPI-collective, document this behavior in the MPI overview, the deferred data API contract and the API headers, including the fact that per-component flush counters may skip the flush, and that handles are [[nodiscard]] so the flush cannot happen unnoticed.
Split the joined-dimension buffer-API regression test out of SerialIOTest.cpp into test/Files_SerialIO/joined_dim_buffer_api.cpp and register it in CMakeLists.txt, matching the convention of the other per-issue SerialIO test files.
A joined dimension's extent is only known once all writers have flushed and
the data is read back (close + reopen). During the write session the extent
is Dataset::JOINED_DIMENSION (max), so loading a chunk of a joined array is
not a meaningful operation: the total size is unknown and the streaming
engine cannot read the data back mid-write. After a reopen the extent is
concrete, joinedDimension() is nullopt, and the load goes through the normal
non-joined path (the case exercised by the joined_dim test).
The previous change made loadChunk_impl mirror the store path by accepting an
empty offset for joined arrays, which only validated the in-session case and
then queued a READ_DATASET for 2^64 elements. Reject joined arrays in
loadChunk_impl with a clear error instead; the store path (append) is
unchanged.
Update the joined_dim_buffer_api regression test: store with an empty {} and
a {0} offset still passes the frontend checks, while every buffer-based load
overload now throws error::WrongAPIUsage.
for more information, see https://pre-commit.ci
Co-authored-by: Axel Huebl <[email protected]>
| std::static_pointer_cast<T>(std::move(ptr)), | ||
| std::move(offset), | ||
| std::move(extent)); | ||
| // static_assert(!std::is_same_v<T_with_extent, std::string>, "EVIL"); |
Check notice
Code scanning / CodeQL
Commented-out code Note
|
|
||
| using namespace openPMD; | ||
|
|
||
| TEST_CASE("joined_dim_buffer_api", "[serial][json]") |
Check notice
Code scanning / CodeQL
Unused static function Note test
Rejecting a memory selection inside the backend's writeDataset() throws
while an IO task is being flushed. AbstractIOHandlerImpl::flush() reacts to
an exception in an IO task by clearing the whole IO queue and rethrowing
("Clearing IO queue and passing on the exception"), so every other pending
chunk and the file's root attributes are dropped. The subsequent flush()
and close() then report success, but the file can no longer be read back
(AttributeNotFound: openPMD) in HDF5 and JSON.
Whether a backend supports non-contiguous memory selections is known when
the chunk is enqueued, so check it in RecordComponent::storeChunk_impl()
and throw error::OperationUnsupportedInBackend there, before anything is
queued. Add AbstractIOHandler::supportsMemorySelection(), defaulting to
false and overridden to true only in ADIOS2IOHandler. The backend-side
throws stay as a safety net.
Add a regression test that stores a valid chunk, attempts a rejected
memory selection on another component, and verifies that the Series stays
usable and that the valid chunk and a root attribute survive the roundtrip
in HDF5 and JSON. Without the early check the test fails at flush time.
A memory selection could not be reset before ADIOS2 v2.10.1 (the reset capability was added upstream in v2.11.0 and backported to 2.10.1). On older versions a stale memory selection silently leaked into every following store operation of the same variable, producing wrong data or out-of-bounds reads. This affected later storeChunk() calls, later steps and the unique_ptr path. Instead of applying a workaround, report ADIOS2 as not supporting memory selections in that case via AbstractIOHandler::supportsMemorySelection(), so the frontend rejects them up front, and keep the backend-side check in verifyDataset() as a safety net. Both reuse the existing CanTheMemorySelectionBeReset trait. Remove the now-obsolete one-time warning and its bookkeeping. Adapt available_chunks_test and memory_selection_rejected_before_flush to use an equivalent contiguous buffer / expectation where memory selections are unsupported, and add a dedicated memory_selection_old_adios2 test. Pin the ADIOS2 v2.10 CI job to 2.10.0 to exercise the rejection path.
| return CanTheMemorySelectionBeReset; | ||
| } | ||
| #else | ||
| bool adios2SupportsMemorySelection() |
Check notice
Code scanning / CodeQL
Unused static function Note test
| * Writes a valid 1D chunk for component A and then attempts a store with a | ||
| * non-contiguous memory selection on component B. | ||
| */ | ||
| WriteResult write_and_reject(std::string const &name) |
Check notice
Code scanning / CodeQL
Unused static function Note test
| } | ||
| } // namespace | ||
|
|
||
| TEST_CASE("memory_selection_rejected_before_flush", "[serial]") |
Check notice
Code scanning / CodeQL
Unused static function Note test
We currently don't support memory selections yet, e.g. when you have a 2D memory buffer but want to write only an arbitrary block from it. That block is then non-contiguous, but ADIOS2 has so-called memory selections to support this.
We currently don't have an API that would be able to expose this (except in Python where the native Python buffer protocol is powerful enough to express this; we currently throw an error when detecting this
strides in selection are inefficient, not implemented!). Since theloadChunk()/storeChunk()API is somewhat convoluted by now anyway, I didn't want to add even further overloads.New API Design
The new API is centered around
RecordComponent::prepareLoadStore(), which returns a configuration object that can be chained with various methods before executing the actual load/store operation.Methods return an
auxiliary::DeferredComputation<T>object that encapsulates the operation without immediately executing it. The computation is only performed whenget()(oroperator()()) is called on the returned object:Configuration Methods
The configuration chain supports:
offset(Offset)- Set the offset within the dataset (optional)extent(Extent)- Set the extent within the dataset (optional)memorySelection(MemorySelection)- Set memory selection for non-contiguous buffers (only available afterwithSharedPtr(),withRawPtr(), orwithContiguousContainer())unsafeNoAutomaticFlush(): The returnedDeferredComputationobject will be a no-op object. Buffers will still be returned, but they might not yet be written/read until manually flushing.Buffer Specification Methods
If not specifying a buffer, the following allocating load/store operations are available:
storeSpan<T>()- ReturnsDynamicMemoryView<T>(a span-like view)load<T>()- ReturnsDeferredComputation<std::shared_ptr<T>>loadVariant()- ReturnsDeferredComputation<variant>for type-erased loadingload<T>(policy)- Returnsstd::shared_ptr<T>>Alternatively, you may specify a buffer using one of:
withSharedPtr(std::shared_ptr<T>)- For shared pointer managed bufferswithUniquePtr(UniquePtrWithLambda<T>)- For unique pointer with custom deleterwithRawPtr(T*)- For raw pointer bufferswithContiguousContainer(Container&)- For contiguous containers (automatically deduces size if extent is not set)The following operations are only available after specifying a buffer. Template parameters are no longer needed as the type is given through the buffer type:
store()/load()- ReturnsDeferredComputation<void>(Load operations are only available after specifying a writable buffer, i.e. raw/shared pointer or contiguous container of non-const type)
ADIOS2 Memory Selection Support
This PR adds support for ADIOS2 memory selections, allowing writing from non-contiguous memory blocks:
Note: Memory selection support depends on ADIOS2 version. For ADIOS2 versions that cannot reset memory selections (PR ornladios/ADIOS2#4169), a warning is displayed. The API gracefully handles both cases.
Migration from Old API
The old
loadChunk()andstoreChunk()methods are now fully implemented using the new API internally, providing full backward compatibility while benefiting from the new implementation. Existing code continues to work without changes.Examples of Old to New Migration
Old:
New:
Old:
New:
E_y.prepareLoadStore() .offset({0, 5}) .extent({1, 5}) .withContiguousContainer(data) .store();Design Considerations & Open Questions
Memory Selection Limitations
prepareLoadStore())Files Changed
include/openPMD/LoadStoreChunk.hpp- New header with the main APIinclude/openPMD/LoadStoreChunk.tpp- Template implementationsinclude/openPMD/auxiliary/Future.hpp- NewDeferredComputationwrapperinclude/openPMD/Dataset.hpp- AddedMemorySelectionstructinclude/openPMD/RecordComponent.hpp- Integration of new APIsrc/LoadStoreChunk.cpp- Non-template implementationssrc/auxiliary/Future.cpp- DeferredComputation implementationTODO
.noop_future()option to disable future return types for cases where you don't need to track completionRelated
TODO: