Device/host pointer safety and memory management

[sethrj]

As @pcanal has noted on multiple occasions, there's no type safety in place to prevent device pointers from being unintentionally dereferenced as though they were host pointers. The main safety feature is just the name of accessors being device_pointers.

In multiple classes now, for host-side testing (MaterialParams, LivermoreParams) as well as host-side setup (ParticleParams) we're duplicating memory structures and owning side-by-side vector<Foo> host_foo_ and DeviceVector<Foo> device_foo_, and near-duplicate code for creating host_pointers and device_pointers that both return BarPointers classes.

Maybe there's a way to reduce some of the duplication and complexity, and make the code more typesafe and clean?

enum class Ownership
{
    host_storage,
    device_storage,
    reference,
    const_reference,
    host_reference = reference,
    host_const_reference = const_reference,
    device_reference = reference,
    device_const_reference = const_reference,
};

template<class T, Ownership S>
struct StorageType;

template<class T>
struct StorageTraits<T, host_storage>
{
    using type = std::vector<T>;
};

template<class T>
struct StorageTraits<T, device_storage>
{
    using type = DeviceVector<T>;
};

template<class T>
struct StorageTraits<T, reference>
{
    using type = Span<T>;
};

template<class T>
struct StorageTraits<T, const_reference>
{
    using type = Span<const T>;
};

template<Ownership W>
struct MaterialData
{
    template<class T> using Storage<T> = typename StorageTraits<T, W>::type;

    Storage<ElementDef> elements;
    Storage<MaterialDef> materials;
    size_type max_element_components;

    template<Ownership X>
    MaterialData& operator=(const MaterialData<X>& other)
    {
        // ??? //
    }
};

class MaterialParams
{
  public:
    using HostPointers = MaterialData<Ownership::const_reference>;
    using DevicePointers = MaterialData<Ownership::device_const_reference>;
    
    HostPointers host_pointers() const;
    DevicePointers device_pointers() const;

  private:
    MaterialData<Ownership::host_storage> host_data_;
    MaterialData<Ownership::device_storage> device_data_;
};

class MaterialView
{
  public:
    using MaterialPointers = MaterialData<Ownership::const_reference>;
    MaterialView(const MaterialPointers& pointers, ...);
};

[tmdelellis]

This looks conspicuously like memory spaces in Kokkos.

[sethrj]

I haven't looked in depth at kokkos in a long time. I recall that the kokkos memory model required homogeneous types, and a lot of its magic was in ability to reorder data based on CPU/GPU/OpenMP execution models. I definitely don't want to replicate the complexity of kokkos -- which also supports device-to-host. The use case here is also more about heterogeneous structured data and whether we can avoid defining the same hierarchical data struct in three different places (host storage, device storage, and "pointers").

[pcanal]

enum class Ownership

Does it really indicate ownership (the owner can and should delete the memory eventually) or location?

[sethrj]

In the sketch above it indicates both -- storage vs view is owning vs non-owning. And then there's storage for the host data vs storage for the device data.

[sethrj]

You're right that it defines both the "memory space" and the "ownership" semantics -- but I don't know whether it's better to have two separate enums for that or one. Especially when the BlahView classes need to take a non-owning "native" memory space container, which has to be a single type whether being built on host or device. Let me think about this...

[pcanal]

    MaterialData<Ownership::host_storage> host_data_;
    MaterialData<Ownership::device_storage> device_data_;

Wouldn't those 2 always come in pairs? Should we have:

template <class ConcreteData>
class HostDeviceParamsConnector
{
  public:
    using HostPointers = ConcreteData<Ownership::const_reference>;
    using DevicePointers = ConcreteData<Ownership::device_const_reference>;
    
    HostPointers host_pointers() const;
    DevicePointers device_pointers() const;

  private:
    ConcreteData<Ownership::host_storage> host_data_;
    ConcreteData<Ownership::device_storage> device_data_;
};
and then
class MaterialParams : public HostDeviceParamsConnector<MaterialData>
{

};

and HostDeviceParamsConnector could encapsulate the memory allocation, deallocation and transfer details.

[sethrj]

Usually, yes, a class will have both -- currently the "host pointers" are targeted at unit testing only. From a practicality standpoint, the inheritance-template you suggest will save a few lines per class and help enforce an interface; but from a design standpoint I generally dislike (at least in C++) mixin-type classes, especially those with data. (It can confuse ownership of data between parent and daughter: I hate reading code and wondering if the data belongs in a class or its parent or the grandparent...). Aesthetically I don't like them either 😆 -- a user reading the MaterialParams class would have to know the "implementation detail" of what HDPC provides and how to use it.

[sethrj]

See #131