Shrinking should never fail

[Amanieu]

The shrink_in_place method (and the shrink method proposed in #41) should be treated as "best effort" by the allocator and should never fail. If shrinking is not possible then they should simply return the original size to indicate that no shrinking has taken place.

[Lokathor]

Isn't that already how it works? The current version just returns a bool for if the shrinking happened or not.

[Amanieu]

The current version returns a Result<usize, CannotReallocInPlace>.

[Lokathor]

Ah, i was talking about the alloc-wg repo version: https://github.com/TimDiekmann/alloc-wg/blob/0fe6734bb2f1b6f537b244731c679ea34f3aceee/src/alloc/mod.rs#L240

[TimDiekmann]

CannotReallocInPlace could then be renamed to CannotGrowInPlace

[TimDiekmann]

The current version returns a Result<usize, CannotReallocInPlace>.

@Lokathor This has just landed yesterday 🙂

[TimDiekmann]

Currently, realloc calls shrink_in_place, if new_size < layout.size(). How would you implement realloc with this change? A solution could compare the resturned size with the requested size. So this:

if new_size > old_size {
    if let Ok(size) = self.grow_in_place(ptr, layout, new_size) {
        return Ok((ptr, size));
    }
} else if new_size < old_size {
    if let Ok(size) = self.shrink_in_place(ptr, layout, new_size) {
        return Ok((ptr, size));
    }
} else {
    return Ok((ptr, new_size));
}

could be changed to

if new_size > old_size {
    if let Ok(size) = self.grow_in_place(ptr, layout, new_size) {
        return Ok((ptr, size));
    }
} else if new_size < old_size {
    let shrinked_size = self.shrink_in_place(ptr, layout, new_size);
    if shrinked_size < old_size {
        return Ok((ptr, shrinked_size));
    }
} else {
    return Ok((ptr, new_size));
}

With #41, this would be the same.

[Amanieu]

With #41, there is no realloc any more.

The default implementation of both shrink and shrink_in_place would be the same: just return layout.size directly (no-op).

The default implementation of grow would be similar to the current implementation of realloc: first try grow_in_place, otherwise make a new allocation and copy to it.

[TimDiekmann]

With shrink instead of realloc the first if-clause would be dropped.

I don't think simply returning layout.size() in shrink is a good idea. Vec::shrink_to_fit() wouldn't actually shrink, which is pretty suboptimal. I like to keep the fallback on shrink_in_place in realloc/shrink in the default implementation.

[Amanieu]

It really depends on the allocator. If the allocator doesn't support freeing memory (e.g. bumpalo) then we want shrink to fail immediately by returning layout.size(). On the other hand if this is a more typical allocator then we would want to allocate a new, smaller block of memory and free the old one (assuming it doesn't support realloc).

The question is, what should the default behavior for shrink be?

1. Try shrink_in_place, fall back to alloc + dealloc.
2. Just forward to shrink_in_place.
3. Just do nothing.
4. Don't provide a default and force the user to implement shrink.

Option 1 would require that shrink_in_place returns a Result, where an Err would mean "couldn't shrink_in_place, but shrink might work" and an Ok would mean "we do support shrinking and this is the best we could do".

[TimDiekmann]

I think don't providing a default is the best option in the first place. This also allows us to add a default at a later point. The most intuitive default behavior would be 1. I guess as I don't expect shrink to do nothing, if I don't provide shrink and shrink_in_place.

[gereeter]

This can't work. If shrink succeeds, then any (appropriately aligned) layout between the requested layout.size() and the actual size of the memory block fits the block and is appropriate to pass to dealloc. That means that if a shrink requests a smaller size class and must succeed, so too must dealloc succeed on arbitrarily small passed in size, completely defeating the point of sized deallocation.

[TimDiekmann]

@gereeter Makes sense. So we close this?

[Amanieu]

I agree, let's just close this.

[TimDiekmann]

@gereeter @Amanieu Does this also mean, that shrink shouldn't return an excess?

[Amanieu]

I think it probably should.