NVIDIA / NVIDIA/cudf

[FEA] Make `column_view` _actually_ be a view

Open
#4,270 10 comments 0 reactions 0 assignees View on GitHub
feature request libcudf
Dominant language
C++
Stars
9.8k
Forks
1.1k
Avg merge
3d 6m
Merged PRs (30d)
278

Description

**Is your feature request related to a problem? Please describe.**

`column_view` is so named because it is supposed to be a non-owning, "view" type. However, this is currently a half-truth because `column_view` _is_ an owning class!

`column_view` stores a `vector` internally for it's children. Since `vector` is an owning type, that makes `column_view` an owning type.

This has several ramifications.

First of all `column_view` is not trivially copyable (which view types should be!).

The larger problem is more insidious. The fact that `column_view` contains a `vector` object is the ultimate cause of the many compiler errors that libcudf developers have seen about calling a `__host__` function from a `__host__ __device__` context. The full causal chain is complicated.

As @harrism described in https://github.com/rapidsai/rmm/pull/312:

>Unfortunately, NVCC implicitly adds __host__ __device__ specifiers to explicitly defaulted functions that are called from both __host__ and __device__ (or __host__ __device__) functions. In libcudf, the type_dispatcher uses a __host__ __device__ function, so the above changed resulted in compiler errors since the default compiler-generated constructor necessarily invokes a host-only function.

In short, `column_view` has defaulted ctors. Since these ctors can be used within the context of the `type_dispatcher` (which is `__host__ __device__`), nvcc will implicitly add `__host__ __device__` to these ctors. These defaulted `column_view` ctors will invoke the member `vector` ctors (which are intrinsically `__host__` only). Thus, we end up with trying to call a `__host__` function (`vector` ctor/dtor) inside a `__host__ __device__` function (`column_view` ctor/dtor).

**Describe the solution you'd like**

Remove the `vector` from `column_view` and instead replace it with `column_view* children` and `size_type num_children`:

```c++
class column_view{

column_view(...., column_view * children, size_type num_children);

private:
column_view * children; // pointer to array of child `column_view` objects
size_type num_children;
};
```

This will make `column_view` a true "view" type by being trivially copyable and eliminate any possibility for the host/device errors we've encountered numerous times in the past.

But what about the children!?

The catch is, someone has to own the children `column_view` objects, i.e., the `children` pointer in `column_view` has to point to `column_view` objects that are constructed/owned by someone else. There's two scenarios we have to consider:

1. `column_view`s that view `cudf::column` objects
2. `column_view`s that _do not_ view ` cudf::column` object (e.g., viewing a Python owned column)

In 1. the fix is pretty easy. We can just add a `vector` to `cudf::column`:

```c++
class column{

private:
vector> children; // here's the owning child objects
vector child_views; // non-owning views to the same children

public:
operator column_view(){
// pass pointer to the `child_view`s data when converting to a `column_view`
return column_view{..., child_views.data(), child_views.size()};
}
};
```

Situation 2. shouldn't be too onerous either. The caller was already required to construct a `std::vector` to pass the children to the `column_view` ctor. However, instead of copying that vector, we just take a pointer to it's contents. This does add the added responsibility of the user to ensure that the `column_view` does not outlive the `vector` for the children. @shwina correct me if I'm wrong, but I don't anticipate this should be a problem from the Python/Cython side nor require significant changes?

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.