From efb7f800630c5c2fed5314fcf29e334bd65dbd35 Mon Sep 17 00:00:00 2001 From: Tianyu Liu Date: Tue, 10 Sep 2024 16:00:23 -0400 Subject: [PATCH 1/5] Intentionally leak thread_local CUDA resources --- cpp/src/utilities/stream_pool.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/cpp/src/utilities/stream_pool.cpp b/cpp/src/utilities/stream_pool.cpp index 9d3a7ce5a4e..b53d6ab96b7 100644 --- a/cpp/src/utilities/stream_pool.cpp +++ b/cpp/src/utilities/stream_pool.cpp @@ -130,7 +130,6 @@ rmm::cuda_device_id get_current_cuda_device() */ struct cuda_event { cuda_event() { CUDF_CUDA_TRY(cudaEventCreateWithFlags(&e_, cudaEventDisableTiming)); } - virtual ~cuda_event() { CUDF_ASSERT_CUDA_SUCCESS(cudaEventDestroy(e_)); } operator cudaEvent_t() { return e_; } From f3fac15404f804ffae746862efc26bd91df332b8 Mon Sep 17 00:00:00 2001 From: Tianyu Liu Date: Wed, 11 Sep 2024 13:48:54 -0400 Subject: [PATCH 2/5] Improve implementation --- cpp/src/utilities/stream_pool.cpp | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-) diff --git a/cpp/src/utilities/stream_pool.cpp b/cpp/src/utilities/stream_pool.cpp index b53d6ab96b7..b9f3890a42b 100644 --- a/cpp/src/utilities/stream_pool.cpp +++ b/cpp/src/utilities/stream_pool.cpp @@ -14,6 +14,8 @@ * limitations under the License. */ +#include "driver_types.h" + #include #include #include @@ -125,18 +127,6 @@ rmm::cuda_device_id get_current_cuda_device() return rmm::cuda_device_id{device_id}; } -/** - * @brief RAII struct to wrap a cuda event and ensure its proper destruction. - */ -struct cuda_event { - cuda_event() { CUDF_CUDA_TRY(cudaEventCreateWithFlags(&e_, cudaEventDisableTiming)); } - - operator cudaEvent_t() { return e_; } - - private: - cudaEvent_t e_; -}; - /** * @brief Returns a cudaEvent_t for the current thread. * @@ -146,12 +136,13 @@ struct cuda_event { */ cudaEvent_t event_for_thread() { - thread_local std::vector> thread_events(get_num_cuda_devices()); + thread_local std::vector thread_events(get_num_cuda_devices()); auto const device_id = get_current_cuda_device(); if (not thread_events[device_id.value()]) { - thread_events[device_id.value()] = std::make_unique(); + CUDF_CUDA_TRY( + cudaEventCreateWithFlags(&thread_events[device_id.value()], cudaEventDisableTiming)); } - return *thread_events[device_id.value()]; + return thread_events[device_id.value()]; } /** From 601e83d9bbc30d340750a20d74fc366cc0f43c41 Mon Sep 17 00:00:00 2001 From: Tianyu Liu Date: Wed, 11 Sep 2024 23:34:56 -0400 Subject: [PATCH 3/5] Address reviewers comments --- cpp/src/utilities/stream_pool.cpp | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/cpp/src/utilities/stream_pool.cpp b/cpp/src/utilities/stream_pool.cpp index b9f3890a42b..91eba8335e1 100644 --- a/cpp/src/utilities/stream_pool.cpp +++ b/cpp/src/utilities/stream_pool.cpp @@ -127,6 +127,19 @@ rmm::cuda_device_id get_current_cuda_device() return rmm::cuda_device_id{device_id}; } +/** + * @brief RAII struct to wrap a cuda event and ensure its proper destruction. + */ +struct cuda_event { + cuda_event() { CUDF_CUDA_TRY(cudaEventCreateWithFlags(&e_, cudaEventDisableTiming)); } + virtual ~cuda_event() { CUDF_ASSERT_CUDA_SUCCESS(cudaEventDestroy(e_)); } + + operator cudaEvent_t() { return e_; } + + private: + cudaEvent_t e_; +}; + /** * @brief Returns a cudaEvent_t for the current thread. * @@ -136,13 +149,13 @@ rmm::cuda_device_id get_current_cuda_device() */ cudaEvent_t event_for_thread() { - thread_local std::vector thread_events(get_num_cuda_devices()); + // The program may crash if this function is called from the main thread and user application + // subsequently calls cudaDeviceReset(). + // As a workaround, here we intentionally disable RAII and leak cudaEvent_t. + thread_local std::vector thread_events(get_num_cuda_devices()); auto const device_id = get_current_cuda_device(); - if (not thread_events[device_id.value()]) { - CUDF_CUDA_TRY( - cudaEventCreateWithFlags(&thread_events[device_id.value()], cudaEventDisableTiming)); - } - return thread_events[device_id.value()]; + if (not thread_events[device_id.value()]) { thread_events[device_id.value()] = new cuda_event(); } + return *thread_events[device_id.value()]; } /** From f323335e68b8c4867a87f00a50962ebe74db3a63 Mon Sep 17 00:00:00 2001 From: Tianyu Liu Date: Wed, 11 Sep 2024 23:37:26 -0400 Subject: [PATCH 4/5] Cleanup --- cpp/src/utilities/stream_pool.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/cpp/src/utilities/stream_pool.cpp b/cpp/src/utilities/stream_pool.cpp index 91eba8335e1..039956660a0 100644 --- a/cpp/src/utilities/stream_pool.cpp +++ b/cpp/src/utilities/stream_pool.cpp @@ -14,8 +14,6 @@ * limitations under the License. */ -#include "driver_types.h" - #include #include #include From 5c280235323b05b116557ab62cba4bfae1d100c9 Mon Sep 17 00:00:00 2001 From: Tianyu Liu Date: Wed, 18 Sep 2024 11:27:17 -0400 Subject: [PATCH 5/5] Add copy and move restrictions to the event wrapper --- cpp/src/utilities/stream_pool.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/cpp/src/utilities/stream_pool.cpp b/cpp/src/utilities/stream_pool.cpp index 039956660a0..9824c472b20 100644 --- a/cpp/src/utilities/stream_pool.cpp +++ b/cpp/src/utilities/stream_pool.cpp @@ -132,6 +132,13 @@ struct cuda_event { cuda_event() { CUDF_CUDA_TRY(cudaEventCreateWithFlags(&e_, cudaEventDisableTiming)); } virtual ~cuda_event() { CUDF_ASSERT_CUDA_SUCCESS(cudaEventDestroy(e_)); } + // Moveable but not copyable. + cuda_event(const cuda_event&) = delete; + cuda_event& operator=(const cuda_event&) = delete; + + cuda_event(cuda_event&&) = default; + cuda_event& operator=(cuda_event&&) = default; + operator cudaEvent_t() { return e_; } private: