-
Notifications
You must be signed in to change notification settings - Fork 52
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
Tracking Issue: Multiple Completion of Requests #29
Comments
Hey there! Thanks for your input. I agree that the plural versions of the request completion routines are a very important part of the non-blocking machinery. I have not gotten around to implementing them yet, among other things because I was also still pondering possible ways of making them more idiomatic in Rust. I think your Returning Maybe indexing into the On the other hand, if adding a By the way, you call these routines "collective" twice, but I think you meant "completion". In the context of MPI I have only seen the term "collective" applied to |
Thanks for the quick response! I really like this library, by the way. Apologies about "collective", I'm a bit of an MPI newbie. The MPI 3.1 spec refers to these routines collectively as "Multiple Completions", so that sounds good to me. I'll edit the original issue. I should be a little more specific with the index interface of I think it first makes sense to promote the bulk of Roughly: trait AsyncRequest<'a, S: Scope<'a>> {
// as_raw, get_scope, and unsubscribe must be implemented by struct
unsafe fn as_raw(&self) -> MPI_Request;
fn get_scope(&self) -> S;
// unsubscribe is called when the request is being removed from its scope
fn unsubscribe(&mut self);
// existing `Request` methods are implemented using as_raw and get_scope
pub unsafe fn into_raw(mut self) -> (MPI_Request, S) { ... }
pub fn wait_with() { ... }
pub fn wait(self) -> Status { ... }
// etc...
}
// owns a standalone MPI_Request with an attached scope.
// PANICs on drop if the request is not completed.
struct Request<'a, S: Scope<'a> = StaticScope> {
request: MPI_Request,
scope: S,
phantom: PhantomData<Cell<&'a ()>>,
}
impl<'a, S: Scope<'a>> AsyncRequest<'a, S> for Request<'a, S> {
unsafe fn as_raw(&self) -> MPI_Request { self.request.clone() }
fn get_scope(&self) -> S { self.scope }
fn unsubscribe(&mut self) { /* doesn't need to do anything */ }
}
// owns a reference into a RequestCollection, inherits its scope
// Does not panic on drop, but returns ownership to to the collection.
struct RequestRef<'a, 'b: 'a, S: Scope<'a> = StaticScope> {
request: &'b mut MPI_Request,
scope: S,
phantom: PhantomData<Cell<&'a ()>>,
}
impl<'a, 'b: 'a, S: Scope<'a>> AsyncRequest<'a, S> for RequestRef<'a, 'b, S> {
unsafe fn as_raw(&self) -> MPI_Request { self.request.clone() }
fn get_scope(&self) -> S { self.scope }
// this is so the slot in the request collection appears as null, and MPI will
// ignore it in future multi-completion operations.
fn unsubscribe(&mut self) { self. request = unsafe_extern_static!(ffi::RSMPI_REQUEST_NULL); }
} To answer your concern about indexing into a RequestCollection returning a reference, it's important to note that I think returning the index in the _any and _some operations is important because the user may be storing per-request context information off to the side, so you may need the index as some key to look up additional information about the request. So that's why I think we shouldn't hide the indices from the user. You can also imagine a case where a user may want to Here's a fairly detailed description of an imagined // return value from test_any(). See test_any documentation for more detail.
enum TestAny {
NoneActive,
NoneComplete,
Completed(usize, Status),
}
impl<'a, S: Scope<'a>> RequestCollection<'a, S> {
pub fn new() -> Self { ... }
pub fn with_capacity(capacity: usize) -> Self{ ... }
// etc. whatever other constructors might be useful
pub fn len(&self) -> usize { ... }
// push() maybe should return the index of the request?
pub fn push(&mut self, request: Request<'a, S>) { ... }
// Description:
// returns `None` if the request at idx has already been completed or
// otherwise removed
// from the collection
// Returns:
// - `None` if the request at idx has been completed.
// - `Some(request)` if the request is still active.
// Notes:
// there's no `at()` since it seems like most Request operations require
// mutability.
//
// Since RequestRef owns a mutable reference to RequestCollection,
// the collection cannot be modified until the RequestRef is returned one
// way or another. RequestRef is naturally returned when it goes out of
// scope. But it may also be returned by a one-off test() or wait(), which
// destroys the reference and marks the Request as inactive in the
// collection.
//
// It also may be desirable to get multiple RequestRef's at once, but this
// current model would not allow it. Might be worth thinking more about
// this.
pub fn at_mut<'b>(&'b mut self, idx: usize) -> Option<RequestRef<'a, 'b, S>> { ... }
// Description:
// This is a zero-overhead wrapper of MPI_Waitany.
// Returns:
// - `None` if all requests are inactive.
// - Some((idx, status)) if there are still active requests. idx is the
// index of the completed request, and status is the status it was
// completed with.
// The request at idx will be `None` on return.
pub fn wait_any(&mut self) -> Option<(usize, Status)> { ... }
// Description:
// This is a zero-overhead wrapper of MPI_Waitsome.
// Returns:
// - `None` if all requests are inactive. indices and statuses are not
// modified.
// - If there are still active requests, return `Some(completed)`, where
// `completed` is the number of requests completed by the call.
// `completed` is guaranteed to be >= 1.
// indices[..completed] and statuses[..completed] will contain the
// completed request indices and statuses, respectively.
// The requests at indices[..completed] will be `None` on return.
// Notes:
// It panics if indices.len() and status.len() are not >= self.len().
// This assumes that Status is transmutable to MPI_Status.
pub fn wait_some(&mut self, indices: &mut [usize], statuses: &mut [Status]) -> Option<usize> { ... }
// Description:
// This is a zero-overhead wrapper of MPI_Waitall.
// All requests will be `None` on return.
// Notes:
// I chose to not have it destroy the RequestCollection since the other
// wait_* methods do not destroy the collection.
pub fn wait_all(&mut self) { ... }
// Description:
// This is a zero-overhead wrapper of MPI_Testany.
// Returns:
// - TestAny::NoneActive, if there are no active requests in the
// collection
// - TestAny::NoneComplete, if the test yields no completed request.
// - Completed(idx, status), when a completed request is available.
// idx is the index of the request in the collection, and status is the
// status of the completed request.
// The request at `idx` will be `None` on return.
// Notes:
// A possible alternative return type is
// Option<Option<(usize, Status)>>. That is, it would return
// `None` if all the requests in the collection were inactive. It would
// return `Some(None)` if there aren't any completed requests. And it
// return `Some(Some((idx, status)))` when there is a completed request.
// This matches-ish with test_some, but double nested Options seems kind
// of confusing to me, which is why I opted for the custom enum.
pub fn test_any(&mut self) -> TestAny { ... }
// Description:
// This is a zero-overhead wrapper of MPI_Testsome.
// Returns:
// - `None` if all requests are inactive. indices and statuses are not
// modified.
// - If there are still active requests, return `Some(completed)`, where
// `completed` is the number of requests completed by the call.
// `completed` may be 0.
// indices[..completed] and statuses[..completed] will contain the
// completed request indices and statuses, respectively.
// The requests at indices[..completed] will be `None` on return.
// Notes:
// It panics if indices.len() and statuses.len() are not >= self.len().
// This assumes that Status is transmutable to MPI_Status.
pub fn test_some(&mut self, indices: &mut [usize], statuses: &mut [Status]) -> Option<usize> { ... }
// Description:
// This is a zero-overhead wrapper of MPI_Waitall.
// Returns:
// - `false` if not all requests are complete. No requests will be
// modified. The value of statuses[..self.len()] is undefined.
// - `true` if all active requests are complete. All requests will be set
// to `None`. `statuses` will contain the completion status of each of
// the requests completed by test_all. Statuses for null or inactive
// requests will be empty.
// Notes:
// Panics if statuses.len() is not >= self.len()
pub fn test_all(&mut self, statuses: &mut [Status]) -> bool { ... }
} Apologies for the brain dump. Also, apologies if any of this code, especially the explicit lifetimes, is not quite right. My free-hand Rust isn't too good yet. :) Let me know what you think - I might try making a prototype this weekend. |
Thanks for the praise. Do not worry about not getting the terminology right 100% of the time. At more than 800 pages, the MPI standard is probably too large to know by heart, especially when you are only just starting. I like the Regarding the Further down the road it might also be worthwhile to play around with a higher level interface based on top of this one but using iterators, but this is a good start, I think. Lastly, I am afraid, I will have to do some more rules lawyering. The term "inactive" does not apply to the kind of requests that can be used through this API. MPI has two kinds of requests, "persistent" and "nonpersistent" (which are used here), with different APIs and very different life cycles. Nonpersistent requests have two states, "active" and "deallocated" (meaning they have the value Persistent requests have an additional state "inactive". They are created in the inactive state using "init" routines like I think it is a bit unfortunate, that MPI lumps these two kinds of objects together in the same type so that it effectively leads a double life and I am not yet sure how to map that into If you do find the time this week end I would be happy to review your prototype next week. |
Just so you know, I'm still working on this, just didn't get it finished yesterday. |
No worries. |
I realized to my frustration that this proposal is insufficient. Consider the following pseudo-code let recv_buffers: Vec<&mut [f64]> = ...;
let recv_ranks: Vec<Rank> = ...
mpi::request::scope(|scope| {
let recv_requests =
recv_ranks
.iter()
.zip(&mut recv_buffers)
.map(|(rank, buffer)| {
world
.process_at_rank(rank)
.immediate_receive_into(scope, buffer)
})
.collect_requests(scope);
while let Some(indices) = recv_requests.wait_some_without_status() {
for i in indices {
// ERROR: This borrow fails because recv_buffer is still borrowed
// mutably
let buffer = &recv_buffers[i];
// do something with buffer
}
}
}); Essentially, I think there is a Rust compatible solution to this: Essentially: while let Some(indices) = recv_requests.wait_some_without_status() {
for i in indices {
// PANICS if the request at `i` is non-null
// also comes in get_buffer_mut flavor
let buffer = recv_requests.get_buffer(i);
// do something with buffer
}
} The simple version of this is a
However, I suspect even this isn't enough. You'd probably in some situations want to register essentially a This needs to be fleshed out a little more, and I think it's actually essential for this proposal, outside of
You can side step all this by using I'm sure there are other questions that need to be answered, so let me know if you can think of any. I'll need to let my mind stew on this for a bit. Man, Rust can make things hard. 😜 |
This is what I have in mind: https://gist.github.com/AndrewGaspar/04ab66dbe59e8d85ad73fd8783372589 |
Yeah, I see the problem. I remember that @Rufflewind actually did experiment with attaching Buffers to Requests when he rewrote this part of rsmpi. His work can still be found here: https://github.com/Rufflewind/rsmpi/tree/buffer. Maybe it would make sense to revive that? |
I have an internal |
Thanks for the prior art - it's very helpful. If we have to deal with a I'll see if we can use a trait that would allow you to use a |
I've got a branch off of this one tracking this work here: https://github.com/AndrewGaspar/rsmpi/tree/multi-completion-managed-buffer |
I've been working on getting the data owned by the I'm considering getting rid of the scope parameter to the immediate methods, though I've been trying to avoid changing the public API as much as possible. Scope could really just be another type of "data" that a request can own. So although impl<D> Request<D> {
// ...
fn forget_data<'a, S: Scope<'a>>(self, scope: S) -> Request<S>
where D: 'a {
// ...
}
} This allows you to get a possibly more minimally sized |
The issue happens when the Request borrows data from something else, and then you |
Oh wow you're exactly right. That really complicates things... the notion of a "must run" destructor would be incredibly helpful. So I think the right answer here, then, is to only require scoping for borrows, in which case we could require a |
That actually sounds very nice. The As for transferring ownership of buffers to the requests, have you put any thought into types that implement shared ownership with mutability, e.g., |
Yeah, exactly - it would be illegal to implement the Currently I don't implement
Not for the reason you state (my understanding is the borrow from the Rc would persist through the end of the blocking routine), but we may need to add a separate trait for the buffers passed to blocking routines in light of the addition of a |
Well, now that I think about it, we could have semantics where when a |
Though, in that case, it may be just as easy to implement |
I think this can close now that #122 has merged. Feel free to reopen. |
Just putting some notes here on some observations from using this library -
It's kind of a pain point right now that there's no MPI_Waitall equivalent in this library. It's not too bad since you can always use mpi::ffi, but the interop experience with the existing mpi::request::Request object is poor. MPI_Waitall expects a contiguous array of MPI_Request objects, but the definition of Request makes it impossible to satisfy this requirement without allocating a separate slice of MPI_Request objects, since each MPI_Request is separated by a Scope object.
The easiest and most naive option would be to just add a wait_all operation that takes a slice (or perhaps an Iterator) of mpi::request::Request objects. It would have to internally move the MPI_Request handles into a contiguous array. APIs like MPI_Waitsome or MPI_Waitany would have some additional indirection, since they must invalidate some of the MPI_Request objects that are passed in, but not all of them. You would need to pass in a
&mut [Option<mpi::request::Request>]
, and then wait_some or wait_any would need to update the slice to remove any request objects that have been completed.I don't love this option since it imposes overhead in the name of keeping the API simple, especially for any calls that are likely to be called many times on the same set of requests such as MPI_Waitsome, MPI_Waitany, MPI_Testsome, and MPI_Testany.
The second option is to add some sort of RequestCollection object. For example:
This would allow the layout of the request objects to be directly composable with existing MPI_ multiple completion operations, while still tracking the request objects via the Scope.
This type would ideally have these features:
.push(request)
operation. This allows all existing APIs to be used to construct mpi::request::Request objects that will then be added to the collection.wait_all
,wait_some
,test_some
, etc.Option<Request>
rather thanRequest
when indexing since wait_* may invalidate request objects.The text was updated successfully, but these errors were encountered: