Skip to content

Fix the view memory leak and array/view copy/move. #485

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

Merged
tcojean merged 2 commits into from
Apr 2, 2020
Merged

Fix the view memory leak and array/view copy/move. #485

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
673e71f
Fix the view memory leak and array/view copy/move.
Mar 23, 2020
ad9802b
Fix issues pointed by reviews.
Mar 25, 2020
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
213 changes: 211 additions & 2 deletions core/test/base/array.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
#include <ginkgo/core/base/array.hpp>


#include <algorithm>


#include <gtest/gtest.h>


Expand Down Expand Up @@ -284,6 +287,46 @@ TYPED_TEST(Array, CanBeResized)
}


TYPED_TEST(Array, ViewCannotBeResized)
{
TypeParam data[] = {1, 2, 3};
auto view = gko::Array<TypeParam>::view(this->exec, 3, data);

EXPECT_THROW(view.resize_and_reset(1), gko::NotSupported);
EXPECT_EQ(view.get_num_elems(), 3);
ASSERT_EQ(view.get_data()[0], TypeParam{1});
}


template <typename T>
class my_null_deleter {
public:
using pointer = T *;

void operator()(pointer) const noexcept {}
};

template <typename T>
class my_null_deleter<T[]> {
public:
using pointer = T[];

void operator()(pointer) const noexcept {}
};


TYPED_TEST(Array, CustomDeleterCannotBeResized)
{
TypeParam data[] = {1, 2, 3};
auto view_custom_deleter = gko::Array<TypeParam>(
this->exec, 3, data, my_null_deleter<TypeParam[]>{});

EXPECT_THROW(view_custom_deleter.resize_and_reset(1), gko::NotSupported);
EXPECT_EQ(view_custom_deleter.get_num_elems(), 3);
ASSERT_EQ(view_custom_deleter.get_data()[0], TypeParam{1});
}


TYPED_TEST(Array, CanBeAssignedAnExecutor)
{
gko::Array<TypeParam> a;
Expand All @@ -304,16 +347,182 @@ TYPED_TEST(Array, ChangesExecutors)
}


TYPED_TEST(Array, CanCreateView)
TYPED_TEST(Array, ViewModifiesOriginalData)
{
TypeParam data[] = {1, 2, 3};
auto view = gko::Array<TypeParam>::view(this->exec, 3, data);

TypeParam new_data[] = {5, 4, 2};
std::copy(new_data, new_data + 3, view.get_data());
Comment thread
thoasm marked this conversation as resolved.

EXPECT_EQ(data[0], TypeParam{5});
EXPECT_EQ(data[1], TypeParam{4});
EXPECT_EQ(data[2], TypeParam{2});
ASSERT_EQ(view.get_num_elems(), 3);
}


TYPED_TEST(Array, CopyArrayToArray)
{
gko::Array<TypeParam> array(this->exec, {1, 2, 3});
gko::Array<TypeParam> array2(this->exec, {5, 4, 2, 1});

array = array2;

EXPECT_EQ(array.get_data()[0], TypeParam{5});
EXPECT_EQ(array.get_data()[1], TypeParam{4});
EXPECT_EQ(array.get_data()[2], TypeParam{2});
EXPECT_EQ(array.get_data()[3], TypeParam{1});
EXPECT_EQ(array.get_num_elems(), 4);
EXPECT_NE(array.get_data(), array2.get_data());
ASSERT_EQ(array2.get_num_elems(), 4);
Comment thread
tcojean marked this conversation as resolved.
}


TYPED_TEST(Array, CopyViewToView)
{
TypeParam data[] = {1, 2, 3};
auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
view = gko::Array<TypeParam>{this->exec, {5, 4, 2}};
TypeParam data2[] = {5, 4, 2};
auto view2 = gko::Array<TypeParam>::view(this->exec, 3, data2);
TypeParam data_size4[] = {5, 4, 2, 1};
auto view_size4 = gko::Array<TypeParam>::view(this->exec, 4, data_size4);

view = view2;
view2.get_data()[0] = 2;
Comment thread
thoasm marked this conversation as resolved.

EXPECT_EQ(data[0], TypeParam{5});
EXPECT_EQ(data[1], TypeParam{4});
EXPECT_EQ(data[2], TypeParam{2});
EXPECT_EQ(view.get_num_elems(), 3);
EXPECT_EQ(view2.get_num_elems(), 3);
EXPECT_EQ(view2.get_data()[0], TypeParam{2});
ASSERT_THROW(view2 = view_size4, gko::OutOfBoundsError);
}


TYPED_TEST(Array, CopyViewToArray)
{
TypeParam data[] = {1, 2, 3, 4};
auto view = gko::Array<TypeParam>::view(this->exec, 4, data);
gko::Array<TypeParam> array(this->exec, {5, 4, 2});

array = view;
view.get_data()[0] = 2;

EXPECT_EQ(array.get_data()[0], TypeParam{1});
EXPECT_EQ(array.get_data()[1], TypeParam{2});
EXPECT_EQ(array.get_data()[2], TypeParam{3});
EXPECT_EQ(array.get_data()[3], TypeParam{4});
EXPECT_EQ(array.get_num_elems(), 4);
ASSERT_EQ(view.get_num_elems(), 4);
}


TYPED_TEST(Array, CopyArrayToView)
{
TypeParam data[] = {1, 2, 3};
auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
gko::Array<TypeParam> array_size2(this->exec, {5, 4});
gko::Array<TypeParam> array_size4(this->exec, {5, 4, 2, 1});

view = array_size2;

EXPECT_EQ(data[0], TypeParam{5});
EXPECT_EQ(data[1], TypeParam{4});
EXPECT_EQ(data[2], TypeParam{3});
EXPECT_EQ(view.get_num_elems(), 3);
EXPECT_EQ(array_size2.get_num_elems(), 2);
ASSERT_THROW(view = array_size4, gko::OutOfBoundsError);
}


TYPED_TEST(Array, MoveArrayToArray)
{
gko::Array<TypeParam> array(this->exec, {1, 2, 3});
gko::Array<TypeParam> array2(this->exec, {5, 4, 2, 1});
auto data2 = array2.get_data();

array = std::move(array2);
Comment thread
thoasm marked this conversation as resolved.

EXPECT_EQ(array.get_data(), data2);
EXPECT_EQ(array.get_data()[0], TypeParam{5});
EXPECT_EQ(array.get_data()[1], TypeParam{4});
EXPECT_EQ(array.get_data()[2], TypeParam{2});
EXPECT_EQ(array.get_data()[3], TypeParam{1});
EXPECT_EQ(array.get_num_elems(), 4);
EXPECT_EQ(array2.get_data(), nullptr);
ASSERT_EQ(array2.get_num_elems(), 0);
}


TYPED_TEST(Array, MoveViewToView)
{
TypeParam data[] = {1, 2, 3, 4};
auto view = gko::Array<TypeParam>::view(this->exec, 4, data);
TypeParam data2[] = {5, 4, 2};
auto view2 = gko::Array<TypeParam>::view(this->exec, 3, data2);

view = std::move(view2);
Comment thread
pratikvn marked this conversation as resolved.

EXPECT_EQ(view.get_data(), data2);
EXPECT_EQ(view.get_data()[0], TypeParam{5});
EXPECT_EQ(view.get_data()[1], TypeParam{4});
EXPECT_EQ(view.get_data()[2], TypeParam{2});
EXPECT_EQ(view.get_num_elems(), 3);
EXPECT_EQ(view2.get_data(), nullptr);
EXPECT_EQ(view2.get_num_elems(), 0);
EXPECT_NE(data, nullptr);
EXPECT_EQ(data[0], TypeParam{1});
EXPECT_EQ(data[1], TypeParam{2});
EXPECT_EQ(data[2], TypeParam{3});
ASSERT_EQ(data[3], TypeParam{4});
}


TYPED_TEST(Array, MoveViewToArray)
{
TypeParam data[] = {1, 2, 3, 4};
gko::Array<TypeParam> array(this->exec, {5, 4, 2});
auto view = gko::Array<TypeParam>::view(this->exec, 4, data);

array = std::move(view);

EXPECT_EQ(array.get_data(), data);
EXPECT_EQ(array.get_data()[0], TypeParam{1});
EXPECT_EQ(array.get_data()[1], TypeParam{2});
EXPECT_EQ(array.get_data()[2], TypeParam{3});
EXPECT_EQ(array.get_data()[3], TypeParam{4});
EXPECT_EQ(array.get_num_elems(), 4);
EXPECT_EQ(data[0], TypeParam{1});
EXPECT_EQ(data[1], TypeParam{2});
EXPECT_EQ(data[2], TypeParam{3});
EXPECT_EQ(data[3], TypeParam{4});
EXPECT_EQ(view.get_data(), nullptr);
ASSERT_EQ(view.get_num_elems(), 0);
}


TYPED_TEST(Array, MoveArrayToView)
{
TypeParam data[] = {1, 2, 3};
auto view = gko::Array<TypeParam>::view(this->exec, 3, data);
gko::Array<TypeParam> array_size2(this->exec, {5, 4});
gko::Array<TypeParam> array_size4(this->exec, {5, 4, 2, 1});
auto size2_ptr = array_size2.get_data();
auto size4_ptr = array_size4.get_data();

view = std::move(array_size2);
Comment thread
pratikvn marked this conversation as resolved.

EXPECT_EQ(view.get_data()[0], TypeParam{5});
EXPECT_EQ(view.get_data()[1], TypeParam{4});
EXPECT_EQ(view.get_num_elems(), 2);
EXPECT_NE(view.get_data(), data);
EXPECT_EQ(view.get_data(), size2_ptr);
EXPECT_NO_THROW(view = std::move(array_size4));
Comment thread
yhmtsai marked this conversation as resolved.
EXPECT_EQ(view.get_data(), size4_ptr);
EXPECT_EQ(array_size2.get_data(), nullptr);
ASSERT_EQ(array_size2.get_num_elems(), 0);
Comment thread
tcojean marked this conversation as resolved.
}


Expand Down
79 changes: 65 additions & 14 deletions include/ginkgo/core/base/array.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.


#include <ginkgo/core/base/exception.hpp>
#include <ginkgo/core/base/exception_helpers.hpp>
#include <ginkgo/core/base/executor.hpp>
#include <ginkgo/core/base/types.hpp>
#include <ginkgo/core/base/utils.hpp>
Expand Down Expand Up @@ -254,7 +255,8 @@ class Array {
* Creates an Array from existing memory.
*
* The Array does not take ownership of the memory, and will not deallocate
* it once it goes out of scope.
* it once it goes out of scope. This array type cannot use the function
* `resize_and_reset` since it does not own the data it should resize.
*
* @param exec executor where `data` is located
* @param num_elems number of elements in `data`
Expand All @@ -269,7 +271,10 @@ class Array {
}

/**
* Copies data from another array.
* Copies data from another array or view. In the case of an array target,
* the array is resized to match the source's size. In the case of a view
* target, if the dimensions are not compatible a gko::OutOfBoundsError is
* thrown.
*
* This does not invoke the constructors of the elements, instead they are
* copied as POD types.
Expand All @@ -291,17 +296,39 @@ class Array {
data_ = data_manager{nullptr, other.data_.get_deleter()};
}
if (other.get_executor() == nullptr) {
this->resize_and_reset(0);
this->clear();
return *this;
}
this->resize_and_reset(other.get_num_elems());
exec_->copy_from(other.get_executor().get(), num_elems_,

if (this->is_owning()) {
this->resize_and_reset(other.get_num_elems());
} else {
GKO_ENSURE_COMPATIBLE_BOUNDS(other.get_num_elems(),
this->num_elems_);
}
exec_->copy_from(other.get_executor().get(), other.get_num_elems(),
other.get_const_data(), this->get_data());
return *this;
}

/**
* Moves data from another array.
* Moves data from another array or view. Only the pointer and deleter type
* change, a copy only happens when targeting another executor's data. This
* means that in the following situation:
* ```cpp
* gko::Array<int> a; // an existing array or view
* gko::Array<int> b; // an existing array or view
* b = std::move(a);
* ```
* Depending on whether `a` and `b` are array or view, this happens:
* + `a` and `b` are views, `b` becomes the only valid view of `a`;
* + `a` and `b` are arrays, `b` becomes the only valid array of `a`;
* + `a` is a view and `b` is an array, `b` frees its data and becomes the
* only valid view of `a` ();
* + `a` is an array and `b` is a view, `b` becomes the only valid array
* of `a`.
*
* In all the previous cases, `a` becomes invalid (e.g., a `nullptr`).
*
* This does not invoke the constructors of the elements, instead they are
* copied as POD types.
Expand All @@ -323,17 +350,17 @@ class Array {
data_ = data_manager{nullptr, other.data_.get_deleter()};
}
if (other.get_executor() == nullptr) {
this->resize_and_reset(0);
this->clear();
return *this;
}
if (exec_ == other.get_executor() &&
data_.get_deleter().target_type() != typeid(view_deleter)) {
// same device and not a view, only move the pointer
if (exec_ == other.get_executor()) {
// same device, only move the pointer
using std::swap;
swap(data_, other.data_);
swap(num_elems_, other.num_elems_);
other.clear();
} else {
// different device or a view, copy the data
// different device, copy the data
*this = other;
}
return *this;
Expand All @@ -354,6 +381,8 @@ class Array {

/**
* Resizes the array so it is able to hold the specified number of elements.
* For a view and other non-owning Array types, this throws an exception
* since these types cannot be resized.
*
* All data stored in the array will be lost.
*
Expand All @@ -371,11 +400,16 @@ class Array {
throw gko::NotSupported(__FILE__, __LINE__, __func__,
"gko::Executor (nullptr)");
}
num_elems_ = num_elems;
if (num_elems > 0) {
if (!this->is_owning()) {
throw gko::NotSupported(__FILE__, __LINE__, __func__,
"Non owning gko::Array cannot be resized.");
}

if (num_elems > 0 && this->is_owning()) {
num_elems_ = num_elems;
data_.reset(exec_->alloc<value_type>(num_elems));
} else {
data_.reset(nullptr);
this->clear();
}
}

Expand Down Expand Up @@ -432,6 +466,23 @@ class Array {
data_ = std::move(tmp.data_);
}

/**
* Tells whether this Array owns its data or not.
*
* Views do not own their data and this has multiple implications. They
* cannot be resized since the data is not owned by the Array which stores a
* view. It is also unclear whether custom deleter types are owning types as
* they could be a user-created view-type, therefore only proper Array which
* use the `default_deleter` are considered owning types.
*
* @return whether this Array can be resized or not.
*/
bool is_owning()
{
return data_.get_deleter().target_type() == typeid(default_deleter);
}


private:
using data_manager =
std::unique_ptr<value_type[], std::function<void(value_type[])>>;
Expand Down
Loading