kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
398 stars 65 forks source link

Mutable reference to container and mapping in mdarray #309

Closed spraetor closed 7 months ago

spraetor commented 7 months ago

In the current stable branch the mdarray implementation has a container_type& container() member returning a mutable reference, https://github.com/kokkos/mdspan/blob/fddbcb85fcff3b450593669ef40914f09cf40f71/include/experimental/__p1684_bits/mdarray.hpp#L368 I see that the current implementation is not yet aligned with the recent proposal P1684R5, but this mutable reference was removed in the R2 proposal version with the additional comment "it’s a bad idea to allow someone to resize the container owned by an mdarray …". I tried to find a more detailed explanation for this removal (and comment), but couldn't find one (maybe someone could redirect me to the discussion)

I tried to implement a dynamic-sized matrix data-structure on top of the mdarray code. A common pattern that I find in our code is either start with an zero-size matrix and then resize when needed (size might be known only after construction) (or start with an upper bound on the size and resize to a smaller size). In the proposal there is no access to the stored internal storage container anymore, so a resize would not be possible. Sure this must be done with care, e.g., properly updating the mapping alongside with the container, and it only makes sense if the underlying container is resizeable. But all this can be controlled in derived classes.

So, my questions are:

  1. What are the rationale behind removing it from the proposal?
  2. How to access the underlying container without the access functions, e.g. in derived classes?
  3. Would it make sense to make the mapping() also mutable accessible?
  4. An issue would clearly be that the container+mapping might get out of sync if only one of the two would be updated - maybe there is a pattern to guarantee that this is not happening.

PS: Having a mutable mapping would also be interesting in applications where the actual mapping must be build up step by step.

spraetor commented 7 months ago

Maybe I have answered my question myself. Sorry for the noise. (It is always good to write down the question in a concise way to see more clear :-) ).

A possible answer for question 4. is the introduced extract_container() function in R5. It moves out the container leaving the mdarray in an undetermined but valid state. This could be used to modify the extracted container and replace the mdarray with a new constructed one, e.g., a resize function could be implemented in something along the lines

template <class V,
    std::enable_if_t<std::is_convertible_v<V,value_type>, int> = 0>
void resize (const extents_type& e, const V& v)
{
  *this = [](auto&& self, const mapping_type& mapping, const value_type& value) {
      auto container = std::forward<decltype(self)>(self).extract_container();
      container.resize(mapping.required_span_size(), value);
      return base_type{mapping, std::move(container)};
  }(std::move(*this), mapping_type(e), v);
}
mhoemmen commented 7 months ago

Greetings! It does look like you answered the first question yourself : - ) . R5 has extract_container(), which is rvalue-reference-qualified and returns an rvalue reference to the container.

container_type&& extract_container() && { return std::move(ctr_); }

This exists precisely for the use case that you mentioned.

@spraetor wrote:

This could be used to modify the extracted container and replace the mdarray with a new constructed one, e.g., a resize function could be implemented in something along the[se] lines[.]

The main concern with extract_container() is the same concern that LEWG has shown with defining mdarray's moved-from state. This is clear if the container is std::vector, but it's not so clear for other container types. Unless you know what the container type is, a moved-from mdarray is unusable (its only safe member function is its destructor). extract_container() has the same effect.

spraetor commented 7 months ago

Thanks for your feedback. Immediately, after posting my questions I realized, that question 4 is essentially an answer to the other 3 and then I remembers the new added extract_container function and I thought, maybe this leads to a working pattern.

The moved-from state issue is clearly not trivial. I am mainly looking into cases where I explicitly know the type of container (mostly std::vector or std::array) and only for the vector case I intend to implement a resize function. Then, in my understanding, the assignment to the mdarray with extracted container should be fine. Maybe it is even safe, in the case of std::vector container, to ask for the extents or the mapping after extracting the container - but I don't need to in my application.

mhoemmen commented 7 months ago

Thanks for the reply and explanation! : - )

I am mainly looking into cases where I explicitly know the type of container (mostly std::vector or std::array) and only for the vector case I intend to implement a resize function. Then, in my understanding, the assignment to the mdarray with extracted container should be fine.

You can implement resize without assigning, by returning a new mdarray.