rust-lang / wg-allocators

Home of the Allocators working group: Paving a path for a standard set of allocator traits to be used in collections!
http://bit.ly/hello-wg-allocators
207 stars 9 forks source link

Is something like `realloc_zeroed` and `grow_in_place_zeroed` useful? #14

Closed TimDiekmann closed 4 years ago

TimDiekmann commented 5 years ago

When growing an allocation, is there a reason why there is no method that guarantees that the new memory is zeroed?

petertodd commented 5 years ago

+1

Implementation-wise for large reallocations there's definitely cases where allocators can efficiently do things like mmap additional (zeroed) pages to the end of an allocation. Also can have a default implementation that just forwards to realloc() so impls don't need to worry about them if they don't want too.

gnzlbg commented 5 years ago

I think these two can be useful. Whether these are worth having in an MVP, I don't know, we can always add them later.

TimDiekmann commented 4 years ago

AllocRef has alloc_zeroed, but no realloc_zeroed and grow_in_place_zeroed. Do we want to add those?

Shall we introduce a NonZeroLayout and use this in the alloc trait(s)?

Currently, zeroed allocation is pretty error prone and even experienced users may miss this: https://github.com/rust-lang/rust/issues/63291#issuecomment-538692745. On the other hand, we would restrict implementations which are fine to get zeroed layout like a bump allocator.

Add them now Not for an MVP No need
@Amanieu :+1:
@Ericson2314
@glandium
@gnzlbg
@Lokathor :confused:
@scottjmaddox :+1:
@TimDiekmann :+1:
@Wodann :+1:

Please vote with

Lokathor commented 4 years ago

This seems like maybe it's about two things.

Is this about having newly allocated memory blocks already be zeroed? Or is this about being able to pass a zero sized allocation request into the allocator?

TimDiekmann commented 4 years ago

It's just like alloc_zeroed: The newly allocated memory is guaranteed to be zeroed. A default implementation would look like alloc_zeroed.

Lokathor commented 4 years ago

Ah, that's very good, but i agree that it's not really necessary for the minimum "get something working" stage. Voted.

scottjmaddox commented 4 years ago

It would be better to have these before stabilizing AllocRef.

TimDiekmann commented 4 years ago

I think stabilizing the whole thing will take at least one year

Lokathor commented 4 years ago

I also wouldn't actually stabilize AllocRef without this

But it's low priority to just get people using AllocRef and get people hammering on usage problems in Nightly and so on.

TimDiekmann commented 4 years ago

I like to mention though, that implementing those function is trivial and straight forward. But we shouldn't clutter the trait unless it's more or less fixed.

Wodann commented 4 years ago

I find it hard to make a call on this one for the following reasons:

pub fn alloc_with<F>(self, layout: Layout, f: F) -> Result<NonNull<u8>, AllocErr>
    where
        F: FnOnce(ptr: NonNull<u8>, size: usize) -> NonNull<u8>;
Amanieu commented 4 years ago

I think these should be added since they can be efficiently implemented with the mremap system call on Linux. mremap allows you to move/grow/shrink a memory mapping, and any new pages added for growth are guaranteed to be zeroed.

If AllocRef does not have these methods then the user will have to manually write zeroes to the added memory since the API makes no guarantees on their contents.