openPMD-api icon indicating copy to clipboard operation
openPMD-api copied to clipboard

Add unique_ptr overloads for storeChunk calls

Open franzpoeschel opened this issue 3 years ago • 2 comments

See https://github.com/openPMD/openPMD-api/issues/1216 for an explanation. The approach in this PR is to use unique_ptrs instead of relying on the refcount of shared_ptrs to make this optimization more explicit to the user.

TODO

  • [x] Add a class OpenpmdUniquePtr that allows specifying dynamic destructors via std::function<void(T*)>, similar to std::shared_ptr. In the STL, unique_ptrs have a different type depending on their destructor, which is annoying for our use case, and the overhead of making this dynamic is negligible in our use case. This way, it's easily possible to e.g. pass ownership of pinned memory to openPMD and still let it correctly destroy it once used.
  • [x] Extend the IOTask for Write_Dataset operations to accept std::variant<std::shared_ptr<void const>, OpenpmdUniquePtr<void const>> as a buffer. This requires some light refactoring because with this change, the IOTask is not copyable any more. This includes making the backends deal with both types correctly.
  • [x] Add a storeChunk overload that accepts OpenpmdUniquePtrs
  • [x] Actually implement the intended optimization in the ADIOS2 backend
  • [ ] testing, documentation
  • [x] support for determineDatatype
  • [ ] Merge #1207 first
  • [x] Check if it also works to let this be handled by Engine::close().

@ax3l This PR should not be too much work any more, so we might as well add this to 0.15.*?

franzpoeschel avatar Jun 24 '22 13:06 franzpoeschel

This pull request introduces 13 alerts when merging 41b5eb94a430b7b7a2d5919ec235d7ab13e4d581 into d64365bbc2348b5622e8e65d1a4f4dbfdc8ae313 - view on LGTM.com

new alerts:

  • 13 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com[bot] avatar Jun 24 '22 14:06 lgtm-com[bot]

This pull request introduces 1 alert when merging 28ec211b86c92eeeca17474fdb433872318e9626 into 230afddf72e537b957408ec5ebb4702711abdd09 - view on LGTM.com

new alerts:

  • 1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

lgtm-com[bot] avatar Oct 12 '22 09:10 lgtm-com[bot]