Add unique_ptr overloads for storeChunk calls by franzpoeschel · Pull Request #1294 · openPMD/openPMD-api

franzpoeschel · 2022-06-24T13:22:35Z

See #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

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.
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.
Add a storeChunk overload that accepts OpenpmdUniquePtrs
Actually implement the intended optimization in the ADIOS2 backend
testing, documentation
support for determineDatatype
Merge ADIOS2: Flush to disk within a step #1207 first
Check if it also works to let this be handled by Engine::close().
I think it might be possible to implement this in such a way that makes OpenpmdUniquePtr not leak into the public API.

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

lgtm-com · 2022-06-24T14:00:33Z

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

new alerts:

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

include/openPMD/auxiliary/Memory.hpp

franzpoeschel · 2022-07-07T11:11:58Z

src/IO/ADIOS/ADIOS2IOHandler.cpp

+
+            if (performDataWrite)
+            {
+                // should we really do this now?


Suggested change

// should we really do this now?

Should we write attributes before PerformDataWrites()?

Yes: Then setting another value for the attribute later won't work (see e.g SerialIOTest:)
o.setOpenPMDextension( 1); // this happens intentionally "late" in this test

No: Then attributes would only be written very late, i.e. before EndStep().

Personally, I think if preferred_flush_target = disk is selected, then everything that can be written to disk should be, so we should leave it as it is.

I think we can revisit this once we change the defaults of frontends and avoid setting defaults of attributes too early. Writing attributes after datasets are defined can help with sensible defaults, e.g., x,y,z for 3D but only x,y for 2D mesh axis labels, etc.

lgtm-com · 2022-10-12T09:44:35Z

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

new alerts:

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

test/CoreTest.cpp

franzpoeschel · 2023-01-10T09:03:24Z

include/openPMD/RecordComponent.hpp


+    void storeChunk(
+        auxiliary::WriteBuffer buffer, Datatype datatype, Offset o, Extent e);
+


private, no new API

(could nonetheless have Doxygen block for us ;-) )

include/openPMD/IO/ADIOS/ADIOS2Auxiliary.hpp

franzpoeschel · 2023-01-10T13:52:31Z

include/openPMD/Datatype.hpp

-template <typename T_ContiguousContainer>
-inline constexpr Datatype determineDatatype(T_ContiguousContainer &&)
+template <typename T>
+inline constexpr Datatype determineDatatype(T &&val)


This function is now aware of std::unique_ptr as well as OpenpmdUniquePtr, additionally to the previous raw and shared pointers. It's now implemented via if constexpr (previously overloads). Otherwise, the API is unchanged.

Ok. Could be a good candidate for a C++ concept at some point.

include/openPMD/RecordComponent.hpp

include/openPMD/auxiliary/UniquePtr.hpp

Refactor ::clone() to ::to_heap(), also make copy/move constructors/operators in AbstractParameter protected instead of deleting them.

No optimizations based on that yet

Not yet implemented, so test fails

test/CoreTest.cpp

 #endif
 }
+
+TEST_CASE("unique_ptr", "[core]")


ax3l · 2023-02-22T22:51:28Z

include/openPMD/IO/IOTask.hpp

@@ -704,9 +677,10 @@ struct OPENPMDAPI_EXPORT Parameter<Operation::DEREGISTER>
    Parameter(Parameter const &) : AbstractParameter()


CodeQL: Looks like Parameter generally lacks the copy assignment operator (independent of this PR):

Parameter& operator=(const Parameter& other)

https://github.com/openPMD/openPMD-api/security/code-scanning/498

ax3l · 2023-02-22T22:52:39Z

include/openPMD/RecordComponent.hpp


+    void storeChunk(
+        auxiliary::WriteBuffer buffer, Datatype datatype, Offset o, Extent e);
+


(could nonetheless have Doxygen block for us ;-) )

ax3l · 2023-02-22T22:58:10Z

src/IO/ADIOS/ADIOS2IOHandler.cpp

+                    "ADIOS2 backend: Flush of late writes was requested at the "
+                    "wrong time.");


This error message is not hard to understand for me :)

To be fair, this is an error::Internal that should only happen when there is a bug somewhere in our code. So if this error occurs, it's not really supposed to be understood by the user, but to help them report the issue in more detail.

error::Internal is always thrown in combination with a request to report it:

Internal::Internal(std::string const &what) : Error( "Internal error: " + what + "\nThis is a bug. Please report at ' " "https://github.com/openPMD/openPMD-api/issues'.") {}

That is great!

I mean for us (devs) to remember, I would extend the half sentence about the "wrong time" a bit :D

ax3l

This look really good to me. I have a few inline comments that can be addressed as a follow-up :)

franzpoeschel added backend: ADIOS2 api: new additions to the API labels Jun 24, 2022

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from 41b5eb9 to c02669a Compare June 24, 2022 15:24

This was referenced Jun 27, 2022

Optimizations for BP5 engine of ADIOS2: Use span-based API and avoid flushing before closing an Iteration BLAST-WarpX/warpx#3200

Closed

storeChunk: Add an overload for shared_ptr<T[]> #1296

Merged

franzpoeschel commented Jun 28, 2022

View reviewed changes

include/openPMD/auxiliary/Memory.hpp Outdated Show resolved Hide resolved

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch 3 times, most recently from 020d2dd to dd71f38 Compare July 6, 2022 19:04

franzpoeschel commented Jul 7, 2022

View reviewed changes

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch 3 times, most recently from 01a0056 to ed1ac70 Compare July 19, 2022 12:25

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from ed1ac70 to 96b4721 Compare July 29, 2022 08:27

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch 2 times, most recently from 1e73b45 to 8f3a93e Compare August 17, 2022 09:17

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from 8f3a93e to 28ec211 Compare October 12, 2022 09:06

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from 28ec211 to 829eb3e Compare October 12, 2022 09:49

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from 829eb3e to 86025de Compare October 26, 2022 10:13

ax3l added this to the 0.15.0 milestone Nov 3, 2022

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from b1e6f8b to 4acd9d1 Compare December 19, 2022 12:00

github-advanced-security bot found potential problems Dec 19, 2022

View reviewed changes

test/CoreTest.cpp Fixed Show fixed Hide fixed

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch 3 times, most recently from a271126 to 7645264 Compare January 9, 2023 15:57

franzpoeschel commented Jan 10, 2023

View reviewed changes

include/openPMD/IO/ADIOS/ADIOS2Auxiliary.hpp Outdated Show resolved Hide resolved

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from 7645264 to 15c5bef Compare January 10, 2023 13:04

franzpoeschel commented Jan 10, 2023

View reviewed changes

include/openPMD/RecordComponent.hpp Outdated Show resolved Hide resolved

franzpoeschel commented Jan 10, 2023

View reviewed changes

include/openPMD/auxiliary/UniquePtr.hpp Outdated Show resolved Hide resolved

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch 3 times, most recently from 8ef87d9 to c685485 Compare January 16, 2023 10:18

ax3l self-requested a review January 25, 2023 17:55

ax3l self-assigned this Jan 25, 2023

franzpoeschel mentioned this pull request Feb 10, 2023

Update examples #1371

Merged

1 task

franzpoeschel added 9 commits February 20, 2023 14:18

Add OpenpmdUniquePtr class

05f41cd

Prepare IOTask.hpp for a non-copyable parameter

30b73f3

Refactor ::clone() to ::to_heap(), also make copy/move constructors/operators in AbstractParameter protected instead of deleting them.

Backend fully prepared for accepting unique_ptr buffers

8084f4f

No optimizations based on that yet

Add storeChunk(OpenpmdUniquePtr, ...) overload

3b42ce3

Fix invasive tests

ae69537

Add test for ADIOS2 backend optimization

2d9714f

Not yet implemented, so test fails

Implement ADIOS2 backend optimization

ad124af

Support also regular std::unique_ptr

edd2e90

CI fixes

cfa03e5

franzpoeschel force-pushed the topic-storeChunk-unique-ptr branch from c685485 to cfa03e5 Compare February 20, 2023 13:28

franzpoeschel added 2 commits February 21, 2023 17:45

Use OpenpmdUniquePtr with storeChunk in an example

2a385f1

Rename OpenpmdUniquePtr -> UniquePtrWithLambda

74470f4

github-advanced-security bot found potential problems Feb 22, 2023

View reviewed changes

test/CoreTest.cpp

#endif

}

TEST_CASE("unique_ptr", "[core]")

Check notice

Code scanning / CodeQL

Unused static function

Static function C_A_T_C_H_T_E_S_T_50 is unreachable ([autoRegistrar51](1) must be removed at the same time)

ax3l mentioned this pull request Feb 22, 2023

ADIOS2 BP5 optimization: Allow users to "give up" their data into openPMD #1216

Closed

ax3l reviewed Feb 22, 2023

View reviewed changes

ax3l approved these changes Feb 22, 2023

View reviewed changes

ax3l merged commit 9a215b2 into openPMD:dev Feb 22, 2023


		void storeChunk(
		auxiliary::WriteBuffer buffer, Datatype datatype, Offset o, Extent e);

		@@ -704,9 +677,10 @@ struct OPENPMDAPI_EXPORT Parameter<Operation::DEREGISTER>
		Parameter(Parameter const &) : AbstractParameter()

		"ADIOS2 backend: Flush of late writes was requested at the "
		"wrong time.");

Conversation

franzpoeschel commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgtm-com bot commented Jun 24, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgtm-com bot commented Oct 12, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Check notice

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l Mar 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ax3l left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

franzpoeschel commented Jun 24, 2022 •

edited

Loading

ax3l Feb 22, 2023 •

edited

Loading

ax3l Feb 22, 2023 •

edited

Loading

ax3l Mar 21, 2023 •

edited

Loading