Skip to content

Commit

Permalink
Minor I/O code quality improvements (#17105)
Browse files Browse the repository at this point in the history
This PR makes small improvements for the I/O code. Specifically,
- Place type constraint on a template class to allow only for rvalue argument. In addition, replace `std::move` with `std::forward` to make the code more *apparently* consistent with the convention, i.e. use `std::move()` on the rvalue references, and `std::forward` on the forwarding references (Effective modern C++ item 25).
- Alleviate (but not completely resolve) an existing cuFile driver close issue by removing the explicit driver close call. See #17121 
- Minor typo fix (`struct` → `class`).

Authors:
  - Tianyu Liu (https://github.com/kingcrimsontianyu)

Approvers:
  - Nghia Truong (https://github.com/ttnghia)
  - Vukasin Milovanovic (https://github.com/vuule)

URL: #17105
  • Loading branch information
kingcrimsontianyu authored Nov 1, 2024
1 parent 0a87284 commit 8219d28
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 9 deletions.
21 changes: 14 additions & 7 deletions cpp/include/cudf/io/datasource.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ class datasource {
template <typename Container>
static std::unique_ptr<buffer> create(Container&& data_owner)
{
return std::make_unique<owning_buffer<Container>>(std::move(data_owner));
return std::make_unique<owning_buffer<Container>>(std::forward<Container>(data_owner));
}
};

Expand Down Expand Up @@ -335,26 +335,33 @@ class datasource {
template <typename Container>
class owning_buffer : public buffer {
public:
// Require that the argument passed to the constructor be an rvalue (Container&& being an rvalue
// reference).
static_assert(std::is_rvalue_reference_v<Container&&>,
"The container argument passed to the constructor must be an rvalue.");

/**
* @brief Moves the input container into the newly created object.
*
* @param data_owner The container to construct the buffer from (ownership is transferred)
* @param moved_data_owner The container to construct the buffer from. Callers should explicitly
* pass std::move(data_owner) to this function to transfer the ownership.
*/
owning_buffer(Container&& data_owner)
: _data(std::move(data_owner)), _data_ptr(_data.data()), _size(_data.size())
owning_buffer(Container&& moved_data_owner)
: _data(std::move(moved_data_owner)), _data_ptr(_data.data()), _size(_data.size())
{
}

/**
* @brief Moves the input container into the newly created object, and exposes a subspan of the
* buffer.
*
* @param data_owner The container to construct the buffer from (ownership is transferred)
* @param moved_data_owner The container to construct the buffer from. Callers should explicitly
* pass std::move(data_owner) to this function to transfer the ownership.
* @param data_ptr Pointer to the start of the subspan
* @param size The size of the subspan
*/
owning_buffer(Container&& data_owner, uint8_t const* data_ptr, size_t size)
: _data(std::move(data_owner)), _data_ptr(data_ptr), _size(size)
owning_buffer(Container&& moved_data_owner, uint8_t const* data_ptr, size_t size)
: _data(std::move(moved_data_owner)), _data_ptr(data_ptr), _size(size)
{
}

Expand Down
6 changes: 5 additions & 1 deletion cpp/src/io/utilities/file_io_utilities.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,11 @@ class cufile_shim {

~cufile_shim()
{
if (driver_close != nullptr) driver_close();
// Explicit cuFile driver close should not be performed here to avoid segfault. However, in the
// absence of driver_close(), cuFile will implicitly do that, which in most cases causes
// segfault anyway. TODO: Revisit this conundrum once cuFile is fixed.
// https://github.com/rapidsai/cudf/issues/17121

if (cf_lib != nullptr) dlclose(cf_lib);
}

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/io/utilities/file_io_utilities.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ class cufile_shim;
/**
* @brief Class that provides RAII for cuFile file registration.
*/
struct cufile_registered_file {
class cufile_registered_file {
void register_handle();

public:
Expand Down

0 comments on commit 8219d28

Please sign in to comment.