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.

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

Remove unused variable and fix memory issue in ORC writer #12984

Merged
merged 6 commits into from
Mar 23, 2023
Merged

Remove unused variable and fix memory issue in ORC writer #12984

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b43de26
Fix unused variables
ttnghia Mar 21, 2023
001e113
Revert "Fix unused variables"
ttnghia Mar 22, 2023
8dfa60a
Fix dangling pointer
ttnghia Mar 22, 2023
59d1c8b
Make variable const
ttnghia Mar 22, 2023
bae96ae
Reverse ConfigureCUDA.cmake
ttnghia Mar 22, 2023
d505fd9
Merge branch 'branch-23.04' into fix_orc_writer
ttnghia Mar 22, 2023
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
28 changes: 15 additions & 13 deletions cpp/src/io/orc/writer_impl.cu
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -594,8 +594,6 @@ void build_dictionaries(orc_table_view& orc_table,
bool enable_dictionary,
rmm::cuda_stream_view stream)
{
const auto num_rowgroups = dict.size().first;

for (size_t dict_idx = 0; dict_idx < orc_table.num_string_columns(); ++dict_idx) {
auto& str_column = orc_table.string_column(dict_idx);
str_column.attach_stripe_dict(stripe_dict.base_host_ptr(), stripe_dict.base_device_ptr());
Expand Down Expand Up @@ -2208,6 +2206,7 @@ std::tuple<orc_streams,
file_segmentation,
std::vector<StripeInformation>,
orc_table_view,
pushdown_null_masks,
rmm::device_buffer,
intermediate_statistics,
pinned_buffer<uint8_t>>
Expand All @@ -2227,7 +2226,7 @@ convert_table_to_orc_data(table_view const& input,

auto orc_table = make_orc_table_view(input, *input_tview, table_meta, stream);

auto const pd_masks = init_pushdown_null_masks(orc_table, stream);
auto pd_masks = init_pushdown_null_masks(orc_table, stream);

auto rowgroup_bounds = calculate_rowgroup_bounds(orc_table, row_index_stride, stream);

Expand Down Expand Up @@ -2299,6 +2298,7 @@ convert_table_to_orc_data(table_view const& input,
std::move(segmentation),
std::move(stripes),
std::move(orc_table),
std::move(pd_masks),
rmm::device_buffer{}, // compressed_data
intermediate_statistics{stream},
pinned_buffer<uint8_t>{nullptr, cudaFreeHost}};
Expand Down Expand Up @@ -2384,6 +2384,7 @@ convert_table_to_orc_data(table_view const& input,
std::move(segmentation),
std::move(stripes),
std::move(orc_table),
std::move(pd_masks),
std::move(compressed_data),
std::move(intermediate_stats),
std::move(stream_output)};
Expand Down Expand Up @@ -2454,16 +2455,17 @@ void writer::impl::write(table_view const& input)
// is still intact.
// Note that `out_sink_` is intentionally passed by const reference to prevent accidentally
// writing anything to it.
auto [streams,
comp_results,
strm_descs,
enc_data,
segmentation,
stripes,
orc_table,
compressed_data,
intermediate_stats,
stream_output] = [&] {
[[maybe_unused]] auto [streams,
comp_results,
strm_descs,
enc_data,
segmentation,
stripes,
orc_table,
pd_masks, /* unused, but needs to be kept alive */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you help me understand why the lifetime matters if it's unused? I don't see what this is holding, or what is holding this data, that makes its lifetime relevant.

Copy link
Contributor Author

@ttnghia ttnghia Mar 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

That buffer contains intermediate null mask for the intermediate data (orc_table) during null pushdown. Thus, we need to keep it alive in order to access the null mask later in the write_ function. Otherwise, the null mask pointer in orc_table will be dangling pointer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is easy to overlook and ignore such variable because it is unused, leading to hidden invalid memory access bugs when the buffer is destroyed earlier than needed, similar to this situation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is it needed after convert_table_to_orc_data? statistics computation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not used, but it stores null mask for orc_table to be valid. So as long as we still need orc_table we need to keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So as long as we still need orc_table we need to keep it.

Not exactly. We only need it as long as we need the null masks in the orc_table. I can't find where that is used convert_table_to_orc_data.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is used in calculate_aligned_rowgroup_bounds which is called in encode_columns.

compressed_data,
intermediate_stats,
stream_output] = [&] {
try {
return convert_table_to_orc_data(input,
*table_meta,
Expand Down