Separate methods v.s. "mode" parameters #58
Description
#41 has split realloc into separate grow and shrink methods. However as of this writing some methods take AllocInit or ReallocPlacement 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 and ReallocPlacement.
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:
allocdeallocreallocby_ref
Or 10 methods:
allocalloc_zeroeddeallocgrowgrow_zeroedgrow_in_placegrow_in_place_zeroedshrinkshrink_in_placeby_ref
I tend to favor the latter. The reason against it given in #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.
Activity
TimDiekmann commentedon Apr 26, 2020
After playing around with
AllocRefa lot it's pretty annoying to passAllocInit::Uninitializedevery time I want a simplealloc. I'm up for the change!Amanieu commentedon Apr 27, 2020
While I might be convinced to split
_in_placeinto separate methods, I feel that we definitely should not do this for_zeroed. We currently provide convenience methods onAllocInitwhich allow allocators to easily handle both the uninitialized and zeroed cases.TimDiekmann commentedon Apr 27, 2020
We could add
init_zeroedandinit_zeroed_offsettoMemoryBlock.Amanieu commentedon Apr 29, 2020
Eh, I guess I don't feel too strongly either way.
nnethercote commentedon May 25, 2020
I think the
in_placevariants 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?
TimDiekmann commentedon May 25, 2020
If we decide to use
NonNull<[u8]>instead ofMemoryBlock, implementingwrite_bytesonNonNull<T>seems to be convenient enough. It's also the (IMO) expected way to zero a block of memory, especially as*mut Talso works this way.TimDiekmann commentedon May 25, 2020
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
AllocRefwhen reallocating in-place remains the same, which is a big deal.nnethercote commentedon May 25, 2020
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
InPlaceis suggestive.Amanieu commentedon May 26, 2020
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.
TimDiekmann commentedon May 29, 2020
This is a good point. Then we should just remove the API, use the second approach, replace
MemoryBlockwithNonNull<[u8]>like proposed in #61, and add more methods from*mut _toNonNull. This also unblocks rust-lang/rust#72417.Amanieu commentedon May 29, 2020
I agree. Shall we go ahead with this and unblock rust-lang/rust#72417?
SimonSapin commentedon May 29, 2020
Removing in-place reallocation from
AllocRefsounds good to me, as does replacing (the one remaining) mode parameter with separate methods.MemoryBlockv.s.NonNull<[u8]>seems orthogonal to everything else in this thread, though.Lokathor commentedon May 29, 2020
Yeah, I still think
NonNull<[u8]>is a bad change even though the rest of these proposals are good.TimDiekmann commentedon May 29, 2020
Not directly:
With
NonNull<T>::write_bytesin mind, which absolutely makes sense, we could remove those methods, too.SimonSapin commentedon May 29, 2020
To replace
initandinit_offsetmethods onAllocInitwe can haveinit_zeroedandinit_zeroed_fromas suggested by… you #58 (comment) :)These methods can be on
MemoryBlockorNonNull<[_]>. Which one is used doesn’t really affect separate methods v.s. mode parameter.Wodann commentedon Jun 1, 2020
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.
TimDiekmann commentedon Jul 26, 2020
I will come back to this next week and create a PR upstream, which removes
in_placeallocations and reverts back to separate methods as suggested. @SimonSapin do you want me to r? you?SimonSapin commentedon Jul 27, 2020
No need to block on me, but if the auto-assigned reviewer is unresponsive feel free to ping me.
Auto merge of rust-lang#74850 - TimDiekmann:remove-in-place-alloc, r=…
Kixunil commentedon Jan 21, 2022
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?
Kixunil commentedon Jan 22, 2022
Also just realized that a simple
Vec::reserve()could use this to avoid copying uninitialized part. So maybe not that niche?nnethercote commentedon Jan 24, 2022
@Kixunil I don't recall any situations like that.