dimforge
`resize_*_mut` always clones
In-place resize operations all currently clone the method receiver and then pass the cloned value on to other resize methods. The subsequent resize methods may also copy all the data if, e.g., a reallocation is necessary. This often means copying all the data twice.
Using the *_mut methods seem strictly worse than moving the matrix into and out-of the non-mut versions of resize.
There are TODOs in the code, so I assume this was understood and accepted at the time of writing, but I wanted to raise the issue here for visibility. I may work on an PR if I find the time.
Huh, I remember noticing resize_mut showing up in my profiling at one point. This puzzled me, as I expected it to be mostly a no-op for my case. Would be great to get this fixed, and also make sure that resize_mut performs no work if no resize is required, since resizing a buffer to the appropriate size in a loop is a common operation in my experience.
To fix this, maybe we need a new ResizableAllocator trait? The cleanest way I can think of immediately, but there may be better ways.
I'll have to check if it actually works, but I was going to try a trick using std::mem::swap and writing things in terms of the non-mut versions. I think that will only be efficient for matrices with a dynamic allocation which are easily moved.
I'll have to check if it actually works, but I was going to try a trick using
std::mem::swapand writing things in terms of the non-mut versions. I think that will only be efficient for matrices with a dynamic allocation which are easily moved.
The trick I had in mind isn't safe in the presence of panicking code. There are some crates that more thoroughly handle this (https://docs.rs/take_mut/latest/take_mut/), but I think writing a proper in-place resize is the better move.
I'll have to check if it actually works, but I was going to try a trick using
std::mem::swapand writing things in terms of the non-mut versions. I think that will only be efficient for matrices with a dynamic allocation which are easily moved.
FWIW only dynamic matrices are resizable to begin with. (There's reshape for static matrices, which doesn't change the total number of elements)
Here's what I've got so far: https://github.com/CattleProdigy/nalgebra/commit/7cc7406ce1928ac05d75b99c45aad3fcc52c7787
Without taking ownership, it's difficult to leverage MaybeUninit as thoroughly as the existing resize code. Accordingly, I think this new code may copy the fill value more than is necessary in some cases.
It's probably possible to reserve_exact, return the uninit storage with spare_capacity_mut and then have the caller call set_len when finished populating the space. Generally, I try to avoid having the unsafe maintenance of invariants span function calls like this. Though, that is more or less what's happening with the existing non-mut resize_generic code.
@Andlon Could you take a look at the commit in the previous comment?
You had mentioned making a new type of allocator, but I ended up making this change by adding a new storage trait. This seems to work fine, but I'm not familiar enough with the internal design to see any negative implications.
Thanks for looking into this @CattleProdigy! Could you perhaps start a draft PR? It would be easier to discuss than a free-standing commit. I'm sorry but I don't have the capacity to review this properly at the moment (since I'm also not intimately familiar with the resize code it'd take me more time than I'm able to spare right now). It definitely looks like a good start though! I can't give a good guess of when I'll be able to review unfortunately, I'm still in a bit of a crunch with work, I can only hope it resolves soon.
A storage trait also makes more sense, that's what I intended (like ResizableStorage) but I wrote Allocator instead, glad you caught that anyway!