Closed Amanieu closed 4 years ago
Isn't that already how it works? The current version just returns a bool for if the shrinking happened or not.
The current version returns a Result<usize, CannotReallocInPlace>
.
Ah, i was talking about the alloc-wg repo version: https://github.com/TimDiekmann/alloc-wg/blob/0fe6734bb2f1b6f537b244731c679ea34f3aceee/src/alloc/mod.rs#L240
CannotReallocInPlace
could then be renamed to CannotGrowInPlace
The current version returns a
Result<usize, CannotReallocInPlace>
.
@Lokathor This has just landed yesterday :slightly_smiling_face:
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.
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.
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.
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?
shrink_in_place
, fall back to alloc
+ dealloc
.shrink_in_place
.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".
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
.
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.
@gereeter Makes sense. So we close this?
I agree, let's just close this.
@gereeter @Amanieu Does this also mean, that shrink
shouldn't return an excess?
I think it probably should.
The
shrink_in_place
method (and theshrink
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.