kokkos / mdspan

Reference implementation of mdspan targeting C++23
Other
413 stars 69 forks source link

Add a resize member function to mdarray #357

Closed stefanbajakobsson closed 2 months ago

stefanbajakobsson commented 2 months ago

It would be nice to have a resize member function in mdarray taking the same extent arguments as the constructor.

nmm0 commented 2 months ago

This would probably be a lot trickier -- the Container inside of mdarray does not have to be resizable. In some cases it's certainly not (i.e. Array) and the size is fixed at compile time. Other containers may have a runtime size, but one that is fixed throughout the lifetime of the container. We could potentially detect if the container has a resize function, but that would be a bit of a complicated argument to bring to the standards committee, since in most cases you can just construct a new mdarray and move from the old one. It would also make the API a lot more confusing since some functions would be disabled depending on the type of the Container.

Another alternative would be to introduce resize(), but have as a precondition that the container can hold the new size. This seems... limiting, however.

Could you elaborate on a particular use case or motivation?

stefanbajakobsson commented 2 months ago

Thanks for you answer. I understand your points. I use mdarrays as member variables inside a class and sometimes I need to resize these variables like I resize astd::vector. Since there is no resizefunction, I have (as you write) just assigned a new mdarray with the right extents to that variable. I thought it would be neat to have a resize function for that but if this complicates the implementation too much I agree that it is not worth it.

crtrott commented 2 months ago

One problem with resize is that it doesn't actually have the same performance expectations as with vector. Consider an mdarray of size 2x3 you want to resize to 2x2. For layout_right we have the following elements in memory: {11,12,13,21,22,23}. After the resize our memory needs to look like this: {11,12,21,22}. That means the optimization of just changing the size upon shrinking but leaving some kind of capacity larger isn't actually as much of an optimization as you think, because we may need to move the majority of contained elements. Furthermore, for std::vector resize actually seems to guarantee that references (i.e. iterators, pointers) to the first count elements stay valid. Which wouldn't be possible here.

mhoemmen commented 2 months ago
  1. resize would have to be enabled conditionally, depending on whether all the extents are compile-time values.
  2. Christian's example shows that resize may either need to change the layout (impossible; the layout is a template argument) or repack the values (which might not be possible either, given a nonunique layout).
stefanbajakobsson commented 2 months ago

It is clear from the discussion that adding a resize function is not a good idea.