Skip to content

Conversation

@franzpoeschel
Copy link
Contributor

@franzpoeschel franzpoeschel commented Mar 3, 2022

  1. See this comment, the Span-based API is still very much relevant for performance in BP5.
  2. The BP5 engine requires that every interaction with it happens inside an active IO step, take care of this

@franzpoeschel franzpoeschel mentioned this pull request Mar 8, 2022
@franzpoeschel franzpoeschel changed the title Enable Span-based API in ADIOS2 BP5 engine Fixes for BP5 engine Mar 15, 2022
@franzpoeschel franzpoeschel force-pushed the adios2-bp5-enable-span-api branch 2 times, most recently from c33e761 to 91b438f Compare March 15, 2022 16:55
@franzpoeschel franzpoeschel force-pushed the adios2-bp5-enable-span-api branch from 91b438f to 02cffc4 Compare March 15, 2022 17:22
if (it != streamingEngines.end())
{
isStreaming = true;
optimizeAttributesStreaming =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can think about removing this optimization, it becomes irrelevant in variable-based iteration encoding and there is no harm in using that encoding for streaming.
Not relevant for this PR though.

{
adios2::Engine &eng = getEngine();
/*
* If streamStatus is Parsing, do NOT open the step.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should have added this comment earlier already, just took the opportunity now

// return status is stored in m_lastStepStatus
if (streamStatus != StreamStatus::DuringStep)
{
flush(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flushing here was wrong but harmless before. This dormant bug has woken up with this PR, so change it.

#if openPMD_HAVE_ADIOS2 && \
ADIOS2_VERSION_MAJOR * 1000000000 + ADIOS2_VERSION_MINOR * 100000000 + \
ADIOS2_VERSION_PATCH * 1000000 + ADIOS2_VERSION_TWEAK >= \
2701001223
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note that our CI does not yet have any ADIOS2 versions that satisfy these conditions

@franzpoeschel
Copy link
Contributor Author

Reminder to myself: ReadRandomAccess

@franzpoeschel franzpoeschel force-pushed the adios2-bp5-enable-span-api branch from 9bbaf87 to e58ff30 Compare March 22, 2022 14:04
@ax3l ax3l added this to the 0.14.5 milestone Apr 15, 2022
@ax3l ax3l self-requested a review April 15, 2022 17:12
Copy link
Member

@ax3l ax3l left a comment

Choose a reason for hiding this comment

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

Thank you!

Did I see correctly that we should backport this to 0.14.5?

@ax3l ax3l merged commit 29c587b into openPMD:dev Apr 15, 2022
@ax3l
Copy link
Member

ax3l commented May 25, 2022

As discussed: targeting BP5 patches for the 0.15.0 release only (no backports to 0.14.*)

@ax3l ax3l removed this from the 0.14.5 milestone May 25, 2022
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