Remove all non-trivial view handling in Distributor #1103
Conversation
aprokop
added
performance
|
Testing failure in |
|
@masterleinad Could I ask you to review this PR? You are probably the person most familiar with the code, and may spot the stupid stuff I missed. |
aprokop
force-pushed
the
remove_all_nd_distributed
branch
2 times, most recently
from
8db61db to
29ada0e
Compare
aprokop
force-pushed
the
remove_all_nd_distributed
branch
from
29ada0e to
35bbe1d
Compare
| auto imports_comm = Kokkos::create_mirror_view(Kokkos::WithoutInitializing, | ||
| MirrorSpace{}, imports); | ||
| #else | ||
| auto exports_comm = permuted_exports; | ||
| auto imports_comm = imports; |
There was a problem hiding this comment.
So the exports_comm size differs based on ARBORX_ENABLE_GPU_AWARE_MPI?
There was a problem hiding this comment.
No, it doesn't. It's always the size of permuted_exports, which is the size of exports.
There was a problem hiding this comment.
OK. We just omit copying based on that option.
| requests.emplace_back(); | ||
| MPI_Irecv(receive_buffer_ptr, message_size, MPI_BYTE, _sources[i], 123, | ||
| _comm, &requests.back()); | ||
| } | ||
| } | ||
|
|
||
| // make sure the data in dest_buffer has been copied before sending it. | ||
| if (permutation_necessary) |
There was a problem hiding this comment.
Why do we need an extra fence now if no permutation is necessary?
There was a problem hiding this comment.
Because it is for both GPU-aware and non-GPU aware paths. For non-GPU aware, we need to make sure the (parts of) export data are finished copying to the host. I think the right check for this would be "(if gpu-aware and permutation is necessary) or (not gpu-aware)".
Edit: additionally, it may be even different worse than that. If we launched a kernel using space that fills the exports prior to calling doPostsAndWaits, and we are in the GPU-aware mode, we can't guarantee that kernel completed. So, we have to call fence even if gpu-aware and permutation is unnecessary.
|
@masterleinad Thank you for the review! |
The rationale: we only need to deal with simple 1D views internally. All the complexity was stemming from trying to handle multiple dimensions. It really is unnecessary.
This patch does the following:
sendAcrossNetworkNo longer necessary as it does the same thing as
doPostsAndWaitsdoPostsAndWaitsStraightforward handling of views
doPostsAndWaitsOnly parts that need to be sent are copied to the host (when running in non-GPU aware MPI)
doPostsAndWaitsfromtstDetailsDistributedTreeUtils.cppinto a separatetstDetailsDistributor.cpp