Closed gnzlbg closed 4 years ago
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.
The same applies to alloc_one
and dealloc_one
I guess?
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).
(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.)
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.
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.
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.
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 https://github.com/rust-lang/wg-allocators/issues/16
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.
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
?
@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.
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.
@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
.
@Avi-D-coder I think such solutions are being discussed in https://github.com/rust-lang/wg-allocators/issues/15 , maybe you can chime in with your use case there?
@gnzlbg I'm unclear how #15 relates? My concern is not about additional traits, but preserving and clarifying the semantics of T: Sized
allocations.
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 ?
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 returnErr
, or to returnOk
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
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.
I don't see any objection against an extension trait that provides default implementations for all of the above trait functions.
I'm happy to have these methods removed.
@Amanieu I'd like to r? you when pushing the changes if that is ok?
Sure
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.
@Lokathor if I understand correctly, extension traits with blanket impls cannot be implemented for user types, due to the lack of specialization.
We should evaluate removing the
Alloc::alloc_array
andAlloc::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
withAlloc::dealloc
and vice-versa? etc..Ideally,
Alloc
andGlobalAlloc
would "converge" some day, e.g., such that you can just implementAlloc
for a#[global_allocator]
and that's it, instead of having to implement multiple traits. Having generic methods inAlloc
does make that a bit harder due to how#[global_allocator]
is implemented internally and the constraints on that.Layout
already has an unstableLayout::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.