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

Support for #[derive(Equivalence)] to allow Rust structs to be sent directly over MPI #52

Merged
merged 51 commits into from
Oct 27, 2020

Conversation

AndrewGaspar
Copy link
Contributor

@AndrewGaspar AndrewGaspar commented Oct 10, 2018

NOTE: This PR should be merged after #49 and #51.

This adds a crate that provides a custom derive for Equivalence, allowing a user to mark a complex struct with #[derive(Equivalence)] to be able to safely send that type via MPI without having to manually serialize the data.

E.g.

#[derive(Equivalence, Default)]
struct MyDataRust {
    b: bool,
    f: f64,
    i: u16,
}

let mut my_data = if world.rank() == root_rank {
    MyDataRust { b: true, f: 3.4, i: 7 }
} else {
    MyDataRust::default()
};

root_process.broadcast_into(&mut my_data);
// all ranks should have MyDataRust { b: true, f: 3.4, i: 7 }

This should work with any struct in Rust that is composed of fundamental types (or arrays of fundamental types) that implement Equivalence. I believe I've tested all forms of structs (with named fields, unnamed fields, and no fields).

This commit adds support for the MS-MPI implementation, primarily
targeting Windows. I add some code to discover for MS-MPI that only
run on Windows, and moved the other MPI probing to a separate probe
function for Unix.

I inject an msmpi cfg into both mpi-sys and rsmpi. This may be useful to
either surface msmpi specifically functionality or, like I do in this
commit, to disable certain features that are missing from MS-MPI.

I noticed two issues when building with the MS-MPI headers:
1. MPI_Comm_create_group was missing.
2. Bindgen does not seem to pick up the MPI_User_function definition

For both of these issues, I just removed the APIs that called these
functions when using MS-MPI. This may not be the best method.

I also disabled portions of any tests that rely on these
functionalities.

I recommend building without default features on Windows since libffi is
not available by default.

I also swapped the gcc crate with cc, as that seems to be
the stable version of gcc.
Currently only supports simple structures of separate, scalar fields
- Adds dup() for all implementations of Datatype trait
- Adds as_ref() to get a DatatypeRef from any Datatype
- Adds example of dup()
- Adds new types: UncommittedUserDatatype + UncommittedDatatypeRef
  - same constructors as UserDatatype and DatatypeRef
  - UncommittedUserDatatype::commit consumes the old value to produce a
    UserDatatype
  - structured constructor takes a slice that can be passed directly to
    underlying MPI_Type_create_struct call without copy
- Adds UncommittedDatatype trait
  - Implemented for UncommittedUserDatatype, UncommittedDatatypeRef,
    UserDatatype, and DatatypeRef
  - Supports capability for duplicating the datatype
- New generic trait: FromRaw
  - Provides fixed way for constructing an mpi::Foo from a MPI_Foo
  - Implemented for all the Datatype types
- New generic trait: MatchesRaw
  - Marker trait that indicates it's representationally safe to treat
    a slice of Foo as a slice of Foo::Raw
  - Implemented for all of the Datatype types
- Adds a complex_datatype test that constructs a UserDatatype from
  multiple UncommittedUserDatatype values
-
This also marks it as unsafe since, with some implementations, it may be
possible to create an mpi::ffi::MPI_Comm safely that is invalid.
@benruijl
Copy link

benruijl commented Sep 8, 2020

This is great feature that I'd love to use right now! Is there any timeline for when this can be merged?

@AndrewGaspar
Copy link
Contributor Author

Oh, I was originally hoping for a review... I should probably just update it and merge it.

@fzyzcjy
Copy link

fzyzcjy commented Sep 23, 2020

Wow this is really helpful and can reduce boilerplate! Looking forward to seeing it merged :)

@AndrewGaspar
Copy link
Contributor Author

Haha, ok, I've been convinced, let me get this done...

@AndrewGaspar
Copy link
Contributor Author

I was working on this last night - mainly the issue is that the diff was a little gnarly. However, I did run into another issue of significance - the Equivalence derive requires doing a bunch of "offset_of"s to get the offset of each field in the struct. The problem with this is that it requires UB to do (and was also using the deprecated mem::uninitialized, but switching to MaybeUninit doesn't fix the UB).

There's discussion of this issue in the memoffset crate (which we don't use, but we accomplish the task in a similar way): Gilnaa/memoffset#24

Of note, there's an implicit promise that even though the code in memoffset is technically UB, the compiler team will ensure that the code continues to work as expected: Gilnaa/memoffset#24 (comment)

So there are two options:

  • Take Ralf at his word that this pattern won't be broken by the compiler ("defined" UB)
  • Just require the user to #[derive(Default)], which would eliminate all UB (we can construct a default object to get the field offsets)

Any thoughts?

@adamreichold
Copy link
Contributor

Any thoughts?

I would suggest deferring to the memoffset crate as a community-wide solution should eventually materialise there. Until it does we will have working code without a future maintenance burden. It does certainly seem worth the dependency...

@AndrewGaspar AndrewGaspar changed the title Support for #[derive(Equivalence)] to allow Rust structs to be sent directly over MPI WIP: Support for #[derive(Equivalence)] to allow Rust structs to be sent directly over MPI Oct 22, 2020
@AndrewGaspar
Copy link
Contributor Author

Can't be merged until Gilnaa/memoffset#48 is merged and a new release of memoffset is cut.

- Initializes datatype for derive(Equivalence) once per-process
- Fixes a possible race condition in mpi::initialize allowing for
  multiple threads to initialize MPI
- mpi::initialize now keeps bookkeeping on which thread initialized MPI
- equivalent_datatype panics for derive(Equivalence) if the MPI library
  isn't currently initialized
- Add tests to check that the code panics when expected
mpi-derive/src/lib.rs Outdated Show resolved Hide resolved
mpi-derive/src/lib.rs Outdated Show resolved Hide resolved
@AndrewGaspar AndrewGaspar changed the title WIP: Support for #[derive(Equivalence)] to allow Rust structs to be sent directly over MPI Support for #[derive(Equivalence)] to allow Rust structs to be sent directly over MPI Oct 24, 2020
@AndrewGaspar
Copy link
Contributor Author

@adamreichold Look good?

@adamreichold
Copy link
Contributor

@adamreichold Look good?

I admittedly did not have time yet to look over your latest changes. Could you give me until the middle of the week? I think I will be able to schedule a more comprehensive review until then.

@adamreichold
Copy link
Contributor

@adamreichold Look good?

@AndrewGaspar Besides the cosmetic question of whether to use examples or tests, this looks good to me now.

@AndrewGaspar AndrewGaspar merged commit 79cb08c into rsmpi:master Oct 27, 2020
@AndrewGaspar AndrewGaspar deleted the mpi-datatype-derive branch October 27, 2020 01:37
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

4 participants