Skip to content

Update request pool to allow individual request operations #48

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zhao-shihan
Copy link
Contributor

Addressing #47. Changes:

  • Update request pool to allow request operations individually
  • Add check in request_pool::cancel

@rabauke rabauke self-requested a review June 15, 2025 10:16
@rabauke rabauke self-assigned this Jun 15, 2025
@@ -193,7 +226,8 @@ namespace mpl {
/// Cancels a pending request in the pool.
/// \param i index of the request for which shall be cancelled
void cancel(size_type i) {
MPI_Cancel(&requests_[i]);
if (requests_[i] != MPI_REQUEST_NULL)
MPI_Cancel(&requests_[i]);
Copy link
Contributor Author

@zhao-shihan zhao-shihan Jun 15, 2025

Choose a reason for hiding this comment

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

Actually I'm not quite sure about this. I added this check since I notice that it is not impossible for user to push an already moved request into the pool. So when I noticed that base_request::cancel (line 100) has this check, I added also here. But if there's an assumption that user will never push a null request into the pool, this check can be removed for consistency.

Copy link
Owner

Choose a reason for hiding this comment

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

I need to double-check this. But I think the check against MPI_REQUEST_NULL is actually needed here. Thanks for spotting this.

Copy link
Owner

@rabauke rabauke left a comment

Choose a reason for hiding this comment

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

I am not sure if this is actually an improvement. I can hardly imagine a situation where one wants to put a request into a request_pool and wait for its completion individually (rather than using waitall or waitany). If one needs to wait for a specific request, one should not move it into a pool.

Furthermore, the new functions are inconsistent with the existing class method. The new methods return a status object, whereas the existing one operate on status objects that are maintained by the pool but not accessible by the user.

@zhao-shihan
Copy link
Contributor Author

zhao-shihan commented Jun 16, 2025

There is a common situation where we need both individual MPI_Start/Wait and MPI_Waitsome: a master-slave scheduler. The master process/thread maintain a task queue and communicates with worker processes with persistent point-to-point send/recv. The master process uses MPI_Waitsome to wait for one or more workers finishing their current jobs and send next batch of jobs to workers by individual MPI_Start and wait for their following response by MPI_Wait.

The issue actually raise from the flexibility. If we use native MPI interface, it is natural to perform both individual operations like MPI_Start/Wait and multiple operations like MPI_Wait/Testall/some/any since we are maintaining an array of requests. But in MPL, MPI_Wait/Testall/some/any are wrapped into a request pool, and cannot be call with an request array (e.g. std::vector<prequest>), and we have to completely fallback to native MPI since there's not interoperability between MPL request and MPI request if we need both individual and multiple operations.

If we wish to keep the current pattern of request_pool, I suggest

  1. Adding functions like mpl::waitsome(<range of request>) to allow user to perform those actions. (<range of request> can be pair of random access iterators of a contiguous request array, or std::span<T> if C++20 is enabled)
  2. Improve the interoperability between MPL request and MPI request, but I'm not really sure whether it is possible.

@zhao-shihan
Copy link
Contributor Author

I think try_get_status is a more semantic naming than get_status_of.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants