Locality-Aware Neighbor Updates by bienz2 · Pull Request #63 · mpi-advance/locality_aware

bienz2 · 2026-03-05T17:52:29Z

Large refactor of locality-aware neighbor collectives:

Changed MPIL_Request object to hold pointers to MPIL_Requests for local_L, local_S, and local_R comm. This simplified the neighbor_start/neighbor_wait routines.
Since MPIL_Request objects are now used, I was able to replace calls to init_communication with neighbor_alltoallv_init_standard. I deleted the file containing init_communication routines.
I replaced dynamic communication throughout neighbor_locality.cpp with calls to MPIL_Alltoall_crs and alltoallv_crs_personalized and combined calls when possible to reduce communication overheads of setting up locality neighborhood collectives.
I cleaned up code, removing all locality methods from the communication directory. CommData structs are directly within neighbor_locality.cpp now since they are not used elsewhere.

Fix syntax for date command in traffic workflow

Added logic to fetch and output daily clone traffic data.

Run at midnight is breaking; run at 6am each day instead.

…munication

… to remove object entirely...

…nly on node

bienz2 · 2026-03-05T17:57:45Z

This also includes a bug fix. The suitesparse neighbor alltoallv init test was missing :
MPIL_Start(xrequest);
MPIL_Wait(xrequest, &status);
MPIL_Request_free(&xrequest);

Which caused an error (creating a persistent request) without freeing it. Tests were still passing, but if you ran the test individually, you received an error statement from MPICH.

bienz2 · 2026-03-05T17:59:01Z

Actually, there are two bug fixes here. The second was a missing MPI_Waitall in one implementation of alltoall_crs.

TheMasterDirk

I leave the conceptual validation of the code to you, as I just skimmed that part (and it seemed ok to me).

Most of my requested changes are just consistency fixes and/or keeping things simpler. Let me know if there are any issues, and feel free to reject any of my suggestions (just let me know if you do).

TheMasterDirk · 2026-03-24T14:23:52Z

+int main(int argc, char* argv[])
+{
+    MPI_Init(&argc, &argv);
+


Should we add an MPIL_Init here as well? I know this may not use what it sets up, but if we ever add more to the init function, might be a good idea

TheMasterDirk · 2026-03-24T14:27:43Z

+    int* counts;
+    int* indices;
+    char* buffer;
+} CommData;


Since the struct is still around, would you mind this setup:

Keep comm_data.h and comm_data.cpp but

Move header to "library/include/neighborhood"

Move source file to "library/source/neighborhood" (can sit just in that folder and not any subfolder) -- but still add it to CMakeLists in this folder

Delete the struct here, and comm-data specific methods here.

That way we can keep the struct in its own header (so it's easier to find where it is), and we get to keep the documentation we made?

TheMasterDirk · 2026-03-24T14:31:27Z


-void map_procs_to_nodes(LocalityComm* locality,
-                        const int orig_num_msgs,
+#ifdef __cplusplus


The CPP header guards are only necessary around the bindings (any file that has a MPIL_Foo function) Any other header/file inside the library can skip them since the main locality_aware.h header doesn't use these files.

If there are issues with undefined references at linking when removing this, lmk and I can help trace it down.

TheMasterDirk · 2026-03-24T14:39:23Z

 extern "C" {
 #endif

+typedef struct _MPIL_Request MPIL_Request;


Was this necessary to avoid some compilation error?

If the compiler was unhappy about MPIL_Request* foo inside the struct, I believe you can just use _MPIL_Request* foo instead. I only ask because Andrew and I had a few headaches on resolving naming issues upstream when we did it like this, so if possible, I want to try and keep it the same.

TheMasterDirk · 2026-03-24T14:45:17Z

-	locality_comm.cpp
 	topology_functions.cpp
-)
+	get_tag.cpp


Can I get the files in alphabetical order here? :)
Just personal preference

TheMasterDirk · 2026-03-24T15:06:36Z

+    // Get MPI Information
+    int rank, num_procs;
+    MPI_Comm_rank(mpil_comm->global_comm, &rank);
+    MPI_Comm_size(mpil_comm->global_comm, &num_procs);


Suggested change

// Get MPI Information

int rank, num_procs;

MPI_Comm_rank(mpil_comm->global_comm, &rank);

MPI_Comm_size(mpil_comm->global_comm, &num_procs);

These aren't used in this function, might as well delete them until needed.

TheMasterDirk · 2026-03-24T15:08:02Z

-    // Initialize final variable (MPI_Request arrays, etc.)
-    finalize_locality_comm(locality_comm);
+    // Don't need local_S or global recv indices (just contiguous)
+    if (local_S_recv_data->indices)


Suggested change

if (local_S_recv_data->indices)

if (local_S_recv_data->indices != NULL)

TheMasterDirk · 2026-03-24T15:08:16Z

+        free(local_S_recv_data->indices);
+        local_S_recv_data->indices = NULL;
+    }
+    if (global_recv_data->indices)


Suggested change

if (global_recv_data->indices)

if (global_recv_data->indices != NULL)

TheMasterDirk · 2026-03-24T15:11:38Z

+    if (data->procs)
+    {
+        free(data->procs);
+    }
+    if (data->indptr)
+    {
+        free(data->indptr);
+    }
+    if (data->counts)
+    {
+        free(data->counts);
+    }
+    if (data->indices)
+    {
+        free(data->indices);
+    }
+    if (data->buffer)
+    {
+        free(data->buffer);
+    }

-    // Don't need local_S or global recv indices (just contiguous)
-    free(locality->local_S_comm->recv_data->indices);
-    locality->local_S_comm->recv_data->indices = NULL;
-    free(locality->global_comm->recv_data->indices);
-    locality->global_comm->recv_data->indices = NULL;
+    free(data);


For consistency, can these all get:

!= NULL add to if statements

Variables set to NULL after freeing

TheMasterDirk · 2026-03-24T15:12:25Z

+    // Initialize send side for dynamic communication
+    std::vector<int> dest(global_send_data->num_msgs);
+    std::vector<int> vals(global_send_data->num_msgs, mpil_comm->rank_node);
+    for (int i = 0; i < global_send_data->num_msgs; i++)


Optional: Add "{}" to all the one line for loops.

bienz2 and others added 30 commits February 4, 2026 14:22

Fix syntax for date command in traffic workflow

fdd03a7

Merge pull request mpi-advance#58 from mpi-advance/bienz2-patch-1

78cd87d

Fix syntax for date command in traffic workflow

Fetch daily clone count and unique clones

930cda3

Added logic to fetch and output daily clone traffic data.

Change cron schedule to run at 06:00 daily

83964b0

Run at midnight is breaking; run at 6am each day instead.

Delete .github/workflows/traffic.yml

3080489

Delete .github/metrics directory

015711d

adding reorder code

8f7039d

Adding spmv init code

05de4d5

Updating to non-persistent

74ae0db

using probe

d612b6a

removed nan from check

7863415

Restructuring requests, cleaner

43f6b67

Added missing waitall to alltoall_crs

e273912

Fixed bug in suitesparse neighbor alltoallv init test

fdcb9c3

Working to remove locality object

49915e3

Fixing bugs

cdedaa0

Fixing bugs

ee214a2

Working code, requests no longer use locality object

10b9d38

Removed locality struct from Request structure

ffa1ab2

Progress towards using Topo+NeighborAlltoallvInit instead of init_com…

d43866e

…munication

Fixing bugs

7cf33a6

Fixed bug (sends/recvs were reverse in Topo)

3183967

Moved all calls to locality object into neighor_locality.cpp. Working…

be10c30

… to remove object entirely...

removed init_communication method

07f2c6d

removed locality/commpkg classes

ea863a3

Replaced some dynamic communication with MPIL_Alltoallv_crs

97fb9fe

Switched dynamic communication to use MPI_Alltoall_crs

3dd3032

Combined two allreduces, hardcoded to personalized alltoallv_crs as o…

8c7ce85

…nly on node

Removed unused tag variables

8a3ce0e

merging from develop

7642cb0

TheMasterDirk requested changes Mar 24, 2026

View reviewed changes

	if (local_S_recv_data->indices)
	if (local_S_recv_data->indices != NULL)

	if (global_recv_data->indices)
	if (global_recv_data->indices != NULL)

Conversation

bienz2 commented Mar 5, 2026

Uh oh!

bienz2 commented Mar 5, 2026

Uh oh!

bienz2 commented Mar 5, 2026

Uh oh!

TheMasterDirk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants