Closed SimonSapin closed 3 years ago
After playing around with AllocRef
a lot it's pretty annoying to pass AllocInit::Uninitialized
every time I want a simple alloc
. I'm up for the change!
While I might be convinced to split _in_place
into separate methods, I feel that we definitely should not do this for _zeroed
. We currently provide convenience methods on AllocInit
which allow allocators to easily handle both the uninitialized and zeroed cases.
We could add init_zeroed
and init_zeroed_offset
to MemoryBlock
.
Eh, I guess I don't feel too strongly either way.
I think the in_place
variants are useless. I've spent a lot of time working on things relating to efficient and correct allocation over the years: multiple heap profilers of different kinds, multiple heap allocation checkers, and multiple implementation of growing data structures such as vectors and hash tables. Never have I felt the need to grow or shrink memory while requiring that it stay in place.
Am I overlooking an important use case?
I feel that we definitely should not do this for
_zeroed
. We currently provide convenience methods onAllocInit
which allow allocators to easily handle both the uninitialized and zeroed cases.
If we decide to use NonNull<[u8]>
instead of MemoryBlock
, implementing write_bytes
on NonNull<T>
seems to be convenient enough. It's also the (IMO) expected way to zero a block of memory, especially as *mut T
also works this way.
Am I overlooking an important use case?
Growing and shrinking in-place does not touch the data stored in the memory block (except the tail when shrinking ofc). The pointer returned from a previous call to AllocRef
when reallocating in-place remains the same, which is a big deal.
What's a specific use case of InPlace
? What problem does it solve? When would someone want to grow or shrink an allocation and have the operation fail if it can't be achieved in place? I can't imagine anything other than extremely contrived use cases.
The fact that all three of the existing implementations just return error on InPlace
is suggestive.
After thinking about it for a bit, I agree with @nnethercote: in-place reallocation is a rather niche operation which can be exposed with an extension trait on crates.io if it really becomes necessary. Another strong point in favor of removing in-place reallocation is that none of the standard allocators even support this functionality.
Another strong point in favor of removing in-place reallocation is that none of the standard allocators even support this functionality.
This is a good point. Then we should just remove the API, use the second approach, replace MemoryBlock
with NonNull<[u8]>
like proposed in #61, and add more methods from *mut _
to NonNull
. This also unblocks https://github.com/rust-lang/rust/pull/72417.
I agree. Shall we go ahead with this and unblock rust-lang/rust#72417?
Removing in-place reallocation from AllocRef
sounds good to me, as does replacing (the one remaining) mode parameter with separate methods.
MemoryBlock
v.s. NonNull<[u8]>
seems orthogonal to everything else in this thread, though.
Yeah, I still think NonNull<[u8]>
is a bad change even though the rest of these proposals are good.
MemoryBlock
v.s.NonNull<[u8]>
seems orthogonal to everything else in this thread, though.
Not directly:
We currently provide convenience methods on
AllocInit
which allow allocators to easily handle both the uninitialized and zeroed cases.
With NonNull<T>::write_bytes
in mind, which absolutely makes sense, we could remove those methods, too.
To replace init
and init_offset
methods on AllocInit
we can have init_zeroed
and init_zeroed_from
as suggested by… you https://github.com/rust-lang/wg-allocators/issues/58#issuecomment-619956605 :)
These methods can be on MemoryBlock
or NonNull<[_]>
. Which one is used doesn’t really affect separate methods v.s. mode parameter.
Irrespective of whether we move ahead with the separate decisions, I'd be in favour of not lumping the changes together. Let's just make those two decisions separately and if both are accepted make two PRs. That also makes the "history" cleaner, if both do get accepted.
In general, I am in favor of removing the in-place reallocation and reverting back to separate methods.
I will come back to this next week and create a PR upstream, which removes in_place
allocations and reverts back to separate methods as suggested. @SimonSapin do you want me to r? you?
No need to block on me, but if the auto-assigned reviewer is unresponsive feel free to ping me.
Removing in_place broke the solution of #40. I have a similar situation: a buffer that can have beginning erased and it'd be great to not copy erased part. However this is theoretical I'm not sure about the real impact. @nnethercote you seem to be experienced, did you ever come across something like this and found out that it doesn't help performance?
Also just realized that a simple Vec::reserve()
could use this to avoid copying uninitialized part. So maybe not that niche?
@Kixunil I don't recall any situations like that.
https://github.com/rust-lang/wg-allocators/issues/41 has split
realloc
into separategrow
andshrink
methods. However as of this writing some methods takeAllocInit
orReallocPlacement
parameters that are effectively boolean mode switches.Part of the reasoning for separate methods is that most callers are always in one of the two cases, so separating saves a branch in the allocator implementation. The same seems to apply to
AllocInit
andReallocPlacement
.The current design with 5 methods seems inconsistent to me. I think the trait should pick one principle among easier/nicer-looking API or single-purpose methods. This would give either 4 methods:
alloc
dealloc
realloc
by_ref
Or 10 methods:
alloc
alloc_zeroed
dealloc
grow
grow_zeroed
grow_in_place
grow_in_place_zeroed
shrink
shrink_in_place
by_ref
I tend to favor the latter. The reason against it given in https://github.com/rust-lang/wg-allocators/issues/44 is that this "blows up the trait". But I feel 10 methods is not that bad, especially given that they are not 10 completely different behaviors that need to be learned independently, but follow a pattern that’s reflected in their names.