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

Replace single-use MPI datatypes with binary serialization #114

Merged
merged 3 commits into from
May 18, 2022

Conversation

fknorr
Copy link
Contributor

@fknorr fknorr commented Apr 22, 2022

So far, we are creating one custom composite MPI_Datatype for each MPI_Isend to transfer variably-sized commands and buffer subranges. For reasons not investigated further, this yields abysmal performance at least on IBM Spectrum MPI.

This PR replaces those single-use MPI datatypes with a serialization into frames, contiguous structs with a zero-sized array as the last member, a common idiom for typing a header together with variably-sized data. These are sent and received over MPI as simple byte streams. The smart pointer type unique_frame_ptr is introduced to allocate and manage frame types with arbitrary, variably-sized payloads.

As a downside, this change adds a semantically unnecessary allocation and copy of incoming buffer transfers: MPI receives data_frames, whereas the buffer manager keeps uncommitted transfers as raw_buffer_data. This might be noticeable for very large transfers. The solution is likely to get rid of the owning raw_buffer_data altogehter, but this will affect a lot of code, so it should be addressed in a follow-up PR.

Copy link

@BlackMark29A BlackMark29A left a comment

Choose a reason for hiding this comment

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

To document the offline discussions we've had, and add additional comments:

include/mpi_support.h Outdated Show resolved Hide resolved
include/buffer_transfer_manager.h Outdated Show resolved Hide resolved
include/buffer_transfer_manager.h Outdated Show resolved Hide resolved
src/buffer_transfer_manager.cc Outdated Show resolved Hide resolved
src/buffer_transfer_manager.cc Outdated Show resolved Hide resolved
src/buffer_transfer_manager.cc Show resolved Hide resolved
src/buffer_transfer_manager.cc Show resolved Hide resolved
Copy link

@BlackMark29A BlackMark29A left a comment

Choose a reason for hiding this comment

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

LGTM.

include/command.h Outdated Show resolved Hide resolved
include/mpi_support.h Outdated Show resolved Hide resolved
include/mpi_support.h Outdated Show resolved Hide resolved
include/mpi_support.h Outdated Show resolved Hide resolved
src/buffer_transfer_manager.cc Outdated Show resolved Hide resolved
include/executor.h Outdated Show resolved Hide resolved
@fknorr
Copy link
Contributor Author

fknorr commented May 10, 2022

As per offline discussion yesterday, I've decided to duplicate the dependency count inside command_frame to allow using const command_frame & without its owning unique_frame_ptr. The previous approach with "mixed queries" to unique_frame_ptr and command_frame will become worse with #101 anyway, when command frames have to transport both dependencies and conflicts.

Copy link
Member

@psalz psalz left a comment

Choose a reason for hiding this comment

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

One minor thing, otherwise LGTM!

include/mpi_support.h Show resolved Hide resolved
@fknorr fknorr merged commit c60527f into master May 18, 2022
@psalz psalz deleted the data-frames branch May 18, 2022 07:53
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.

4 participants