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

Implement BP5 (and BP4) reader-side memory selection, do testing #3591

Merged
merged 5 commits into from
Apr 28, 2023

Conversation

eisenhauer
Copy link
Member

@eisenhauer eisenhauer commented Apr 24, 2023

Despite this having changes in testing/adios/interface, it actually doesn't involve any changes in the API or ABI. The story is this: There is a call, SetMemorySelection(), that affects how ADIOS accesses memory on both the writer and reader side. While there are tests for SetMemorySelection() on the writer side in testing/adios/bp, the only place where reader-side SetMemorySelection was tested was in testing/adios/interface, and oddly those tests were hard-coded to only use the BP3 engine (which hasn't been the default forever). Turns out that the reason that it was hard-coded is that that reader-side functionality was missing from BP4 and BP5. This PR implements that missing functionality in those engines and tweaks the testing so that the engine is no longer hard-coded as BP3 in the test, but is specified on the command line so that we can actually test this with BP3, BP4 and BP5. We might want to think about further steps, like moving this test into engine/bp or staging-common and cleaning up other things, but this PR at least adds the missing functionality and tests it.

@eisenhauer eisenhauer changed the title Implement BP5 reader-side memory selection Implement BP5 (and BP4) reader-side memory selection, do testing Apr 25, 2023
@eisenhauer
Copy link
Member Author

@vicentebolea Despite this having changes in testing/adios/interface, it actually doesn't involve any changes in the API or ABI. The story is this: There is a call, SetMemorySelection(), that affects how ADIOS accesses memory on both the writer and reader side. While there are tests for SetMemorySelection() on the writer side in testing/adios/bp, the only place where reader-side SetMemorySelection was tested was in testing/adios/interface, and oddly those tests were hard-coded to only use the BP3 engine (which hasn't been the default forever). Turns out that the reason that it was hard-coded is that that reader-side functionality was missing from BP4 and BP5. This PR implements that missing functionality in those engines and tweaks the testing so that the engine is no longer hard-coded as BP3 in the test, but is specified on the command line so that we can actually test this with BP3, BP4 and BP5. We might want to think about further steps, like moving this test into engine/bp or staging-common and cleaning up other things, but this PR at least adds the missing functionality and tests it.

Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

From a software packaging perspective it looks good, no incompatible changes in the API/ABI. From a functional perspective, this corrects a missing functionality in BP4/BP5 engine. Currently, the change is small and contained in the BP4Deserializer::PostDataRead method.

Question, where is this functionality implemented for BP5?

@@ -289,12 +293,14 @@ TEST_F(ADIOS2_CXX11_API_Selection, SelectionWrite)

// read back
auto arr_read = MultiArrayT(ref.dims());
m_IOReader.SetEngine("BP3");
// m_IOReader.SetEngine("BP3");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe remove this

Copy link
Member Author

Choose a reason for hiding this comment

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

Question, where is this functionality implemented for BP5?
The changes are in BP5Deserializer, where a single call got replaced by an if with a vastly more complex then clause.

Also, this contains a bug fix for the BP5Serializer that was causing a memory overrun when there was more than one block/variable written and min/max was enabled. That it got past msan means that we're not running any of those sorts of tests there. That is somewhat worrisome as it means that some pretty basic functionality isn't being sanitized. (I know why, we can't do everything within the hour time limit, but its probably something we should look at doing nightly, or weekly, or something.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is somewhat worrisome as it means that some pretty basic functionality isn't being sanitized

Indeed true, we have this idea of extending the sensitization test in our backlog. This discovery increases the priority in the task.

testing/adios2/interface/TestADIOSSelection.cpp Outdated Show resolved Hide resolved
testing/adios2/interface/TestADIOSSelection.cpp Outdated Show resolved Hide resolved
source/adios2/toolkit/format/bp/bp4/BP4Deserializer.tcc Outdated Show resolved Hide resolved
Copy link
Collaborator

@vicentebolea vicentebolea left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the quick response. +1

@eisenhauer eisenhauer merged commit 702cc8f into ornladios:release_29 Apr 28, 2023
@eisenhauer eisenhauer deleted the BP5MemSel branch April 28, 2023 01:17
vicentebolea pushed a commit that referenced this pull request May 3, 2023
Implement BP5 (and BP4) reader-side memory selection, do testing
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants