Separate methods v.s. "mode" parameters

[SimonSapin]

#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:

• 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 #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.

[TimDiekmann]

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!

[Amanieu]

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.

[TimDiekmann]

We could add init_zeroed and init_zeroed_offset to MemoryBlock.

[Amanieu]

Eh, I guess I don't feel too strongly either way.

[nnethercote]

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?

[TimDiekmann]

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.

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.

[TimDiekmann]

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.

[nnethercote]

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.

[Amanieu]

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]

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 rust-lang/rust#72417.

[Amanieu]

I agree. Shall we go ahead with this and unblock rust-lang/rust#72417?

[SimonSapin]

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.

[Lokathor]

Yeah, I still think NonNull<[u8]> is a bad change even though the rest of these proposals are good.

[TimDiekmann]

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.

[SimonSapin]

To replace init and init_offset methods on AllocInit we can have init_zeroed and init_zeroed_from as suggested by… you #58 (comment) :)

These methods can be on MemoryBlock or NonNull<[_]>. Which one is used doesn’t really affect separate methods v.s. mode parameter.

[Wodann]

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]

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?

[SimonSapin]

No need to block on me, but if the auto-assigned reviewer is unresponsive feel free to ping me.

[Kixunil]

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]

Also just realized that a simple Vec::reserve() could use this to avoid copying uninitialized part. So maybe not that niche?

[nnethercote]

@Kixunil I don't recall any situations like that.