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

Implemented MPI_Waitany #88

Merged
merged 3 commits into from
Apr 1, 2020
Merged

Conversation

finoalessiopolimi
Copy link

I needed the MPI_Waitany for a personal project so I decided to implement it as a function in the Request module. This is the first time i work with ffi and unsafe code so I tried to keep things as simple as possible.
I'm not sure if using a Vec of Request as input parameter and removing the completed Request from it is the best way to go but it was the simplest I could find.

Copy link
Contributor

@AndrewGaspar AndrewGaspar left a comment

Choose a reason for hiding this comment

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

Thanks for adding this - I opened an MR a couple years now to resolve this, and I overdid it a bit - was trying to make it so that you didn't have to copy to a separate allocation for the MPI_Request objects. This was probably an unnecessary optimization.

pub fn wait_any<'a, S: Scope<'a>>(requests: &mut Vec<Request<'a, S>>) -> (i32, Status) {
let mut mpi_requests: Vec<MPI_Request> = Vec::with_capacity(requests.len());
let mut scopes: Vec<S> = Vec::with_capacity(requests.len());
unsafe {
Copy link
Contributor

Choose a reason for hiding this comment

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

reduce the scope of this unsafe - be more targeted in where you apply it.

src/request.rs Outdated
let mut scopes: Vec<S> = Vec::with_capacity(requests.len());
unsafe {
let mut index: i32 = mpi_sys::MPI_UNDEFINED;
while let Some(r) = requests.pop() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than draining the requests object and then re-filling it, could you instead just build the mpi_requests objects using as_raw?

e.g.

let mut mpi_requests: Vec<_> = requests.iter().map(|r| r.as_raw()).collect();

Then after MPI_Waitany succeeds, just do requests.remove(index);. You'll need to make sure that there's no panic between MPI_Waitany and the remove call.

I think this approach has two benefits:

  1. Eliminates the allocation of the scopes vector
  2. Fewer moves in the requests vector.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, you're also not reversing the list and then don't have to deal with fixing up the index.

src/request.rs Outdated
///
/// The completed request is removed from the vector of requests.
///
/// If no Request is active the index returned is mpi_sys::MPI_UNDEFINED.
Copy link
Contributor

Choose a reason for hiding this comment

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

return an Option<(i32, Status)> instead - prefer Rust idioms when there's a clear mapping.

src/request.rs Outdated
/// # Examples
///
/// See `examples/wait_any.rs`
pub fn wait_any<'a, S: Scope<'a>>(requests: &mut Vec<Request<'a, S>>) -> (i32, Status) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's actually make this a usize - we're already checking that it's convertible anyway.

@AndrewGaspar
Copy link
Contributor

By the way, stay strong. America stands with Italy!

@finoalessiopolimi
Copy link
Author

Thank you Andrew, I hope the situation there in America don't get as bad as here.

I've fixed the problems you have spotted in the implementation.

@AndrewGaspar AndrewGaspar merged commit 4f07e74 into rsmpi:master Apr 1, 2020
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.

None yet

2 participants