Skip to content

Optimizes the Margo DTL's memory usage by matching the behavior of the UCX DTL #178

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.

Sign up for GitHub

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
ilumsden wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Optimizes the Margo DTL's memory usage by matching the behavior of the UCX DTL #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d0cbbc0
Update Margo DTL to avoid memory leaks and massive performance limita…
ilumsden Jun 5, 2026
8b50a60
Add a pointer check around flux_msg_handler_delvec to avoid potential…
ilumsden Jun 5, 2026
e737b89
Eliminate a redundant goto/jump
ilumsden Jun 5, 2026
2c70594
Make the max transfer size for the DTL backends configurable at build…
ilumsden Jun 5, 2026
c27ed68
Fix a couple of minor syntax mistakes
ilumsden Jun 5, 2026
b75acd5
Fix small bug where there was a missing variable declaration
ilumsden Jun 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ if (DYAD_ENABLE_MARGO_DATA)
set (DYAD_ENABLE_MARGO_DTL 1)
endif()


set(DYAD_DTL_MAX_TRANSFER_SIZE "4294967296" CACHE STRING "Maximum transfer size supported by DYAD in bytes")

@JaeseungYeom JaeseungYeom Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We want this be rather runtime configurable.
CMake can set the deafult value.
DYAD_DTL_MAX_TX_SIZE_DEFAULT, which should be written to dyad_config.h
This should be commonly used for both UCX and MARGO, client and server.

@JaeseungYeom JaeseungYeom Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This one is from shuffle test.
bool parse_size (const char* str, size_t& out)
{
char* end;
long long val = strtoll (str, &end, 10);
if (val <= 0 || end == str)
return false;

switch (*end) {
    case 'K':
    case 'k':
        out = static_cast<size_t> (val) * 1024ULL;
        break;
    case 'M':
    case 'm':
        out = static_cast<size_t> (val) * 1024ULL * 1024ULL;
        break;
    case 'G':
    case 'g':
        out = static_cast<size_t> (val) * 1024ULL * 1024ULL * 1024ULL;
        break;
    case '\0':
        out = static_cast<size_t> (val);
        break;
    default:
        return false;  // unrecognized suffix
}
return true;

}


set(DYAD_PROFILER "NONE" CACHE STRING "Profiler to use for DYAD")
set_property(CACHE DYAD_PROFILER PROPERTY STRINGS PERFFLOW_ASPECT CALIPER DFTRACER NONE)
Expand Down
4 changes: 2 additions & 2 deletions src/dyad/core/dyad_ctx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,11 @@ DYAD_DLL_EXPORTED void dyad_ctx_fini (void)
// pydyad closes dyad by calling dyad_finalize ()
DYAD_C_FUNCTION_START ();
if (ctx == NULL) {
goto dyad_wrapper_fini_done;
DYAD_C_FUNCTION_END ();
return;
}
DYAD_C_FUNCTION_END ();
dyad_finalize ();
dyad_wrapper_fini_done:;
}

dyad_rc_t dyad_clear (void);
Expand Down
16 changes: 14 additions & 2 deletions src/dyad/dtl/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,12 @@ set(FLUX_PRIVATE_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/flux_dtl.h)
set(FLUX_PUBLIC_HEADERS)

# UCX implementation for DTL
set(UCX_DTL_SRC ${CMAKE_CURRENT_SOURCE_DIR}/ucx_dtl.c ${CMAKE_CURRENT_SOURCE_DIR}/ucx_ep_cache.cpp)
set(UCX_DTL_SRC ${CMAKE_CURRENT_BINARY_DIR}/ucx_dtl.c ${CMAKE_CURRENT_SOURCE_DIR}/ucx_ep_cache.cpp)

@JaeseungYeom JaeseungYeom Jun 5, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, MAX_TRANSFER_SIZE as an env variable.
And parse size characters at the end.
We wouldn't need to change CMakeLists.txt and make it needlessly static.

set(UCX_PRIVATE_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/ucx_dtl.h ${CMAKE_CURRENT_SOURCE_DIR}/ucx_ep_cache.h)
set(UCX_PUBLIC_HEADERS)

# Margo implementation for DTL
set(MARGO_DTL_SRC ${CMAKE_CURRENT_SOURCE_DIR}/margo_dtl.c)
set(MARGO_DTL_SRC ${CMAKE_CURRENT_BINARY_DIR}/margo_dtl.c)
set(MARGO_PRIVATE_HEADERS ${CMAKE_CURRENT_SOURCE_DIR}/margo_dtl.h)
set(MARGO_PUBLIC_HEADERS)

Expand All @@ -28,13 +28,25 @@ list(APPEND DTL_PUBLIC_HEADERS ${FLUX_PUBLIC_HEADERS})

# UCX: compile-time selection
if(DYAD_ENABLE_UCX_DTL)
# Use configure_file to set UCX_MAX_TRANSFER_SIZE
configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/ucx_dtl.c
${CMAKE_CURRENT_BINARY_DIR}/ucx_dtl.c
@ONLY
)
list(APPEND DTL_SRC ${UCX_DTL_SRC})
list(APPEND DTL_PRIVATE_HEADERS ${UCX_PRIVATE_HEADERS})
list(APPEND DTL_PUBLIC_HEADERS ${UCX_PUBLIC_HEADERS})
endif()

# Margo: compile-time selection
if(DYAD_ENABLE_MARGO_DTL)
# Use configure_file to set MARGO_MAX_TRANSFER_SIZE
configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/margo_dtl.c
${CMAKE_CURRENT_BINARY_DIR}/margo_dtl.c
@ONLY
)
list(APPEND DTL_SRC ${MARGO_DTL_SRC})
list(APPEND DTL_PRIVATE_HEADERS ${MARGO_PRIVATE_HEADERS})
list(APPEND DTL_PUBLIC_HEADERS ${MARGO_PUBLIC_HEADERS})
Expand Down
Loading
Loading