Skip to content

Commit

Permalink
Fix libcudf memory errors (#8884)
Browse files Browse the repository at this point in the history
Fixes #8883

All memory errors caught in libcudf unit tests are fixed in this PR.
These unit tests are checked with `cuda-memcheck` before and after the fix.


The following tests FAILED:
- [x]          2 - SCALAR_TEST (Failed) (Fixed)
- [x]         32 - COPYING_TEST (Failed) (Fixed)
- [x]         39 - MERGE_TEST (Failed) (Fixed)
- [x]         46 - FACTORIES_TEST (Failed) (Fixed by scalar test fix)

Authors:
  - Karthikeyan (https://github.com/karthikeyann)

Approvers:
  - Christopher Harris (https://github.com/cwharris)
  - David Wendt (https://github.com/davidwendt)
  - MithunR (https://github.com/mythrocks)

URL: #8884
  • Loading branch information
karthikeyann authored Aug 12, 2021
1 parent 7461b20 commit d22fdcf
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 30 deletions.
8 changes: 6 additions & 2 deletions cpp/src/dictionary/detail/merge.cu
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <cudf/column/column.hpp>
#include <cudf/column/column_factories.hpp>
#include <cudf/detail/indexalator.cuh>
#include <cudf/detail/null_mask.hpp>
#include <cudf/dictionary/detail/encode.hpp>
#include <cudf/dictionary/detail/merge.hpp>
#include <cudf/dictionary/dictionary_column_view.hpp>
Expand Down Expand Up @@ -62,8 +63,11 @@ std::unique_ptr<column> merge(dictionary_column_view const& lcol,
return make_dictionary_column(
std::make_unique<column>(lcol.keys(), stream, mr),
std::move(indices_column),
rmm::device_buffer{
lcol.has_nulls() || rcol.has_nulls() ? static_cast<size_t>(merged_size) : 0, stream, mr},
cudf::detail::create_null_mask(
lcol.has_nulls() || rcol.has_nulls() ? static_cast<size_t>(merged_size) : 0,
mask_state::UNINITIALIZED,
stream,
mr),
lcol.null_count() + rcol.null_count());
}

Expand Down
9 changes: 5 additions & 4 deletions cpp/src/scalar/scalar.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
#include <structs/utilities.hpp>

#include <cudf/column/column.hpp>
#include <cudf/detail/utilities/vector_factories.hpp>
#include <cudf/detail/null_mask.hpp>
#include <cudf/fixed_point/fixed_point.hpp>
#include <cudf/scalar/scalar.hpp>
#include <cudf/strings/string_view.hpp>
Expand Down Expand Up @@ -574,12 +574,13 @@ void struct_scalar::superimpose_nulls(rmm::cuda_stream_view stream,
rmm::mr::device_memory_resource* mr)
{
// push validity mask down
std::vector<bitmask_type> host_validity({0});
auto validity = cudf::detail::make_device_uvector_sync(host_validity, stream, mr);
std::vector<bitmask_type> host_validity(
cudf::bitmask_allocation_size_bytes(1) / sizeof(bitmask_type), 0);
auto validity = cudf::detail::create_null_mask(1, mask_state::ALL_NULL, stream);
auto iter = thrust::make_counting_iterator(0);
std::for_each(iter, iter + _data.num_columns(), [&](size_type i) {
cudf::structs::detail::superimpose_parent_nulls(
validity.data(), 1, _data.get_column(i), stream, mr);
static_cast<bitmask_type const*>(validity.data()), 1, _data.get_column(i), stream, mr);
});
}

Expand Down
3 changes: 1 addition & 2 deletions cpp/src/structs/utilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -187,8 +187,7 @@ void superimpose_parent_nulls(bitmask_type const* parent_null_mask,
{
if (!child.nullable()) {
// Child currently has no null mask. Copy parent's null mask.
child.set_null_mask(rmm::device_buffer{
parent_null_mask, cudf::bitmask_allocation_size_bytes(child.size()), stream, mr});
child.set_null_mask(cudf::detail::copy_bitmask(parent_null_mask, 0, child.size(), stream, mr));
child.set_null_count(parent_null_count);
} else {
// Child should have a null mask.
Expand Down
14 changes: 6 additions & 8 deletions cpp/tests/copying/concatenate_tests.cu
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,6 @@ using Table = cudf::table;

template <typename T>
struct TypedColumnTest : public cudf::test::BaseFixture {
static std::size_t data_size() { return 1000; }
static std::size_t mask_size() { return 100; }
cudf::data_type type() { return cudf::data_type{cudf::type_to_id<T>()}; }

TypedColumnTest(rmm::cuda_stream_view stream = rmm::cuda_stream_default)
Expand All @@ -58,14 +56,14 @@ struct TypedColumnTest : public cudf::test::BaseFixture {
{
auto typed_data = static_cast<char*>(data.data());
auto typed_mask = static_cast<char*>(mask.data());
std::vector<char> h_data(data_size());
std::vector<char> h_data(data.size());
std::iota(h_data.begin(), h_data.end(), char{0});
std::vector<char> h_mask(mask_size());
std::vector<char> h_mask(mask.size());
std::iota(h_mask.begin(), h_mask.end(), char{0});
CUDA_TRY(cudaMemcpyAsync(
typed_data, h_data.data(), data_size(), cudaMemcpyHostToDevice, stream.value()));
typed_data, h_data.data(), data.size(), cudaMemcpyHostToDevice, stream.value()));
CUDA_TRY(cudaMemcpyAsync(
typed_mask, h_mask.data(), mask_size(), cudaMemcpyHostToDevice, stream.value()));
typed_mask, h_mask.data(), mask.size(), cudaMemcpyHostToDevice, stream.value()));
stream.synchronize();
}

Expand Down Expand Up @@ -484,7 +482,7 @@ TEST_F(OverflowTest, Presliced)
auto offset_gen = cudf::detail::make_counting_transform_iterator(
0, [string_size](size_type index) { return index * string_size; });
cudf::test::fixed_width_column_wrapper<int> offsets(offset_gen, offset_gen + num_rows + 1);
auto many_chars = cudf::make_fixed_width_column(data_type{type_id::INT8}, num_rows);
auto many_chars = cudf::make_fixed_width_column(data_type{type_id::INT8}, total_chars_size);
auto col = cudf::make_strings_column(
num_rows, offsets.release(), std::move(many_chars), 0, rmm::device_buffer{});

Expand Down Expand Up @@ -515,7 +513,7 @@ TEST_F(OverflowTest, Presliced)
offsets->view().begin<offset_type>(),
offsets->view().end<offset_type>(),
offsets->mutable_view().begin<offset_type>());
auto many_chars = cudf::make_fixed_width_column(data_type{type_id::INT8}, num_rows);
auto many_chars = cudf::make_fixed_width_column(data_type{type_id::INT8}, total_chars_size);
auto col = cudf::make_strings_column(
num_rows, std::move(offsets), std::move(many_chars), 0, rmm::device_buffer{});

Expand Down
22 changes: 8 additions & 14 deletions cpp/tests/utilities/column_utilities.cu
Original file line number Diff line number Diff line change
Expand Up @@ -114,14 +114,6 @@ std::unique_ptr<column> generate_child_row_indices(lists_column_view const& c,
//
// result = [6, 1, 11, 1, 1]
//
auto validity_iter = cudf::detail::make_counting_transform_iterator(
0,
[row_indices = row_indices.begin<size_type>(),
validity = c.null_mask(),
offset = c.offset()] __device__(int index) {
auto const true_index = row_indices[index] + offset;
return !validity || cudf::bit_is_set(validity, true_index) ? 1 : 0;
});
auto output_row_iter = cudf::detail::make_counting_transform_iterator(
0,
[row_indices = row_indices.begin<size_type>(),
Expand All @@ -136,8 +128,9 @@ std::unique_ptr<column> generate_child_row_indices(lists_column_view const& c,
output_row_iter,
output_row_iter + row_indices.size(),
output_row_start->view().begin<size_type>(),
validity_iter,
result->mutable_view().begin<size_type>());
row_size_iter,
result->mutable_view().begin<size_type>(),
[] __device__(auto row_size) { return row_size != 0; });

// generate keys for each output row
//
Expand All @@ -150,11 +143,12 @@ std::unique_ptr<column> generate_child_row_indices(lists_column_view const& c,
keys->mutable_view().end<size_type>(),
[] __device__() { return 0; });
thrust::scatter_if(rmm::exec_policy(),
validity_iter,
validity_iter + row_indices.size(),
row_size_iter,
row_size_iter + row_indices.size(),
output_row_start->view().begin<size_type>(),
validity_iter,
keys->mutable_view().begin<size_type>());
row_size_iter,
keys->mutable_view().begin<size_type>(),
[] __device__(auto row_size) { return row_size != 0; });
thrust::inclusive_scan(rmm::exec_policy(),
keys->view().begin<size_type>(),
keys->view().end<size_type>(),
Expand Down

0 comments on commit d22fdcf

Please sign in to comment.