Introducing MemoryType::PINNED_HOST

[madsbk]

No description provided.

[pentschev]

Left only minor non-blocking questions, otherwise LGTM.

[pentschev]

Should we implement an abstraction, or an type checker perhaps to use in such situations? E.g.:

is_host_memory(MemoryType type) {
    switch (mem_type) {
    case MemoryType::HOST:
    case MemoryType::PINNED_HOST:
        return true;
    default:
        return false;
}

[madsbk]

Let's wait. In this case, I think we want to change behavior for PINNED_HOST: #726 (comment)

[nirandaperera]

shouldnt this be other way around? HOST = 2 and PINNED_HOST=1. Then they will match the indices of MEMORY_TYPES and MEMORY_TYPE_NAMES. Which could be important when someone static_casts the enum to size_t.

[pentschev]

The order that matters is in https://github.com/rapidsai/rapidsmpf/pull/731/files#diff-8284f273564b468de620841d4acdbc243fff0e68b5c0aae3dc13fbcbc10914b0R20-R22 and https://github.com/rapidsai/rapidsmpf/pull/731/files#diff-8284f273564b468de620841d4acdbc243fff0e68b5c0aae3dc13fbcbc10914b0R37-R39, it's just the enum here.

[nirandaperera]

Well, consider the following

auto m = MemoryType::PINNED_HOST;
auto m_to_s = MEMORY_TYPE_NAMES[static_cast<size_t>(m)];  // == "HOST" 

[pentschev]

The definition clearly bases itself off of MEMORY_TYPES and not MemoryType:

/// @brief Memory type names sorted to match `MEMORY_TYPES`.
constexpr std::array<char const*, MEMORY_TYPES.size()> MEMORY_TYPE_NAMES{

But I agree this is confusing, and I was going to say that one alternative could be to provide a function to get names, then I realized it already exists and in fact will hit the exact case you're pointing out.

Thus I agree, we either need to match the order or find a way to make this more robust providing an explicit separation of enums and the MEMORY_TYPES array providing the actual ordering.

[madsbk]

Good catch. It was indeed my intention to have MEMORY_TYPES define the order of preference.
For now, let’s also sort MemoryType: 7410e76.

In a follow-up we should introduce a proper class for arrays that can be indexed with MemoryType without using static_cast<std::size_t>, and that enforces the correct ordering.

[pentschev]

Thanks Mads and Niranda.

[wence-]

Tiny suggestions

[madsbk]

Thanks everyone

[madsbk]

/merge