Skip to content
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.

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

Fix a memory leak in axom::Array copy constructor #1482

Merged
merged 5 commits into from
Dec 31, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ to use Open CASCADE's file I/O capabilities in support of Quest applications.
- Fixes compilation issue with [email protected] 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

Expand Down
6 changes: 4 additions & 2 deletions src/axom/core/Array.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -1139,7 +1139,8 @@ AXOM_HOST_DEVICE Array<T, DIM, SPACE>::Array(const Array& other)
assert(false);
#endif
#else
initialize(other.size(), other.capacity());
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;
Expand All @@ -1149,9 +1150,10 @@ AXOM_HOST_DEVICE Array<T, DIM, SPACE>::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
}

Expand Down
102 changes: 26 additions & 76 deletions src/axom/core/tests/core_array.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -2389,102 +2389,52 @@ TEST(core_array, reserve_nontrivial_reloc_um)
}
#endif

// Regression test for a memory leak in axom::Array's copy/move/initialization
template <typename T1, typename T2>
auto make_arr_data(int SZ1, int SZ2)
-> std::pair<axom::Array<axom::Array<T1>>, axom::Array<axom::Array<T2>>>
class AllocatingDefaultInit
{
using Arr1 = axom::Array<axom::Array<T1>>;
using Arr2 = axom::Array<axom::Array<T2>>;
public:
AllocatingDefaultInit() { m_value.resize(32); }

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)};
}
private:
axom::Array<int> m_value;
};

TEST(core_array, regression_array_move)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kennyweiss -- I replaced the regression test you added in #1286 with a unit test that I think should capture all the leaky cases you were seeing. Let me know if you'd rather keep the regression test as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @publixsubfan -- it looks like it covers similar/more ground, so I'm ok w/ removing the old regression case.

Did you check the bugfix against lsan and/or valgrind or a similar tool?

TEST(core_array, array_ctors_leak)
{
using T1 = int;
using Arr1 = axom::Array<axom::Array<T1>>;

using T2 = NonTriviallyRelocatable;
using Arr2 = axom::Array<axom::Array<T2>>;

using PArrArr = std::pair<Arr1, Arr2>;
using Array = axom::Array<AllocatingDefaultInit>;

// Initialize test array
constexpr int SZ1 = 20;
constexpr int SZ2 = 30;
Array data(SZ1);

// axom::Array copy constructor
{
PArrArr pr;
pr = make_arr_data<T1, T2>(SZ1, SZ2);
Array arr_copy {data};

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());
EXPECT_EQ(SZ1, arr_copy.size());
}

// axom::Array copy assignment operator
{
auto pr = make_arr_data<T1, T2>(SZ1, SZ2);

EXPECT_EQ(SZ1, pr.first.size());
EXPECT_EQ(2 * SZ1, pr.first.capacity());
Array arr_copy {SZ1 + 10};
arr_copy = data;

EXPECT_EQ(SZ2, pr.second.size());
EXPECT_EQ(SZ2, pr.second.capacity());
EXPECT_EQ(SZ1, arr_copy.size());
}

// axom::Array move constructor
{
auto pr = make_arr_data<T1, T2>(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());
Array tmp_move_from {data};
Array arr_move {std::move(tmp_move_from)};

pr = make_arr_data<T1, T2>(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());
EXPECT_EQ(SZ1, arr_move.size());
}

// lots of copy and move assignments and constructions
// axom::Array move assignment operator
{
auto pr = make_arr_data<T1, T2>(SZ1, SZ2);
pr = make_arr_data<T1, T2>(5, 7);

auto pr2 = std::move(pr);
auto pr3 = pr2;

pr2 = make_arr_data<T1, T2>(13, 17);

auto pr4 {make_arr_data<T1, T2>(33, 44)};

auto pr5(std::move(pr4));
Array tmp_move_from {data};
Array arr_move {SZ1 + 10};
arr_move = std::move(tmp_move_from);

EXPECT_EQ(33, pr5.first.size());
EXPECT_EQ(66, pr5.first.capacity());
EXPECT_EQ(44, pr5.second.size());
EXPECT_EQ(44, pr5.second.capacity());
EXPECT_EQ(SZ1, arr_move.size());
}
}