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

Remove Alloc::{alloc,dealloc}_array and Alloc::{alloc,dealloc}_oneAPIs #18

Closed
gnzlbg opened this issue May 4, 2019 · 22 comments
Closed

Comments

@gnzlbg
Copy link

gnzlbg commented May 4, 2019

We should evaluate removing the Alloc::alloc_array and Alloc::dealloc_array APIs.

The main issue I have with these APIs is that they too easily allow creating zero-sized allocations if either T is a ZST, or the array length is zero. They also feel redundant and confusing for the Alloc trait, which is already quite complicated, e.g., Do I have to allocate / deallocate arrays with these? Can I mix match, e.g., Alloc::alloc_array with Alloc::dealloc and vice-versa? etc..

Ideally, Alloc and GlobalAlloc would "converge" some day, e.g., such that you can just implement Alloc for a #[global_allocator] and that's it, instead of having to implement multiple traits. Having generic methods in Alloc does make that a bit harder due to how #[global_allocator] is implemented internally and the constraints on that.

Layout already has an unstable Layout::array<T>(n: usize) method that can be used instead, and that feels like a much better place to put this functionality anyways. So just removing them, at least for the time being, sounds appealing to me. We could always add these later in a backwards compatible way anyways, but we don't have to have them now.


EDIT: as @TimDiekmann mentions all of this applies to alloc_one/dealloc_one as well.

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

I like the idea of leaning on Layout::array<T>(n: usize) array for this, and simplifying the Alloc trait. It makes sense to me that as much size calculation as possible should be in Layout, in order to maximize reusability.

@TimDiekmann
Copy link
Member

The same applies to alloc_one and dealloc_one I guess?

@gnzlbg
Copy link
Author

gnzlbg commented May 5, 2019

The same applies to alloc_one and dealloc_one I guess?

Yes, nice catch! EDIT: I've updated the issue to reflect this.


We should also consider if we want to, at least for an MVP, and as a "design guideline", ban all generic methods in Alloc, but that should be a different issue (it buys us some flexibility w.r.t. deprecating GlobalAlloc with Alloc in the future, where the current internals of GlobalAlloc are not straightforward to extend to generic methods).

@gnzlbg gnzlbg changed the title Remove Alloc::{alloc,dealloc}_array APIs Remove Alloc::{alloc,dealloc}_array and Alloc::{alloc,dealloc}_oneAPIs May 5, 2019
@SimonSapin
Copy link
Contributor

(This is the first I hear about deprecating GlobalAlloc. If there’s desire for that it’s worth discussing separately. But remember that we have to keep stable stuff around and working.)

@TimDiekmann
Copy link
Member

We should also consider if we want to, at least for an MVP

What does MVP mean in this context? I guess you don't mean Most Valuable Player.

@TimDiekmann
Copy link
Member

I don't see why we should deprecate GlobalAlloc in favor of Alloc. Both has their very own use case and acts differently. GlobalAlloc requires inner mutability while Alloc does not require this. For example it doesn't make sense to implement GlobalAlloc on a stack allocator.

@gnzlbg
Copy link
Author

gnzlbg commented May 6, 2019

What does MVP mean in this context?

Minimum Viable Project. The smallest incremental library and language change that we could make to deliver the most value and that we could then continue to iterate on. As opposed to an RFC that attempts to cover all imaginable use cases, but due to its size becomes un-reviewable, un-mergeable, and is never shipped.

@SimonSapin
Copy link
Contributor

Looking at the implementation of the methods that would be removed:

https://github.com/rust-lang/rust/blob/1.34.1/src/libcore/alloc.rs#L1035-L1231

They call corresponding Layout constructors and cast pointer types (easy/obvious enough so far) but also handle the cases where layout.size() == 0 (by returning an error). It looks like forgetting to check for zero-size could be a common source of bug. CC #16

@gnzlbg
Copy link
Author

gnzlbg commented May 7, 2019

It looks like forgetting to check for zero-size could be a common source of bug. CC #16

Note that the docs say: "For zero-sized T, may return either of Ok or Err", so at least currently, whether a zero-sized layout is an error or not would depend on the allocator that Alloc is implemented for. The default implementation of these methods just assumes that an allocator that does not override these will not support zero-sized layouts.

So I don't think that, at least with the current API, we can clearly say that this would be a bug. If we were to ban zero-sized allocations (#16) then I'd suppose we would enforce that at the API boundary by using a NonZeroLayout type or similar to prevent this bug.

Note also that with the current API and implementation, GlobalAlloc cannot be extended to have these methods, so they cannot be overriden for alloc::Global, which means that even if the allocator would support ZSTs, alloc_array for alloc::Global could error, while calling alloc_array on the allocator directly could succeed.

Maybe we could fix this by adding an associated const to GlobalAlloc that specifies whether the allocator supports zero-sized allocations but... this is something that we'd only need to consider if we don't decide to ban zero-sized allocations like #16 proposes.

@Avi-D-coder
Copy link

As far as I can tell removing these methods precludes type aware allocators. Layout does not provide type information. Without type info common typed slab/pool allocators cannot implement the Alloc trait. An allocator library can no longer provide an iterator over all structs of a type. This pattern is very common.

Without type information the Alloc trait cannot be used by ECS systems, typed Pools, heap profilers, many GC optimizations cannot be done, and on the more exotic side heuristic guided allocators lose a significant source of free information.

If generics in the API is the problem at least for my purposes replacements using TypeId would suffice.

alloc_typed(&mut self, layout: Layout, _: TypeId) -> Result<NonNull<u8>, AllocErr> {
  alloc(self, layout)
}

On Different note why avoid Generics? It's unclear to me why generics inherently limit Alloc from converging with GlobalAlloc?

@gnzlbg
Copy link
Author

gnzlbg commented Sep 11, 2019

@Avi-D-coder What you want is a very reasonable thing to want, but it is unclear to me that adding generic methods with the purpose of allocating arrays is the best way to support type aware allocators.

Type aware allocators deserve a better solution than just hacking on the fact that the array methods are accidentally generic.

@gnzlbg
Copy link
Author

gnzlbg commented Sep 11, 2019

It's unclear to me why generics inherently limit Alloc from converging with GlobalAlloc?

GlobalAlloc methods just call a symbol in the binary. If the method is generic, no such symbol can easily exist.

@Avi-D-coder
Copy link

Avi-D-coder commented Sep 11, 2019

@gnzlbg What would you consider a better solution?

GlobalAlloc methods just call a symbol in the binary. If the method is generic, no such symbol can easily exist.

Linking is a useful property to preserve. In that case I agree with you that the generics be removed, but they should be atomically replaced with TypeId equivalents. Layout cannot not be used for this purpose, and the docs should state use the TypeId equivalents for all allocations/deallocations of T: Sized.

I would be willing to add a these methods to nightly to try and get a feel of if they work? I would argue they also be added to GlobalAlloc.

@gnzlbg
Copy link
Author

gnzlbg commented Sep 11, 2019

@Avi-D-coder I think such solutions are being discussed in #15 , maybe you can chime in with your use case there?

@Avi-D-coder
Copy link

@gnzlbg I'm unclear how #15 relates? My concern is not about additional traits, but preserving and clarifying the semantics of T: Sized allocations.

@gnzlbg
Copy link
Author

gnzlbg commented Sep 11, 2019

You are right, I misread. I though that issue was about type-aware allocators (but it is about allocator-aware types). I don't think we currently have an issue tracking type-aware allocators. Maybe you could open one ?

@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

TimDiekmann commented Feb 2, 2020

I like to remove AllocRef::{alloc_one, dealloc_one, alloc_array, realloc_array, dealloc_array} upstream.

Actually, they add nothing to AllocRef except a convenience wrapper around Layout and other methods in this trait but have a major flaw: The documentation of AllocRefs notes, that

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.

With the current API, GlobalAlloc does not have those methods, so they cannot be overridden for liballoc::Global, which means that even if the global allocator would support zero-sized allocations, alloc_one, alloc_array, and realloc_array for liballoc::Global will error, while calling alloc with a zeroed-size Layout could succeed. Even worse: allocating with alloc and deallocating with dealloc_{one,array} could end up with not calling dealloc at all!

For now we should focus on a MVP for AllocRef without blowing it up unnecessarily. Even if someone wants to use those wrappers, an extension trait on crates.io is probably better suited.

If you have any concerns, please post them.

@Amanieu I'd like to r? you when pushing the changes if that is ok?

CC @gnzlbg @Ericson2314 @scottjmaddox @glandium @Wodann @Lokathor @Avi-D-coder

@Avi-D-coder
Copy link

I am no longer attempting to use the Alloc API heavily. One of the reasons is that the that the Alloc API is based on bytes, not typed memory. Untyped memory should be a special case of typed memory, since you cannot recover many of the performance advantages of typed memory with on top of untyped memory.

Anyhow I no longer have any objections to removing these methods. Though I believe we will come to regret bytes as a allocation primitive.

@Wodann
Copy link

Wodann commented Feb 3, 2020

I don't see any objection against an extension trait that provides default implementations for all of the above trait functions.

@Amanieu
Copy link
Member

Amanieu commented Feb 3, 2020

I'm happy to have these methods removed.

@Amanieu I'd like to r? you when pushing the changes if that is ok?

Sure

@Lokathor
Copy link

Lokathor commented Feb 3, 2020

I'd propose that these go away from the trait but then they get restored as free functions (in whatever module) that do the necessary layout computation and allocation call for you, if they get restored at all.

These don't make sense as trait methods or extension trait methods because individual allocators don't have any reason to override the default version of how you'd allocate/deallocate an array. Making the free functions let's people have a little help in their code without ever having to worry that a particular allocator did something weird.

@scottjmaddox
Copy link

@Lokathor if I understand correctly, extension traits with blanket impls cannot be implemented for user types, due to the lack of specialization.

JohnTitor added a commit to JohnTitor/rust that referenced this issue Feb 12, 2020
Remove common usage pattern from `AllocRef`

This removes the common usage patterns from `AllocRef`:
- `alloc_one`
- `dealloc_one`
- `alloc_array`
- `realloc_array`
- `dealloc_array`

Actually, they add nothing to `AllocRef` except a [convenience wrapper around `Layout` and other methods in this trait](https://doc.rust-lang.org/1.41.0/src/core/alloc.rs.html#1076-1240) but have a major flaw: The documentation of `AllocRefs` notes, that

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

With the current API, `GlobalAlloc` does not have those methods, so they cannot be overridden for `liballoc::Global`, which means that even if the global allocator would support zero-sized allocations, `alloc_one`, `alloc_array`, and `realloc_array` for `liballoc::Global` will error, while calling `alloc` with a zeroed-size `Layout` could succeed. Even worse: allocating with `alloc` and deallocating with `dealloc_{one,array}` could end up with not calling `dealloc` at all!

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

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

8 participants