Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is something like realloc_zeroed and grow_in_place_zeroed useful? #14

Closed
TimDiekmann opened this issue May 4, 2019 · 12 comments
Closed

Comments

@TimDiekmann
Copy link
Member

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

@TimDiekmann TimDiekmann added A-Alloc Trait Issues regarding the Alloc trait Discussion Issues containing a specific idea, which needs to be discussed in the WG labels May 4, 2019
@petertodd
Copy link

+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
Copy link

gnzlbg commented May 5, 2019

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 TimDiekmann added Proposal and removed Discussion Issues containing a specific idea, which needs to be discussed in the WG labels Oct 17, 2019
@TimDiekmann
Copy link
Member Author

TimDiekmann commented Jan 30, 2020

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: rust-lang/rust#63291 (comment). 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 👍
@Ericson2314
@glandium
@gnzlbg
@Lokathor 😕
@scottjmaddox 👍
@TimDiekmann 👍
@Wodann 👍

Please vote with

  • 👍 merge: Add them now
  • 😕 postpone: I want to have them, but not for an MVP
  • 👎 close: We don't need those (why?)

@Lokathor
Copy link

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
Copy link
Member Author

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

@Lokathor
Copy link

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

@scottjmaddox
Copy link

It would be better to have these before stabilizing AllocRef.

@TimDiekmann
Copy link
Member Author

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

@Lokathor
Copy link

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
Copy link
Member Author

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
Copy link

Wodann commented Feb 5, 2020

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

  • What is the design goal of the std's AllocRef API? Should it be a minimal API that can be used to build others APIs in external crates? If so, alloc_zeroed, realloc_zeroed, etc. could be in a non-std crate.
  • Should the API focus on being general-purpose? If so this might be a better design:
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
Copy link
Member

Amanieu commented Feb 5, 2020

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.

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 12, 2020
Add missing `_zeroed` varants to `AllocRef`

The majority of the allocator wg has decided to add the missing `_zeroed` variants to `AllocRef`:

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

For the full discussion please see rust-lang/wg-allocators#14.

This PR provides default implementations for `realloc_zeroed`, `alloc_excess_zeroed`, `realloc_excess_zeroed`, and `grow_in_place_zeroed`.

r? @Amanieu
JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 12, 2020
Add missing `_zeroed` varants to `AllocRef`

The majority of the allocator wg has decided to add the missing `_zeroed` variants to `AllocRef`:

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

For the full discussion please see rust-lang/wg-allocators#14.

This PR provides default implementations for `realloc_zeroed`, `alloc_excess_zeroed`, `realloc_excess_zeroed`, and `grow_in_place_zeroed`.

r? @Amanieu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants