Skip to content

Conversation

hmenke
Copy link
Member

@hmenke hmenke commented Sep 26, 2023

All tests were run with multiple nodes and different number of slots on each node:

$ cat hostfile 
pcscqm04 slots=4
pcscqm05 slots=3
pcscqm06 slots=5
pcscqm07 slots=2
$ mpirun -hostfile ./hostfile build/test/c++/mpi_window

Some ideas:

  • MPI allocator

    Similar to std::allocator implement a shared_allocator and a distributed_shared_allocator, such that one can use e.g. std::vector<double, mpi::shared_allocator<double>>

    Questions:

    • Should this be in mpi or in nda?
    • For shared memory one can probably use a best guess and use split_shared() on the default communicator, but for distributed shared memory there needs to be internode communication and that is not easily guessed from the default communicator. Maybe a global hash table with allocation information is needed?
    • For shared memory race conditions must be prevented somehow.
    • On top of that, for distributed shared memory access must be fenced and broadcasted between nodes. That's not so easy to abstract away.

@hmenke hmenke force-pushed the shm branch 3 times, most recently from 92e2e2b to 1738e29 Compare October 4, 2023 07:41
@hmenke hmenke changed the title WIP: MPI shared memory MPI shared memory Oct 9, 2023
@hmenke hmenke marked this pull request as ready for review October 9, 2023 10:00
Copy link
Member Author

@hmenke hmenke left a comment

Choose a reason for hiding this comment

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

Can we make this non-MPI compatible?

Thoemi09 and others added 7 commits October 1, 2025 16:18
- update docs
- wrap MPI calls with check_mpi_call
- remove noexcept if not necessary
- update docs
- simplify implementation
- update docs
- remove get_attr and return the stored attributes instead
- remove noexcept
- update docs
- remove noexcept when not necessary
Wentzell and others added 7 commits October 1, 2025 16:52
For consistency with mpi::window which uses comm_, rename the
communicator member variable from com_ to comm_ throughout the
mpi::communicator class.
- Change window<T>::size() to return element count for consistency
- Update test expectation accordingly

Co-Authored-By: Claude <[email protected]>
Copy link
Member Author

@hmenke hmenke left a comment

Choose a reason for hiding this comment

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

@Wentzell LGTM, only minor nitpick.

Copy link
Member

@Wentzell Wentzell left a comment

Choose a reason for hiding this comment

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

Thank you @hmenke for this massive feature upgrade! The PR was already in great condition. I have iterated once on it, and from my side it is now ready for merge. Please confirm once the two most recent changes in e45281f and c40fbee

@Wentzell
Copy link
Member

Wentzell commented Oct 3, 2025

Thank you also @Thoemi09 for your improvements

Copy link
Member Author

@hmenke hmenke left a comment

Choose a reason for hiding this comment

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

LGTM, but could you please not squash on merge? This makes it much easier to bisect later if something is wrong. You can clean up the history and rebase on unstable if you want.

@Wentzell
Copy link
Member

Wentzell commented Oct 3, 2025

LGTM, but could you please not squash on merge? This makes it much easier to bisect later if something is wrong. You can clean up the history and rebase on unstable if you want.

I would have been ok with a full squash, but if you want group things into a few commits along the way, feel free to create a suggested a grouping.

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