From 66e225d796e39c86e5577cc3a305a8a3bdecf6ec Mon Sep 17 00:00:00 2001 From: Max Yang Date: Thu, 26 Dec 2024 14:50:18 -0800 Subject: [PATCH 1/5] Fix axom::Array copy ctor leak --- src/axom/core/Array.hpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/axom/core/Array.hpp b/src/axom/core/Array.hpp index 01afc96fda..3e90e54289 100644 --- a/src/axom/core/Array.hpp +++ b/src/axom/core/Array.hpp @@ -1127,6 +1127,7 @@ AXOM_HOST_DEVICE Array::Array(const Array& other) : ArrayBase>( static_cast>&>(other)) , m_allocator_id(other.m_allocator_id) + , m_executeOnGPU(axom::isDeviceAllocator(m_allocator_id)) { #if defined(AXOM_DEVICE_CODE) #if defined(AXOM_DEBUG) @@ -1139,7 +1140,7 @@ AXOM_HOST_DEVICE Array::Array(const Array& other) assert(false); #endif #else - initialize(other.size(), other.capacity()); + this->setCapacity(other.capacity()); // Use fill_range to ensure that copy constructors are invoked for each // element. MemorySpace srcSpace = SPACE; @@ -1149,9 +1150,10 @@ AXOM_HOST_DEVICE Array::Array(const Array& other) } OpHelper {m_allocator_id, m_executeOnGPU}.fill_range(m_data, 0, - m_num_elements, + other.size(), other.data(), srcSpace); + this->updateNumElements(other.size()); #endif } From 0a67bf834e960acb4bb6fbaa4992328fd6a1b96a Mon Sep 17 00:00:00 2001 From: Max Yang Date: Thu, 26 Dec 2024 14:58:24 -0800 Subject: [PATCH 2/5] Add a unit test to capture leaky ctor behavior --- src/axom/core/tests/core_array.hpp | 50 ++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/axom/core/tests/core_array.hpp b/src/axom/core/tests/core_array.hpp index b7cacc83f5..e0bd18944e 100644 --- a/src/axom/core/tests/core_array.hpp +++ b/src/axom/core/tests/core_array.hpp @@ -2488,3 +2488,53 @@ TEST(core_array, regression_array_move) EXPECT_EQ(44, pr5.second.capacity()); } } + +class AllocatingDefaultInit +{ +public: + AllocatingDefaultInit() { m_value.resize(32); } + +private: + axom::Array m_value; +}; + +TEST(core_array, array_ctors_leak) +{ + using Array = axom::Array; + + // Initialize test array + constexpr int SZ1 = 20; + Array data(SZ1); + + // axom::Array copy constructor + { + Array arr_copy {data}; + + EXPECT_EQ(SZ1, arr_copy.size()); + } + + // axom::Array copy assignment operator + { + Array arr_copy {SZ1 + 10}; + arr_copy = data; + + EXPECT_EQ(SZ1, arr_copy.size()); + } + + // axom::Array move constructor + { + Array tmp_move_from {data}; + Array arr_move {std::move(tmp_move_from)}; + + EXPECT_EQ(SZ1, arr_move.size()); + } + + // axom::Array move assignment operator + { + Array tmp_move_from {data}; + Array arr_move {SZ1 + 10}; + arr_move = std::move(tmp_move_from); + + EXPECT_EQ(SZ1, arr_move.size()); + } +} From 57584b15ee1320f323b7ddc40a94cac3b90638f1 Mon Sep 17 00:00:00 2001 From: Max Yang Date: Thu, 26 Dec 2024 15:26:10 -0800 Subject: [PATCH 3/5] Remove regression test --- src/axom/core/tests/core_array.hpp | 100 ----------------------------- 1 file changed, 100 deletions(-) diff --git a/src/axom/core/tests/core_array.hpp b/src/axom/core/tests/core_array.hpp index e0bd18944e..d5db12bda4 100644 --- a/src/axom/core/tests/core_array.hpp +++ b/src/axom/core/tests/core_array.hpp @@ -2389,106 +2389,6 @@ TEST(core_array, reserve_nontrivial_reloc_um) } #endif -// Regression test for a memory leak in axom::Array's copy/move/initialization -template -auto make_arr_data(int SZ1, int SZ2) - -> std::pair>, axom::Array>> -{ - using Arr1 = axom::Array>; - using Arr2 = axom::Array>; - - T1 val1 {}; - T2 val2 {}; - - Arr1 arr1(SZ1, 2 * SZ1); - for(int i = 0; i < SZ1; ++i) - { - arr1[i].resize(i + 10); - arr1[i].shrink(); - arr1[i].push_back(val1); - } - - Arr2 arr2(SZ2, 2 * SZ2); - for(int i = 0; i < SZ2; ++i) - { - arr2[i].resize(i + 10); - arr2[i].shrink(); - arr2[i].push_back(val2); - } - arr2.shrink(); - - return {std::move(arr1), std::move(arr2)}; -} - -TEST(core_array, regression_array_move) -{ - using T1 = int; - using Arr1 = axom::Array>; - - using T2 = NonTriviallyRelocatable; - using Arr2 = axom::Array>; - - using PArrArr = std::pair; - - constexpr int SZ1 = 20; - constexpr int SZ2 = 30; - - { - PArrArr pr; - pr = make_arr_data(SZ1, SZ2); - - EXPECT_EQ(SZ1, pr.first.size()); - EXPECT_EQ(2 * SZ1, pr.first.capacity()); - - EXPECT_EQ(SZ2, pr.second.size()); - EXPECT_EQ(SZ2, pr.second.capacity()); - } - - { - auto pr = make_arr_data(SZ1, SZ2); - - EXPECT_EQ(SZ1, pr.first.size()); - EXPECT_EQ(2 * SZ1, pr.first.capacity()); - - EXPECT_EQ(SZ2, pr.second.size()); - EXPECT_EQ(SZ2, pr.second.capacity()); - } - - { - auto pr = make_arr_data(SZ1, SZ2); - EXPECT_EQ(SZ1, pr.first.size()); - EXPECT_EQ(2 * SZ1, pr.first.capacity()); - EXPECT_EQ(SZ2, pr.second.size()); - EXPECT_EQ(SZ2, pr.second.capacity()); - - pr = make_arr_data(SZ2, SZ1); - EXPECT_EQ(SZ2, pr.first.size()); - EXPECT_EQ(2 * SZ2, pr.first.capacity()); - EXPECT_EQ(SZ1, pr.second.size()); - EXPECT_EQ(SZ1, pr.second.capacity()); - } - - // lots of copy and move assignments and constructions - { - auto pr = make_arr_data(SZ1, SZ2); - pr = make_arr_data(5, 7); - - auto pr2 = std::move(pr); - auto pr3 = pr2; - - pr2 = make_arr_data(13, 17); - - auto pr4 {make_arr_data(33, 44)}; - - auto pr5(std::move(pr4)); - - EXPECT_EQ(33, pr5.first.size()); - EXPECT_EQ(66, pr5.first.capacity()); - EXPECT_EQ(44, pr5.second.size()); - EXPECT_EQ(44, pr5.second.capacity()); - } -} - class AllocatingDefaultInit { public: From bc0c76f7df2b21eefb548a5e3bde16c83df64a0b Mon Sep 17 00:00:00 2001 From: Max Yang Date: Fri, 27 Dec 2024 13:18:24 -0800 Subject: [PATCH 4/5] Move call to axom::isDeviceAllocator into host-only section --- src/axom/core/Array.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/axom/core/Array.hpp b/src/axom/core/Array.hpp index 3e90e54289..a30b845814 100644 --- a/src/axom/core/Array.hpp +++ b/src/axom/core/Array.hpp @@ -1127,7 +1127,6 @@ AXOM_HOST_DEVICE Array::Array(const Array& other) : ArrayBase>( static_cast>&>(other)) , m_allocator_id(other.m_allocator_id) - , m_executeOnGPU(axom::isDeviceAllocator(m_allocator_id)) { #if defined(AXOM_DEVICE_CODE) #if defined(AXOM_DEBUG) @@ -1141,6 +1140,7 @@ AXOM_HOST_DEVICE Array::Array(const Array& other) #endif #else this->setCapacity(other.capacity()); + m_executeOnGPU = axom::isDeviceAllocator(m_allocator_id); // Use fill_range to ensure that copy constructors are invoked for each // element. MemorySpace srcSpace = SPACE; From c9101fe36865c38090b58df85e78288a1ae1ecfc Mon Sep 17 00:00:00 2001 From: Max Yang Date: Mon, 30 Dec 2024 11:26:38 -0800 Subject: [PATCH 5/5] Update RELEASE-NOTES --- RELEASE-NOTES.md | 1 + 1 file changed, 1 insertion(+) diff --git a/RELEASE-NOTES.md b/RELEASE-NOTES.md index 938309dcb3..19e1e02fa8 100644 --- a/RELEASE-NOTES.md +++ b/RELEASE-NOTES.md @@ -43,6 +43,7 @@ to use Open CASCADE's file I/O capabilities in support of Quest applications. - Fixes compilation issue with RAJA@2024.07 on 32-bit Windows configurations. This required a [RAJA fix to avoid 64-bit intrinsics](https://github.com/LLNL/RAJA/pull/1746), as well as support for 32-bit `Word`s in Slam's `BitSet` class. +- Fixes a memory leak in `axom::Array` copy constructor. ## [Version 0.10.1] - Release date 2024-10-22