Ucx opts - Githubissues

flux-framework / dyad

DYAD: DYnamic and Asynchronous Data Streamliner

GNU Lesser General Public License v3.0

7 stars 5 forks source link

Ucx opts #67

Closed JaeseungYeom closed 7 months ago

JaeseungYeom commented 7 months ago

imported Ian's branch PR #58 from TauferLab fork as Hari does not have access.

JaeseungYeom commented 7 months ago

I tried rebasing. Something might get lost.

JaeseungYeom commented 7 months ago

I think we should breakdown this PR into multiple PRs. There are many changes. We not only have to review these for merging but also be careful not to create conflicts with outstanding PRs. Some changes are crucial. Some can be applied later. Some will be irrelevant.

DTL related changes (crucial)
There are modifications for caliper instrumentation. (later)
Some modifications are formatting and minor changes that are not really crucial. (later if relevant)
We will also drop autoconfig-based build later as there will be complicated dependencies in the near future. (irrelevant)
We will also remove dead code like urpc. (irrelevant)

It is best If a new PR can be created containing only those commits related dtl changes. Otherwise, I can give it a try to make this PR as minimal as possible for easy digestion. Let me know your opinion.

hariharan-devarajan commented 7 months ago

@JaeseungYeom we can only include optimization from your list and remove the rest that would be ideal.

We can do instrumentation stuff later as I am also working on a way to do it cleanly with DLIO profiler

JaeseungYeom commented 7 months ago

The below is the list of the changes I think need to be merged in as the first PR.

src/dyad/dtl/ucx_ep_cache.h src/dyad/dtl/ucx_ep_cache.cpp src/dyad/dtl/ucx_dtl.h src/dyad/dtl/ucx_ep_cache.h src/dyad/dtl/ucx_ep_cache.c src/dyad/dtl/flux_dtl.h src/dyad/dtl/ucx_dtl.c src/dyad/dtl/ucx_dtl.h

src/dyad/common/dyad_rc.h lines 49-52 src/dyad/core/dyad_core.c lines 405, 433, 526-528 might be a conflict, 647

src/dyad/dtl/CMakeLists.txt lines 12-13

src/dyad/dtl/dyad_dtl_impl.h lines 51-53

src/dyad/dtl/flux_dtl.c lines 18, 27, 28, 91-123, 182-186, 191, 192, 208-211

src/dyad/modules/dyad.c lines 74-77, 90-96, 206, 216, 258-262

JaeseungYeom commented 7 months ago

I think the best way now is to merge PR #65 first by @hariharan-devarajan Then, @ilumsden can create a new PR based on the latest code with the changes listed above.