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.

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

ADIOS2: v2.7.0+ #927

Merged
merged 8 commits into from
Mar 8, 2021
Merged

ADIOS2: v2.7.0+ #927

merged 8 commits into from
Mar 8, 2021

Conversation

ax3l
Copy link
Member

@ax3l ax3l commented Feb 10, 2021

Increase ADIOS2 dependency to v2.7.0+

  • update docs and build logic @ax3l
  • bump CI to have access to newer ADIOS @ax3l
  • remove pure ADIOS<2.7.0 work-arounds from code-base @franzpoeschel

@@ -22,6 +22,7 @@ Steps

ADIOS2 is optimized towards organizing the process of reading/writing data into IO steps.
In order to activate steps, it is imperative to use the :ref:`Streaming API <usage-streaming>` (which can be used for either file-based or streaming-based workflows).
@franzpoeschel: let's remove these work-arounds :-)
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing this workaround would be a breaking change to our written datasets, making them unreadable with old versions of the openPMD API. I'd postpone the removal of this workaround until switching to the new schema (I've re-set the defaults accordingly for the new schema in #855).

Copy link
Member Author

@ax3l ax3l Feb 24, 2021

Choose a reason for hiding this comment

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

Sounds great!
Do you like to rephrase the section here then? It might be confusing for us and users to talk about 2.6.0 if we mean the openPMD 1 layout after this PR (because this PR drops ADIOS 2.6.0). Maybe you can use the same phrasing as in the doxygen comment (backwards compatible with 2.6.0 output that needed to work around a bug...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I somehow missed this one. Updated it.

@@ -1245,6 +1245,8 @@ namespace detail
* is opt-in for now.
* 2) Reading with the Streaming API any file that has been written
* without steps.
*
* @todo [franzpoeschel] let's remove these work-arounds :-)
Copy link
Contributor

Choose a reason for hiding this comment

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

The description in this one is a bit incomplete. As of ADIOS 2.7.0, it is still necessary to delay opening the first step in case a reader does not use steps at all.
But in any case, I'll need to update the description of this (and also mention somewhere that its difference to the state OutsideOfStep lies in the fact that requireActiveStep() will not open a step).

@franzpoeschel
Copy link
Contributor

Hmm, I tried to activate the tests under Windows that I thought were failing because of ADIOS 2.6.0. Apparently, they're still failing..

@franzpoeschel
Copy link
Contributor

I threw out the failing commit, but something else went wrong again

@franzpoeschel franzpoeschel force-pushed the adios-2.7.0 branch 2 times, most recently from befbd74 to b7009eb Compare February 24, 2021 13:10
@franzpoeschel
Copy link
Contributor

I've fixed the SSC issues. Also, rebased since there was a conflict.

@@ -102,7 +102,10 @@ namespace detail
{
numItems *= extent;
}
new( dest ) T[ numItems ]{};
for( size_t i = 0; i < numItems; ++i )
Copy link
Member Author

@ax3l ax3l Mar 4, 2021

Choose a reason for hiding this comment

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

That construct looks expensive, why do we need to loop this now?
Update: just saw #926 (comment)

Just to get a feeling, how large is numItems usually?
Update: I just learned what a placement new is, so it's probably not more expensive to loop here? :D

I trust that you know what you are doing: https://stackoverflow.com/a/36854950/2719194 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, should have written something on that one here too.
In most cases, numItems is 1, for our unit dimension things it is 7. I think, the largest instances are for the VEC_STRING datatype since those are represented as 2D char vectors, so you'll get some 2-digit number there.

so it's probably not more expensive to loop here

I think it shouldn't be since this doesn't allocate anything.

Theoretically, we could also memset the whole thing to zero and use placement new only for non-basic datatypes, but that makes things very finicky to deal with and I don't think it's worth.

Also, I don't use delete[], see struct AttributeLocationDestroy in the same file for the destructors.

But yeah, this file was tricky to get right

ax3l and others added 7 commits March 8, 2021 12:24
Increase ADIOS2 dependency to v2.7.0+
This one does not yet have E4S caches, so build times might go
up a bit.
To do this, add .ssc as a possible file ending and enlist it under the
file endings.
This also fixes broken array placement new in MSVC
@franzpoeschel
Copy link
Contributor

Rebased this again to get the frontend redesign (I use this as base for some other PRs).

@ax3l ax3l merged commit 969b430 into openPMD:dev Mar 8, 2021
@ax3l ax3l deleted the adios-2.7.0 branch March 8, 2021 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants