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

Allocator traits and std::heap #32838

Open
nikomatsakis opened this Issue Apr 8, 2016 · 403 comments

Comments

Projects
None yet
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 8, 2016

FCP proposal: #32838 (comment)
FCP checkboxes: #32838 (comment)


Tracking issue for rust-lang/rfcs#1398 and the std::heap module.

  • land struct Layout, trait Allocator, and default implementations in alloc crate (#42313)
  • decide where parts should live (e.g. default impls has dependency on alloc crate, but Layout/Allocator could be in libcore...) (#42313)
  • fixme from source code: audit default implementations (in Layout for overflow errors, (potentially switching to overflowing_add and overflowing_mul as necessary).
  • decide if realloc_in_place should be replaced with grow_in_place and shrink_in_place (comment) (#42313)
  • review arguments for/against associated error type (see subthread here)
  • determine what the requirements are on the alignment provided to fn dealloc. (See discussion on allocator rfc and global allocator rfc and trait Alloc PR.)
    • Is it required to deallocate with the exact align that you allocate with? Concerns have been raised that allocators like jemalloc don't require this, and it's difficult to envision an allocator that does require this. (more discussion). @ruuda and @rkruppe look like they've got the most thoughts so far on this.
  • should AllocErr be Error instead? (comment)
  • Is it required to deallocate with the exact size that you allocate with? With the usable_size business we may wish to allow, for example, that you if you allocate with (size, align) you must deallocate with a size somewhere in the range of size...usable_size(size, align). It appears that jemalloc is totally ok with this (doesn't require you to deallocate with a precise size you allocate with) and this would also allow Vec to naturally take advantage of the excess capacity jemalloc gives it when it does an allocation. (although actually doing this is also somewhat orthogonal to this decision, we're just empowering Vec). So far @Gankro has most of the thoughts on this. (@alexcrichton believes this was settled in #42313 due to the definition of "fits")
  • similar to previous question: Is it required to deallocate with the exact alignment that you allocated with? (See comment from 5 June 2017)
  • OSX/alloc_system is buggy on huge alignments (e.g. an align of 1 << 32) #30170 #43217
  • should Layout provide a fn stride(&self) method? (See also rust-lang/rfcs#1397, #17027 )
  • Allocator::owns as a method? #44302

State of std::heap after #42313:

pub struct Layout { /* ... */ }

impl Layout {
    pub fn new<T>() -> Self;
    pub fn for_value<T: ?Sized>(t: &T) -> Self;
    pub fn array<T>(n: usize) -> Option<Self>;
    pub fn from_size_align(size: usize, align: usize) -> Option<Layout>;
    pub unsafe fn from_size_align_unchecked(size: usize, align: usize) -> Layout;

    pub fn size(&self) -> usize;
    pub fn align(&self) -> usize;
    pub fn align_to(&self, align: usize) -> Self;
    pub fn padding_needed_for(&self, align: usize) -> usize;
    pub fn repeat(&self, n: usize) -> Option<(Self, usize)>;
    pub fn extend(&self, next: Self) -> Option<(Self, usize)>;
    pub fn repeat_packed(&self, n: usize) -> Option<Self>;
    pub fn extend_packed(&self, next: Self) -> Option<(Self, usize)>;
}

pub enum AllocErr {
    Exhausted { request: Layout },
    Unsupported { details: &'static str },
}

impl AllocErr {
    pub fn invalid_input(details: &'static str) -> Self;
    pub fn is_memory_exhausted(&self) -> bool;
    pub fn is_request_unsupported(&self) -> bool;
    pub fn description(&self) -> &str;
}

pub struct CannotReallocInPlace;

pub struct Excess(pub *mut u8, pub usize);

pub unsafe trait Alloc {
    // required
    unsafe fn alloc(&mut self, layout: Layout) -> Result<*mut u8, AllocErr>;
    unsafe fn dealloc(&mut self, ptr: *mut u8, layout: Layout);

    // provided
    fn oom(&mut self, _: AllocErr) -> !;
    fn usable_size(&self, layout: &Layout) -> (usize, usize);
    unsafe fn realloc(&mut self,
                      ptr: *mut u8,
                      layout: Layout,
                      new_layout: Layout) -> Result<*mut u8, AllocErr>;
    unsafe fn alloc_zeroed(&mut self, layout: Layout) -> Result<*mut u8, AllocErr>;
    unsafe fn alloc_excess(&mut self, layout: Layout) -> Result<Excess, AllocErr>;
    unsafe fn realloc_excess(&mut self,
                             ptr: *mut u8,
                             layout: Layout,
                             new_layout: Layout) -> Result<Excess, AllocErr>;
    unsafe fn grow_in_place(&mut self,
                            ptr: *mut u8,
                            layout: Layout,
                            new_layout: Layout) -> Result<(), CannotReallocInPlace>;
    unsafe fn shrink_in_place(&mut self,
                              ptr: *mut u8,
                              layout: Layout,
                              new_layout: Layout) -> Result<(), CannotReallocInPlace>;

    // convenience
    fn alloc_one<T>(&mut self) -> Result<Unique<T>, AllocErr>
        where Self: Sized;
    unsafe fn dealloc_one<T>(&mut self, ptr: Unique<T>)
        where Self: Sized;
    fn alloc_array<T>(&mut self, n: usize) -> Result<Unique<T>, AllocErr>
        where Self: Sized;
    unsafe fn realloc_array<T>(&mut self,
                               ptr: Unique<T>,
                               n_old: usize,
                               n_new: usize) -> Result<Unique<T>, AllocErr>
        where Self: Sized;
    unsafe fn dealloc_array<T>(&mut self, ptr: Unique<T>, n: usize) -> Result<(), AllocErr>
        where Self: Sized;
}

/// The global default allocator
pub struct Heap;

impl Alloc for Heap {
    // ...
}

impl<'a> Alloc for &'a Heap {
    // ...
}

/// The "system" allocator
pub struct System;

impl Alloc for System {
    // ...
}

impl<'a> Alloc for &'a System {
    // ...
}
@gereeter

This comment has been minimized.

Copy link
Contributor

gereeter commented Apr 11, 2016

I unfortunately wasn't paying close enough attention to mention this in the RFC discussion, but I think that realloc_in_place should be replaced by two functions, grow_in_place and shrink_in_place, for two reasons:

  • I can't think of a single use case (short of implementing realloc or realloc_in_place) where it is unknown whether the size of the allocation is increasing or decreasing. Using more specialized methods makes it slightly more clear what is going on.
  • The code paths for growing and shrinking allocations tend to be radically different - growing involves testing whether adjacent blocks of memory are free and claiming them, while shrinking involves carving off properly sized subblocks and freeing them. While the cost of a branch inside realloc_in_place is quite small, using grow and shrink better captures the distinct tasks that an allocator needs to perform.

Note that these can be added backwards-compatibly next to realloc_in_place, but this would constrain which functions would be by default implemented in terms of which others.

For consistency, realloc would probably also want to be split into grow and split, but the only advantage to having an overloadable realloc function that I know of is to be able to use mmap's remap option, which does not have such a distinction.

@gereeter

This comment has been minimized.

Copy link
Contributor

gereeter commented Apr 11, 2016

Additionally, I think that the default implementations of realloc and realloc_in_place should be slightly adjusted - instead of checking against the usable_size, realloc should just first try to realloc_in_place. In turn, realloc_in_place should by default check against the usable size and return success in the case of a small change instead of universally returning failure.

This makes it easier to produce a high-performance implementation of realloc: all that is required is improving realloc_in_place. However, the default performance of realloc does not suffer, as the check against the usable_size is still performed.

eddyb added a commit to eddyb/rust that referenced this issue Oct 18, 2016

Rollup merge of rust-lang#37117 - pnkfelix:may-dangle-attr, r=nikomat…
…sakis

`#[may_dangle]` attribute

`#[may_dangle]` attribute

Second step of rust-lang#34761. Last big hurdle before we can work in earnest towards Allocator integration (rust-lang#32838)

Note: I am not clear if this is *also* a syntax-breaking change that needs to be part of a breaking-batch.

eddyb added a commit to eddyb/rust that referenced this issue Oct 19, 2016

Rollup merge of rust-lang#37117 - pnkfelix:may-dangle-attr, r=nikomat…
…sakis

`#[may_dangle]` attribute

`#[may_dangle]` attribute

Second step of rust-lang#34761. Last big hurdle before we can work in earnest towards Allocator integration (rust-lang#32838)

Note: I am not clear if this is *also* a syntax-breaking change that needs to be part of a breaking-batch.

eddyb added a commit to eddyb/rust that referenced this issue Oct 19, 2016

Rollup merge of rust-lang#37117 - pnkfelix:may-dangle-attr, r=nikomat…
…sakis

`#[may_dangle]` attribute

`#[may_dangle]` attribute

Second step of rust-lang#34761. Last big hurdle before we can work in earnest towards Allocator integration (rust-lang#32838)

Note: I am not clear if this is *also* a syntax-breaking change that needs to be part of a breaking-batch.
@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 26, 2016

Another issue: The doc for fn realloc_in_place says that if it returns Ok, then one is assured that ptr now "fits" new_layout.

To me this implies that it must check that the alignment of the given address matches any constraint implied by new_layout.

However, I don't think the spec for the underlying fn reallocate_inplace function implies that it will perform any such check.

  • Furthermore, it seems reasonable that any client diving into using fn realloc_in_place will themselves be ensuring that the alignments work (in practice I suspect it means that the same alignment is required everywhere for the given use case...)

So, should the implementation of fn realloc_in_place really be burdened with checking that the alignment of the given ptr is compatible with that of new_layout? It is probably better in this case (of this one method) to push that requirement back to the caller...

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 26, 2016

@gereeter you make good points; I will add them to the check list I am accumulating in the issue description.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Oct 31, 2016

(at this point I am waiting for #[may_dangle] support to ride the train into the beta channel so that I will then be able to use it for std collections as part of allocator integration)

@Ixrec Ixrec referenced this issue Dec 10, 2016

Closed

Alloca for Rust #1808

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jan 4, 2017

I'm new to Rust, so forgive me if this has been discussed elsewhere.

Is there any thought on how to support object-specific allocators? Some allocators such as slab allocators and magazine allocators are bound to a particular type, and do the work of constructing new objects, caching constructed objects which have been "freed" (rather than actually dropping them), returning already-constructed cached objects, and dropping objects before freeing the underlying memory to an underlying allocator when required.

Currently, this proposal doesn't include anything along the lines of ObjectAllocator<T>, but it would be very helpful. In particular, I'm working on an implementation of a magazine allocator object-caching layer (link above), and while I can have this only wrap an Allocator and do the work of constructing and dropping objects in the caching layer itself, it'd be great if I could also have this wrap other object allocators (like a slab allocator) and truly be a generic caching layer.

Where would an object allocator type or trait fit into this proposal? Would it be left for a future RFC? Something else?

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Jan 4, 2017

I don't think this has been discussed yet.

You could write your own ObjectAllocator<T>, and then do impl<T: Allocator, U> ObjectAllocator<U> for T { .. }, so that every regular allocator can serve as an object-specific allocator for all objects.

Future work would be modifying collections to use your trait for their nodes, instead of plain ole' (generic) allocators directly.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor Author

nikomatsakis commented Jan 4, 2017

@pnkfelix

(at this point I am waiting for #[may_dangle] support to ride the train into the beta channel so that I will then be able to use it for std collections as part of allocator integration)

I guess this has happened?

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jan 4, 2017

@Ericson2314 Yeah, writing my own is definitely an option for experimental purposes, but I think there'd be much more benefit to it being standardized in terms of interoperability (for example, I plan on also implementing a slab allocator, but it would be nice if a third-party user of my code could use somebody else's slab allocator with my magazine caching layer). My question is simply whether an ObjectAllocator<T> trait or something like it is worth discussing. Although it seems that it might be best for a different RFC? I'm not terribly familiar with the guidelines for how much belongs in a single RFC and when things belong in separate RFCs...

@steveklabnik

This comment has been minimized.

Copy link
Member

steveklabnik commented Jan 4, 2017

@joshlf

Where would an object allocator type or trait fit into this proposal? Would it be left for a future RFC? Something else?

Yes, it would be another RFC.

I'm not terribly familiar with the guidelines for how much belongs in a single RFC and when things belong in separate RFCs...

that depends on the scope of the RFC itself, which is decided by the person who writes it, and then feedback is given by everyone.

But really, as this is a tracking issue for this already-accepted RFC, thinking about extensions and design changes isn't really for this thread; you should open a new one over on the RFCs repo.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Jan 4, 2017

@joshlf Ah, I thought ObjectAllocator<T> was supposed to be a trait. I meant prototype the trait not a specific allocator. Yes that trait would merit its own RFC as @steveklabnik says.


@steveklabnik yeah now discussion would be better elsewhere. But @joshlf was also raising the issue lest it expose a hitherto unforeseen flaw in the accepted but unimplemented API design. In that sense it matches the earlier posts in this thread.

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jan 4, 2017

@Ericson2314 Yeah, I thought that was what you meant. I think we're on the same page :)

@steveklabnik Sounds good; I'll poke around with my own implementation and submit an RFC if it ends up seeming like a good idea.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 4, 2017

@joshlf I don't any reason why custom allocators would go into the compiler or standard library. Once this RFC lands, you could easily publish your own crate that does an arbitrary sort of allocation (even a fully-fledged allocator like jemalloc could be custom-implemented!).

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jan 4, 2017

@alexreg This isn't about a particular custom allocator, but rather a trait that specifies the type of all allocators which are parametric on a particular type. So just like RFC 1398 defines a trait (Allocator) that is the type of any low-level allocator, I'm asking about a trait (ObjectAllocator<T>) that is the type of any allocator which can allocate/deallocate and construct/drop objects of type T.

@Ericson2314

This comment has been minimized.

Copy link
Contributor

Ericson2314 commented Jan 4, 2017

@alexreg See my early point about using standard library collections with custom object-specific allocators.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 4, 2017

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 4, 2017

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jan 4, 2017

Sure, but I’m not sure that would belong in the standard library. Could easily go into another crate, with no loss of functionality or usability.

Yes but you probably want some standard library functionality to rely on it (such as what @Ericson2314 suggested).

I think you’d want to use standard-library collections (any heap-allocated value) with an arbitrary custom allocator; i.e. not limited to object-specific ones.

Ideally you'd want both - to accept either type of allocator. There are very significant benefits to using object-specific caching; for example, both slab allocation and magazine caching give very significant performance benefits - take a look at the papers I linked to above if you're curious.

@alexreg

This comment has been minimized.

Copy link
Contributor

alexreg commented Jan 4, 2017

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Jan 4, 2017

But the object allocator trait could simply be a subtrait of the general allocator trait. It’s as simple as that, as far as I’m concerned. Sure, certain types of allocators can be more efficient than general-purpose allocators, but neither the compiler nor the standard really need to (or indeed should) know about this.

Ah, so the problem is that the semantics are different. Allocator allocates and frees raw byte blobs. ObjectAllocator<T>, on the other hand, would allocate already-constructed objects and would also be responsible for dropping these objects (including being able to cache constructed objects which could be handed out later in leu of constructing a newly-allocated object, which is expensive). The trait would look something like this:

trait ObjectAllocator<T> {
    fn alloc() -> T;
    fn free(t T);
}

This is not compatible with Allocator, whose methods deal with raw pointers and have no notion of type. Additionally, with Allocators, it is the caller's responsibility to drop the object being freed first. This is really important - knowing about the type T allows ObjectAllocator<T> to do things like call T's drop method, and since free(t) moves t into free, the caller cannot drop t first - it is instead the ObjectAllocator<T>'s responsibility. Fundamentally, these two traits are incompatible with one another.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Oct 25, 2018

Are these all the methods that you mean?

  • pub fn align_to(&self, align: usize) -> Layout
  • pub fn padding_needed_for(&self, align: usize) -> usize
  • pub fn repeat(&self, n: usize) -> Result<(Layout, usize), LayoutErr>
  • pub fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutErr>
  • pub fn repeat_packed(&self, n: usize) -> Result<Layout, LayoutErr>
  • pub fn extend_packed(&self, next: Layout) -> Result<(Layout, usize), LayoutErr>
  • pub fn array<T>(n: usize) -> Result<Layout, LayoutErr>
@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Oct 25, 2018

@gnzlbg Yes.

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Oct 25, 2018

@Amanieu Sounds ok to me, but this issue is already huge. Consider filing a separate issue (or even a stabilization PR) that we could FCP separately?

bors added a commit that referenced this issue Nov 7, 2018

Auto merge of #55366 - Amanieu:stable_layout, r=Amanieu
Add tracking issue for Layout methods (and some API changes)

These methods are already useful when used with the stable global allocator API (stabilized in #51241).

```rust
pub fn align_to(&self, align: usize) -> Result<Layout, LayoutErr>;
pub fn padding_needed_for(&self, align: usize) -> usize;
pub fn repeat(&self, n: usize) -> Result<(Layout, usize), LayoutErr>;
pub fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutErr>;
pub fn repeat_packed(&self, n: usize) -> Result<Layout, LayoutErr>;
pub fn extend_packed(&self, next: Layout) -> Result<Layout, LayoutErr>;
pub fn array<T>(n: usize) -> Result<Layout, LayoutErr>;
```

cc #32838

r? @SimonSapin

bors added a commit that referenced this issue Nov 8, 2018

Auto merge of #55366 - Amanieu:stable_layout, r=Amanieu
Add tracking issue for Layout methods (and some API changes)

These methods are already useful when used with the stable global allocator API (stabilized in #51241).

```rust
pub fn align_to(&self, align: usize) -> Result<Layout, LayoutErr>;
pub fn padding_needed_for(&self, align: usize) -> usize;
pub fn repeat(&self, n: usize) -> Result<(Layout, usize), LayoutErr>;
pub fn extend(&self, next: Layout) -> Result<(Layout, usize), LayoutErr>;
pub fn repeat_packed(&self, n: usize) -> Result<Layout, LayoutErr>;
pub fn extend_packed(&self, next: Layout) -> Result<Layout, LayoutErr>;
pub fn array<T>(n: usize) -> Result<Layout, LayoutErr>;
```

cc #32838

r? @SimonSapin
@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 24, 2019

from Allocators and lifetimes:

  1. (for allocator impls): moving an allocator value must not invalidate its outstanding memory blocks.

    All clients can assume this in their code.

    So if a client allocates a block from an allocator (call it a1) and then a1 moves to a new place (e.g. vialet a2 = a1;), then it remains sound for the client to deallocate that block via a2.

Does this imply, that an Allocator must be Unpin?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Feb 24, 2019

Good catch!

Since the Alloc trait is still unstable, I think we still get to change the rules if we want to and modify this part of the RFC. But it is indeed something to keep in mind.

@shanemikel

This comment has been minimized.

Copy link

shanemikel commented Feb 25, 2019

@gnzlbg Yes, I'm aware of the huge differences in the generics systems, and that not everything he details is implementable in the same way in Rust. I've been working on the library on-and-off since posting, though, and I'm making good progress.

@withoutboats

This comment has been minimized.

Copy link
Contributor

withoutboats commented Feb 25, 2019

Does this imply, that an Allocator must be Unpin?

It doesn't. Unpin is about the behavior of a type when wrapped in a Pin, there's no particular connection to this API.

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 25, 2019

But can't Unpin be used to enforce the mentioned constraint?

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 25, 2019

Another question regarding dealloc_array: Why does the function return Result? In the current implementation, this may fail in two cases:

  • n is zero
  • capacity overflow for n * size_of::<T>()

For the first we have two cases (as in the documentation, the implementer can choose between these):

  • The allocation returns Ok on zeroed n => dealloc_array should also return Ok.
  • The allocation returns Err on zeroed n => there is no pointer that can be passed to dealloc_array.

The second is ensured by the following safety constraint:

the layout of [T; n] must fit that block of memory.

This means, that we must call dealloc_array with the same n as in the allocation. If an array with n elements could be allocated, n is valid for T. Otherwise, the allocation would have failed.

Edit: Regarding the last point: Even if usable_size returns a higher value than n * size_of::<T>(), this is still valid. Otherwise the implementation violates this trait constraint:

The block's size must fall in the range [use_min, use_max], where:

  • [...]
  • use_max is the capacity that was (or would have been) returned when (if) the block was allocated via a call to alloc_excess or realloc_excess.

This only holds, as the trait requires an unsafe impl.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Feb 25, 2019

For the first we have two cases (as in the documentation, the implementer can choose between these):

  • The allocation returns Ok on zeroed n

Where did you get this information from?

All Alloc::alloc_ methods in the docs specify that the behavior of zero-sized allocations is undefined under their "Safety" clause.

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 25, 2019

Docs of core::alloc::Alloc (highlighted relevant parts):

A note regarding zero-sized types and zero-sized layouts: many methods in the Alloc trait state that allocation requests must be non-zero size, or else undefined behavior can result.

  • However, some higher-level allocation methods (alloc_one, alloc_array) are well-defined on zero-sized types and can optionally support them: it is left up to the implementor whether to return Err, or to return Ok with some pointer.

  • If an Alloc implementation chooses to return Ok in this case (i.e. the pointer denotes a zero-sized inaccessible block) then that returned pointer must be considered "currently allocated". On such an allocator, all methods that take currently-allocated pointers as inputs must accept these zero-sized pointers, without causing undefined behavior.

  • In other words, if a zero-sized pointer can flow out of an allocator, then that allocator must likewise accept that pointer flowing back into its deallocation and reallocation methods.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Feb 25, 2019

So one of the error conditions of dealloc_array is definitely suspicious:

/// # Safety
///
/// * the layout of `[T; n]` must *fit* that block of memory.
///
/// # Errors
///
/// Returning `Err` indicates that either `[T; n]` or the given
/// memory block does not meet allocator's size or alignment
/// constraints.

If [T; N] does not meet the allocator size or alignment constraints, then AFAICT it does not fit the block of memory of the allocation, and the behavior is undefined (per the safety clause).

The other error condition is "Always returns Err on arithmetic overflow." which is pretty generic. It's hard to tell whether it is an useful error condition. For each Alloc trait implementation one might be able to come up with a different one that could do some arithmetic that could in theory wrap, so 🤷‍♂️


Docs of core::alloc::Alloc (highlighted relevant parts):

Indeed. I find it weird that so many methods (e.g. Alloc::alloc) state that zero-sized allocations are undefined behavior, but then we just provide Alloc::alloc_array(0) with implementation-defined behavior. In some sense Alloc::alloc_array(0) is a litmus test to check whether an allocator supports zero-sized allocations or not.

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 25, 2019

If [T; N] does not meet the allocator size or alignment constraints, then AFAICT it does not fit the block of memory of the allocation, and the behavior is undefined (per the safety clause).

Yup, I think this error condition can be dropped as it's redundant. Either we need the safety clause or an error condition, but not both.

The other error condition is "Always returns Err on arithmetic overflow." which is pretty generic. It's hard to tell whether it is an useful error condition.

IMO, it is guarded by the same safety clause as above; if the capacity of [T; N] would overflow, it does not fit that memory block to deallocate. Maybe @pnkfelix could elaborate on this?

In some sense Alloc::alloc_array(1) is a litmus test to check whether an allocator supports zero-sized allocations or not.

Did you mean Alloc::alloc_array(0)?

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Feb 25, 2019

IMO, it is guarded by the same safety clause as above; if the capacity of [T; N] would overflow, it does not fit that memory block to deallocate.

Note that this trait can be implemented by users for their own custom allocators, and that these users can override the default implementations of these methods. So when considering whether this should return Err for arithmetic overflow or not, one should not only focus on what the current default implementation of the default method does, but also consider what it might make sense for users implementing these for other allocators.

Did you mean Alloc::alloc_array(0)?

Yes, sorry.

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Feb 25, 2019

Note that this trait can be implemented by users for their own custom allocators, and that these users can override the default implementations of these methods. So when considering whether this should return Err for arithmetic overflow or not, one should not only focus on what the current default implementation of the default method does, but also consider what it might make sense for users implementing these for other allocators.

I see, but implementing Alloc requires an unsafe impl and implementors has to follow the safety rules mentioned in #32838 (comment).

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Mar 10, 2019

Every API left pointing here for a tracking issue is the Alloc trait or related to the Alloc trait. @rust-lang/libs, do you feel it’s useful to keep this open in addition to #42774?

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Mar 11, 2019

Simple background question: What is the motivation behind the flexibility with ZSTs? It seems to me that, given that we know at compile-time that a type is a ZST, we can completely optimize out both the allocation (to return a constant value) and the deallocation. Given that, it seems to me that we should say one of the following:

  • It's always up to the implementor to support ZSTs, and they can't return Err for ZSTs
  • It's always UB to allocate ZSTs, and it's the caller's responsibility to short-circuit in this case
  • There's some sort of alloc_inner method that callers implement, and an alloc method with a default implementation which does the short circuiting; alloc must support ZSTs, but alloc_inner may NOT be called for a ZST (this is just so that we can add the short-circuiting logic in a single place - in the trait definition - in order to save implementors some boilerplate)

Is there a reason that the flexibility we have with the current API is needed?

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 11, 2019

Is there a reason that the flexibility we have with the current API is needed?

It's a trade-off. Arguably, the Alloc trait is used more often than it is implemented, so it might make sense to make using Alloc as easy as possible by providing built-in support for ZSTs.

This would mean that implementers of the Alloc trait will need to take care of this, but more importantly to me that those trying to evolve the Alloc trait will need to keep ZSTs in mind on every API change. It also complicates the docs of the API by explaining how ZSTs are (or could be if it is "implementation defined") handled.

C++ allocators pursue this approach, where the allocator tries to solve many different problem. This did not only make them harder to implement and harder to evolve, but also harder for users to actually use because of how all these problems interact in the API.

I think that handling ZSTs, and allocating/deallocating raw memory are two orthogonal and different problems, and therefore we should keep the Alloc trait API simple by just not handling them.

Users of Alloc like libstd will need to handle ZSTs, e.g., on each collection. That's definitely a problem worth solving, but I don't think the Alloc trait is the place for that. I'd expect an utility to solve this problem to pop out within libstd out of necessity, and when that happens, we can maybe try to RFC such an utility and expose it in std::heap.

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Mar 11, 2019

That all sounds reasonable.

I think that handling ZSTs, and allocating/deallocating raw memory are two orthogonal and different problems, and therefore we should keep the Alloc trait API simple by just not handling them.

Doesn't that imply that we should have the API explicitly not handle ZSTs rather than be implementation-defined? IMO, an "unsupported" error is not very helpful at runtime since the vast majority of callers will not be able to define a fallback path, and will therefore have to assume that ZSTs are unsupported anyway. Seems cleaner to just simplify the API and declare that they're never supported.

@burdges

This comment has been minimized.

Copy link

burdges commented Mar 11, 2019

Would specialization be used by alloc users to handle ZST? Or just if size_of::<T>() == 0 checks?

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Mar 11, 2019

Would specialization be used by alloc users to handle ZST? Or just if size_of::<T>() == 0 checks?

The latter should be sufficient; the appropriate code paths would be trivially removed at compile time.

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Mar 11, 2019

Doesn't that imply that we should have the API explicitly not handle ZSTs rather than be implementation-defined?

For me, an important constraint is that if we ban zero-sized allocations, the Alloc methods should be able to assume that the Layout passed to them is not zero-sized.

There are multiple ways to achieve this. One would be to add another Safety clause to all Alloc methods stating that if the Layout is zero-sized the behavior is undefined.

Alternatively, we could ban zero-sized Layouts, then Alloc doesn't need to say anything about zero-sized allocations since these cannot safely happen, but doing that would have some downsides.

For example, , some types like HashMap build up the Layout from multiple Layouts, and while the final Layout might not be zero-sized, the intermediate ones might be (e.g. in HashSet). So these types would need to use "something else" (e.g. a LayoutBuilder type) to build up their final Layouts, and pay for a "non-zero-sized" check (or use an _unchecked) method when converting to Layout.

Would specialization be used by alloc users to handle ZST? Or just if size_of::() == 0 checks?

We can't specialize on ZSTs yet. Right now all code uses size_of::<T>() == 0 .

@joshlf

This comment has been minimized.

Copy link
Contributor

joshlf commented Mar 11, 2019

There are multiple ways to achieve this. One would be to add another Safety clause to all Alloc methods stating that if the Layout is zero-sized the behavior is undefined.

It'd be interesting to consider whether there are ways to make this a compile-time guarantee, but even a debug_assert that the layout is non-zero-sized should be sufficient to catch 99% of bugs.

@brson

This comment has been minimized.

Copy link
Contributor

brson commented Apr 4, 2019

I haven't paid any attention to discussions about allocators, so sorry about that. But I've long wished that the allocator had access to the type of the value its allocating. There may be allocator designs that could use it.

@TimDiekmann

This comment has been minimized.

Copy link
Contributor

TimDiekmann commented Apr 4, 2019

Then we'd probably have the same issues as C++ and it's allocator api.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.