Introduce the Store API for great good. #3446
Conversation
The Store offers a more flexible allocation API, suitable for in-line memory store, shared memory store, compact "pointers", const/static use, and more. Adoption of this API, and its use in standard collections, would render a number of specialized crates obsolete in full or in part, such as StackFuture.
ehuss
added
the
T-libs-api
| - Add a separate `SharingStore` trait -- see future possibilities. | ||
|
|
||
| It should be noted that `dyn` usage of `Allocator` and `Store` suffers from the requirement of using unrelated traits as | ||
| it is not possible to have a `dyn Allocator + Clone + PartialEq` trait today, though those traits can be implemented for |
There was a problem hiding this comment.
Multiple trait vtables are coming "soon," along with vtable upcasting.
I am now thinking we might potentially want a separate trait ErasedStore from the trait Store for the purpose of dynamic storages, to shim ErasedHandle in and potentially to provide is_sharing_with and clone functionality. Making Store deliberately not object safe initially might thus be desirable, until we figure out how dynamic storages "should" work.
| Since those extra capabilities can be brought in by user traits for now, I would favor adopting a wait-and-see approach | ||
| here. |
There was a problem hiding this comment.
It's not exactly trivial to add post facto, since all storages would need to provide the "potentially limited storage" API for it to be meaningfully useful. But combined with pattern refined types, I think adding to Store something like:
pub trait Store {
const MAX_STORAGE: usize = isize::MAX as usize;
const MIN_STORAGE: usize = 0;
}should get the job done, since you can have e.g.
struct Vec<T, S> {
handle: S::Handle,
len: usize @ 0..={S::MAX_STORAGE / size_of::<T>()},
cap: usize @ {S::MIN_STORAGE / size_of::<T>()}..={S::MAX_STORAGE / size_of::<T>()},
store: S,
marker: PhantomData<Box<[T]>>,
}and that should hopefully theoretically be able to at least optimize for single-valued cap and cap/len which are always less than uNN... though also maybe not, depending on coercions (e.g. (&usize @ PAT) as &usize); pattern restricted types might just give you niches, not size compression. Interim solutions that use other solutions should definitely be left to 3rd party, since that type jujitzu is definitely beyond anything else std has exposed. (The Store trait hierarchy is already close to if not the most involved API in std, even after the massive simplifications v3 gives over v2 and v2 gives over v1.)
There was a problem hiding this comment.
I'm still not convinced that this would the right point of customization, to be honest.
For example, I regularly use u32 for length (and u8 for capacity, as the exponent of a power-of-2) even with the Allocator API not because the Allocator cannot provide more, but because I don't care about supporting more.
In fact, my implementation of InlineVec uses u16 regardless, since I have uses for more than 255, but not use over 64K elements.
Requiring to wrap the store into an adapter that limits the sizes every time you wish to save up some bytes seem more complicated than directly plugging the types you want to use in the first place.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
I think CAD has already covered most of my thoughts on the API, so I just want to say: I'm very happy to see this initiative moving forwards! |
text/3446-store.md
Outdated
| type Handle: Copy; | ||
|
|
||
| /// Returns a dangling handle, always invalid. | ||
| fn dangling() -> Self::Handle; |
There was a problem hiding this comment.
Would it make sense to take a Layout or ptr::Alignment in dangling, so that Vec::new can use dangling to get the 0-length slice pointer?
There was a problem hiding this comment.
That's a good question.
My intent was for dangling handles to always be invalid, and therefore to be UB to do anything with it, including resolving it. For example, I hoped that an index could use a giant value, which resolve would be able to detect was out of bounds in Debug.
Needless to say, this doesn't mesh well with Vec::as_ptr, which is then used for 0-length slices. It requires Vec to introduce a branch in as_ptr to choose between resolving or returning a dangling pointer, and that branch may not be well-optimized at all... it's all around sad.
This leaves two choices, as far as I can see:
- Alter the invalidity of dangling handle, and state that they must be resolvable, though the resolved pointer itself is then invalid. It would still be invalid to attempt to grow/shrink/deallocate a dangling handle, however.
- Require
Vecto use a 0-sized allocation, and implementers to handle those.
I believe (1) is clearly the superior alternative here, so I'll go ahead and amend the RFC in that direction in the next batch of fixes, and mention why in the alternatives section.
There was a problem hiding this comment.
So... I went ahead and tried having dangling take a ptr::Alignment argument, and there was an unforeseen circumstance: it became fallible.
I created a branch on the companion repository which I invite you to look at. The problem I face there is that inline stores cannot provide an allocation with an alignment greater than their own as allocations are always offsets within the store itself. In turn, this makes Vec::new() fallible, or in the companion repository branch it makes LinkedList::new() and SkipList::new() fallible.
This result, in turn, in making dangling fallible since one may (accidentally, I suppose) ask for an alignment greater than what the Store may be able to provide, even in the absence of actual allocation.
I am quite unsure of how to move forward on this.
There was a problem hiding this comment.
In #3446 (comment) @CAD97 raised the possibility of an allocate_dangling method.
It would make the interface more complex, but we could have:
Store::invalid: an invalid handle, it should never be resolved, deallocated, grown, nor shrunk.Store::allocate_zero_sized: a specialized allocation method for ZSTs. The handle is valid, and I would argue should be treated like any other handle: it may be resolved, deallocated, grown, etc... (it can't be shrunk). Multiple independent ZST handles may resolve to the same memory address. Unlike CAD97's proposal I'd argue it must be deallocated ultimately.
This would help making LinkedList::new and SkipList::new infallible again, as they could use Store::invalid. It would not, unfortunately, help Vec::new be infallible as Store::allocate_zero_sized would remain fallible due to the alignment argument.
There was a problem hiding this comment.
Unlike CAD97's proposal I'd argue it must be deallocated ultimately.
This is a problem for implementations. It also leads to the situations we see where both sides of the allocation interface end up checking for ZSTs and having to special case them (See my links to the ZST allocation issues with the current API elsewhere in the thread). While that's less detrimental for Stores (Store is probably not going to be frequently, if at all, used via virtual dispatch), it's still a bad sign IMO.
There was a problem hiding this comment.
an unforeseen circumstance:
danglingbecame fallible.
Fallibility wasn't unforseen on my end; my allocate_dangling did return Result to allow failure to allocate if the alignment request cannot be fulfilled. It's fine if Vec::new can fail and call handle_alloc_error, imho; the entire collection interface is built around assuming that allocation is essentially infallible. A try_new_in (or whatever solution is used for fallible allocation in collections) would be necessary instead to avoid termination on exhausting the storage.
The ability to skip deallocating an allocate_dangling handle is key to it being any amount of realistically useful. Without that guarantee, it's unambiguously better to just never use the allocator to create zero-sized allocations.
Note that my spitball still allowed zero sized allocations in the standard allocate path, requiring deallocation to prevent leaks, with the expectation that this allocation would be permitted to use the exact same path as non-zero-sized allocation and be wasteful for zero-sized allocation which can conjure a fresh allocation at any nonzero aligned address.
Storeis probably not going to be frequently, if at all, used via virtual dispatch
It'd be used dynamically essentially exactly as much as Allocator would, since it's replacing Allocator as the storage generic axis for collections. FWIW, I fully expect dyn GlobalAlloc will always remain the interface used for #[global_allocator], and changing it to be dyn Allocator to be unnecessary and of no real benefit. It makes sense that the API tradeoff calculus would be different for the globally available static allocation interface. (Though automatically bridging from Allocator to GlobalAlloc is both reasonable and desirable.)
There was a problem hiding this comment.
It's fine if
Vec::newcan fail and callhandle_alloc_error, imho; the entire collection interface is built around assuming that allocation is essentially infallible.
I am coming around to thinking this may be fine.
By defining type InlineVec<T, const N: usize> = Vec<T, InlineSingleStore<[T; N]>>; the type is guaranteed to have an infallible dangling, and thus users of the abstraction will never see a panicking new.
Performance-wise, this would rely on dangling being inlined in new, so the optimizer can see the error path is never taken and optimize out any check/call, but that can be tamed.
There was a problem hiding this comment.
It's fine if Vec::new can fail and call handle_alloc_error, imho; the entire collection interface is built around assuming that allocation is essentially infallible
I'm not sure this is fine, it seems quite unfortunate to me. Note that Vec::new is currently const. Obviously that only applies to the global allocator (or whatever default store is used for Vec), but it seems like quite a drawback to prevent things like Store-based smallvec/arrayvec/etc from having const fn news (which I can say very concretely is something desirable).
There was a problem hiding this comment.
but it seems like quite a drawback to prevent things like
Store-based smallvec/arrayvec/etc from havingconst fn news.
Note that handle_alloc_error is (unstably) const, so it won't prevent InlineVec::new from being const.
On the other hand, Rust doesn't support marking individual trait functions const for now, so we may need to lobby the compiler team or otherwise find a clever work-around for Store::dangling.
|
What does the indirection of having Handle types give us that we don't get from just using raw pointers? Is this mostly to support remote allocation (e.g. GPU resources?) I'm not sure that this is a great way to support such scenarios, and it seems to significantly complicate the API surface. |
Everything! :) It enables in-line stores, to start with, by which I mean a store which returns pointer in the memory range spanned by The same self-referential problem appears in Handles also allow, roughly in order of importance (for me!):
Or in short, yes there's an inconvenience, which I find slight in front of all the I note that all the usecases mentioned here are already part of the Motivation section of this RFC: do you have any suggestion to tweak/rewrite the Motivation section to make it clearer? |
|
Hm, that's fairly compelling, thanks. |
|
I'm cautiously optimistic about this API. I like the way it works, I like the flexibility of it, and I'm hopeful it'll really improve things. Initially, I was worried about the amount of unsafe code still required to make simple array-based storage work, but then I remembered that Honestly, I wonder if there's any merit to talking about "levels of unsafety" considering how UB is itself defined to be maximum unsafety regardless. Either way, I wouldn't be upset to see this implemented as-is. |
There was a problem hiding this comment.
What I currently see as blocking concerns (discussed in above reviews, I'll hopefully edit in direct links later):
- A resolution for
dangling/Vec::newgetting a handle resolvable to a zero-sized allocation, while still guaranteeing that no storage is actually reserved yet - Either a known way to ensure
Boxretains its retag behavior or signoff that this meansBoxdoesn't cause retags / doesn't getnoalias - Further investigation showing that the cost of
&mutmethods outweighs the opportunity cost of requiring all storages with inline state to be!Freeze(and thus making the container using them!Freezeeven if it doesn't utilize the shared mutability) - Added discussion of what happens with the
Allocatortrait; integrating it as part of theStoretrait hierarchy seems preferable to making it essentially an "impl alias."
|
One note about the Allocator trait is that |
Nothing else does right now. I specifically switched to taking
There would be overhead, yes, however it wouldn't be a "surprise", and therefore it's something that can be coded against. For example, if you have a Another possibility is to introduce a further trait which eschew calling the store when the handle is self-resolvable ( trait StoreHandle: Copy {
default fn resolve<S>(self, store: &Store) -> NonNull<u8>
where
S: Store<Handle = Self>,
{
store.resolve(self)
}
}
impl<H> StoreHandle for H
where
H: TryInto<NonNull<u8>> + Copy,
{
default fn resolve<S>(self, store: &Store) -> NonNull<u8>
where
S: Store<Handle = Self>,
{
self.try_into().unwrap_or_else(|| store.resolve(self))
}
} |
This works for a vector/string but not something like a map. It also only works with read-only interfaces. Note that with allocator you can add things to and remove things from the vector with no cost unless you trigger reallocation. |
|
This specific reply wanders pretty far off topic, but I don't think the resolve overhead for The key feature is (unproposed) final trait methods where default method bodies are currently allowed. If a trait method's default is final, it would not be allowed to be replaced by the concrete Pair that with a How practical getting those features are, though, as well as if it's desirable to block stabilization of (at least the Even if it can't be done automatically, a wrapping |
Isn't this the main purpose of I would imagine that using dynamic dispatch for something else would be ill-advised precisely for the performance issues you mention. I can't see why something like an array-based |
|
Right, but I couldn't use that in that way with Vec, could I? At that point, you're back to implementing data structures by hand yourself. |
* Changes: - Rename all XxxStore traits to StoreXxx. * Motivation: As proposed by @CAD97, this establishing a naming convention in which: - StoreXxx are the traits of the API. - XxxStore are the types implementing the API.
It will depend how efficient you want things to be. Firstly, if you use Then, it's just a matter of implementing an adapter struct DynStore<'a, H>(&'a dyn Store<Handle = H>);
unsafe impl<'a, H> const StoreDangling for DynStore<'a, H>
where
H: Copy + From<NonNull<u8>>,
{
type Handle = H;
fn dangling(&self, alignment: Alignment) -> Result<Self::Handle, AllocError> {
let handle: NonNull<u8> = /* magic */;
Ok(handle.into())
}
}
unsafe impl<'a, H> Store for DynStore<'a, H>
where
H: Copy + From<NonNull<u8>> + Into<NonNull<u8>>,
{
unsafe fn resolve(&self, handle: H) -> NonNull<u8> { handle.into() }
fn allocate(&self, layout: Layout) -> Result<Self::Handle, AllocError> { self.0.allocate(layout) }
// ... forward all other methods ...
}And that's it! Just parameterize your If really you somehow got yourself stuck with a If the local increase of memory usage is a problem, it's also possible to use a non-Send store adapter which stores the mapping from handle to pointer (and back) into a thread-local store, then returns the pointer as a handle directly. The mapping only would have to be consulted on allocation/deallocation, so its overhead should be relatively minimal. Anyway, before I start pulling any more hare-brained schemes, I think I've made it clear that there are many solutions that do NOT involve re-implementing collections: the whole point of the Store API is to avoid having to do so, after all. |
* Changes: - Introduce StoreDangling, as super-trait of Store. - Make dangling take an alignment, it becomes fallible. * Motivation: A separate trait is necessary for `dangling` to be usable in const contexts even when the entire `Store` implementation cannot be const. An alignment is necessary for efficient use in `Vec`, which takes advantage of NonNull::dangling being aligned today.
I added the The reasoning for a separate trait is laid out in the RFC, but in short, it's not possible to have a
Given that you used |
|
I don't think I saw anything about being able to use There is a very interesting experiment going on with the /// Defaults to current allocator style of panic on OOM
trait Allocator {
type Result<T> = T;
fn convert_result<T, E>(res: Result<T, E>) -> Self::Result<T> {
match res {
Ok(val) -> val,
Err(e) -> handle_alloc_error(e),
}
}
}
impl Alloc for FallibleAllocator {
type Result<T> = Result<T, AllocError>;
fn map_result .// ..
}
// Resolves to `()` for infallible, `Result<(), AllocError>` for fallible
impl<T, A:Alloc> Vec<T, A> { fn push(val: T) -> A::Result<()>; }
impl<T, A: Alloc> Box<T, A> { fn push(val: T) -> A::Result<Box<T,A>>; }I think support for some sort of behavior like this may be fairly crucial for this proposal since it is intended to work in a lot of places where allocation can fail, but I am unsure what the best solution is that would be. Or if I might have just missed something in the RFC. |
|
I'm curious if this RFC supports the creation of a "resizeable" This could exist as a separate type but I can see people wanting the growability with other smart pointers like |
Both Allocator and Store support reallocation, so in that sense, yes. |
* Changes: - Demonstrate hardships imposed by using for<'a> &'a S: Store, namely the impossibility to name handles for such a store. - Add example demonstrating the collision of expectations resulting from attempting to use resolve_mut in the context of iteration over mutable references to elements. * Motivation: Lay down hard-learned lessons.
|
@CAD97 @thomcc I had some time on my hands, so attempted to rework the Store API in order to pass I unfortunately, ran into a couple of issues, which I've added to the lessons learned of this RFC:
Both of the above are perhaps less problematic for an |
|
I believe we've all been busy, and so progress on this RFC has stalled. I think it would be good to establish a TODO list of things to be done, and for me to consolidate this list in the top comment. The experiment for RFC #3490 is fairly interesting, to me, as it means being able to remove Is there any other outstanding point you'd like to see clarified and/or investigated? |
|
I have a few suggestions and thoughts which I hope may help improving this API. Error handlingInstead of returning #[const_trait]
pub unsafe trait StoreDangling {
/// Error type for fallible operations
type Error: Into<AllocError> = AllocError;
// [...]
}There is a blanket implementation of unsafe impl const StoreDangling for InfallibleExample {
// No alloc, dealloc, or resize will ever fail in this store
type Error = !;
// [...]
}This solution has a few fallbacks though:
It's also be possible to have per-method error type but I feel like that makes the trait less focused and harder to use. Mutability, sharing, and multithreadingThe point that stood out the most for me is the usage of I feel that the desired mutability restrictions to cover all cases are:
The solution that I feel more likely to work is spliting the store into the interface and memory (storefront and back of house? 10 11) with traits that mark capabilities and have requirements 12:
pub trait MultiRef: Store + Clone {}
pub trait ThreadSafe: MultiRef + Send + Sync {}The data modifying methods in The separation is useful because it already exists in case of allocating functions like malloc or a This separation also makes it possible to have multiple storefronts for a single store: a store could have one lock free single-threaded, and one lock-based multithreaded storefronts. In this suggestion composites would be nice. So the following would be tribial to make:
The obvious downside to this is more traits. ResizingIt's technically always possible to try to resize by allocating + moving data + deallocating (because its specified that growing/shrinking invalidates pointers) but some implementation may not like resizing unless forced to. I can see 3 possible ways of managing it:
Lifetimes of pointed memoryWhen resolving a handle, the resulting pointer has no lifetime associated. We know for a fact that this may not be true, as it could be invalidated. One of such cases is when the the store is droped 16. This could be checked at compile time by adding lifetimes to the result type when resolving. Here is a simple example that would limit the lifetime: struct SliceStore<'a>(&'a mut [u8]);
struct StorePtr<'a = 'static>(NonNull<u8>, PhantomData<&'a mut u8>);
unsafe impl<'a> const Store<'a> for SliceStore<'a> {
unsafe fn resolve(&self, handle: Self::Handle) -> StorePtr<'a> {
// [...]
}
// [...]
}I don't really like this exact implementation, but it's useful to show the general idea. There were some discussions about using something like Footnotes
|
|
First of all, @mangelats thank you very much for the detailed feedback!
The API of This is not to say that the API of The intent is for I do think your idea of finer-grained error handling, and I think it would be worth suggesting for
Similar to the above, one of the suggestion made by @thomcc for I would welcome such a change, as it's possible to make a grow-in-place-or-out-of-place out of a grow-in-place + allocate/deallocate but the reverse is not, and thus I see grow-in-place as a more fundamental primitive. Once again, though, I'll defer the decision to
Actually... not really. One of the future extensions proposed is the idea of fungible stores. It should be possible, for example, to design an arena that is held by an But the converse is also true, the lifetime "low bound" of the allocation is also dynamic. You can keep multiple copies of a pointer, but it takes a single @CAD97 tried the lifetime approach in their revised I think it is better for
@CAD97 shares your opinion that it would be nice to avoid mandatory internal mutability, and I've investigated it quite a bit... and I don't think it's going to be fruitful. Store fronts sharing the backing memory are, indeed, extremely common, and the The fundamental problem, I think, is that at the heart of allocators there is a conflict between theory and practice:
Rust only offers a single (blunt) instrument to turn-off aliasing considerations: internal mutability, via As a result, any memory allocator written in Rust which wishes to partition a memory block -- a notable exception thus being single-allocation allocators -- MUST use Let's walk through it with an example, a Let's see it in action with an example: struct MiniStore<T> {
// Invariant: the memory cell at index `i` is allocated if `mask & (1 << i)` is set.
mask: u8,
memory: [MaybeUninit<T>; 8],
};
impl<T> Store for MiniStore<T> {
// **BOOM**
fn resolve_mut(&mut self, handle: Self::Handle) -> *mut u8 {
todo!()
}
}The problem that arises in attempting to call There is a handful of cases where you create a store with a single memory allocation -- for Since it seems that most allocators/stores -- with the clear exception of the handful of single-allocation ones -- will require internal mutability of the backing memory to protect against accidental aliasing, then it just doesn't seem worth it to complicate the API with additional traits/methods for a handful of cases. When it comes to allocator, an aliasing barrier (internal mutability) is just the name of the game, and we have to accept it. |
Is the |
Thanks for bringing this RFC to my attention. I won't even pretend to be an expert in the situation, I'd prefer @CAD97 or @RalfJung to review the situation. Still... I am afraid it actually raises more questions than it answers. Taking back my example from above: struct MiniStore<T> {
// Invariant: the memory cell at index `i` is allocated if `mask & (1 << i)` is set.
mask: u8,
memory: [MaybeUninit<T>; 8],
};
impl<T> Store for MiniStore<T> {
// **BOOM**?
fn resolve(&self, handle: Self::Handle) -> *const u8 {
todo!()
}
// **BOOM**
fn resolve_mut(&mut self, handle: Self::Handle) -> *mut u8 {
todo!()
}
}The issue is that aliasing occurs between:
From the wording of the
I seem to read -- and please correct me if I'm wrong:
If my understanding is correct, then Given that the primary motivation of @CAD97 in having mutable methods to mutate the storage -- and a It would indeed allow
So, I'd be very happy to have understood wrong :) |
text/3446-store.md
Outdated
|
|
||
| The problem is illustrated in [3446-store/aliasing-and-mutability.rs] on which Stacked Borrows chokes. While Tree | ||
| Borrows _does_ accept the program as valid, it is not clear whether LLVM `noalias` would be compatible in such a case | ||
| or not, now and in the future. |
There was a problem hiding this comment.
For all we know, if the code is accepted by Tree Borrows, then it is compatible with LLVM noalias. Everything else would be a serious bug in Tree Borrows.
text/3446-store.md
Outdated
|
|
||
| This RFC introduces 5 new traits. | ||
|
|
||
| The core of this RFC is the `Store` trait, a super-trait of the `StoreDangling` trait: |
There was a problem hiding this comment.
This says Store is a super-trait of StoreDangling, but the declaration below does the opposite: StoreDangling is a super-trait of Store.
There was a problem hiding this comment.
Oops, I'll take note and fix it later.
Ideally there would be no StoreDangling at all, but we'll need const methods in traits for that, as dangling really needs to be const if we want Vec::new to be const too for arbitrary traits (and I'd argue we do).
Thankfully, the question is much more narrow in scope :) In the comment you were replying to, there's a code snippet: struct MiniStore<T> {
// Invariant: the memory cell at index `i` is allocated if `mask & (1 << i)` is set.
mask: u8,
memory: [MaybeUninit<T>; 8],
};
impl<T> Store for MiniStore<T> {
// **BOOM**?
fn resolve(&self, handle: Self::Handle) -> *const u8 { ... }
// **BOOM**
fn resolve_mut(&mut self, handle: Self::Handle) -> *mut u8 { ... }
}The intent of the code is that The allocator/store implementation will take care not to form references to sub-regions, but it cannot help but form references to the Yet those sub-regions should be considered "independent" of each others, and of the
Since the discussion is now veering into |
Yes. That's like the
Just looking at Mutable references are still generally expected to be unique even with In particular, the model I've been using in RustBelt to prove soundness of the type system would make the following function sound, and it is intended to remain sound with fn assume_disjoint<T, U>(x: &mut T, y: &mut U) {
let xsize = mem::size_of_val(x);
let ysize = mem::size_of_val(y);
if size_t == 0 || size_u == 0 { return; }
let xaddr = x as *mut T as usize;
let yaddr = y as *mut T as usize;
// We could assume full disjointness, but checking that isn't actually easy to write, so we just
// check if y starts within x. Good enough to make the point.
if (xaddr .. xaddr+xsize).contains(&yaddr) { unsafe { hint::unreachable_unchecked(); } }
}With your API I could call that function in a way that triggers UB. Now, your API is largely unsafe (if I understood correctly), so this does not immediately imply unsoundness, but it means we have to tread very carefully. |
* Changes: - Move `resolve_mut` higher, and link to Ralf Jung's comment on the soundness challenges it presents. - Reword allocation section. - Fix mention of super-trait. * Motivation: It is now clear that `resolve_mut` is a non-starter, and thus that interior mutability will be necessary to avoid incorret `noalias` annotations. In turn, it makes it pointless to try and use `&mut self` for allocation et al.
|
@CAD97, @thomcc Given the above comment from Ralf, I am tempted to consider than any allocator/store API should mostly stick to Due to aliasing constraints, all but single-slot allocators/stores -- limited to single-element or single-array based collections -- will be incompatible with having a A counter-argument, for If it is desired to special-case such collections, then I would propose changing the traits around. The current And there would be a default implementation of Additionally, the marker store traits There would be no increase in terms of number of traits ( Obviously, this would not solve the issue that in-line stores in |
|
I should slightly revise what I said: really So there's potentially a non-interior-mutable API that works, but it'd probably involve pinning. Which makes sense I think; if your storage type stores data in-line then there is actual aliasing that needs to be handled, but if it stores data behind a pointer indirection then things become a lot less critical. Whether it's worth using
Right, that seems to be the main point... specifically this is about whether the |
Indeed, the issue is for The "multi allocations" collections (BTree{Map|Set}, LinkedList, ...} will necessarily require The "single allocation" collections (Box, Vec, VecDeque, Hash{Map|Set}, ...) could benefit from a special Hopefully I'll get some time next week-end to switch from Thank you very much for your clarifications. |
|
Hey @matthieu-m! Thanks for the lightning-fast reply. I took some time to investigate most of the things further, so sorry for the delay. Error handling & ResizingFair enough. I just wanted to give alternatives to things that were talked about. Maybe I'll see if other people proposed similar things and help there :) Lifetimes of pointed memoryI think I didn't explain myself well enough for this section. Let's imagine that you retrieve a pointer using a handle from a store. This pointer may be invalidated by any reason outlined in this RFC. We could say that it has a lifetime IMHO this is not be possible: some stores may share the handle and the same pointer may be retrieved in one thread and invalidated by another. Because of that, it's impossible to know the exact lifetime since even IO could change it. Thus, I believe the guarantees should be the responsibility of the abstractions on top of the store. But now let's define a lifetime We can build a type
'a ─────────┐
'b ───────────────────┐
'static ─────────────────────────────┐
┌────────┬─────────┬─────────┐
Pointer │ unsafe unsafe unsafe │
└────────────────────────────┘
┌────────┬─────────┬─────────┐
Reference │ valid │ invalid invalid │
└────────┴───────────────────┘
┌────────┬─────────┬─────────┐
AccessGuard │ unsafe unsafe │ invalid │
└──────────────────┴─────────┘Note that Mutability, sharing, and multithreadingThere are 3 orthogonal problems we are trying to tackle:
|
|
Another round of well-articulated feedback, I feel spoiled :)
I can see the advantage of having a maximum bound, indeed. For example, for a stack-pinned store, it's clear that all pointers will be invalidated when the stack frame containing the store is unwound. What is less clear is whether this additional safeguard can be built on top of For example, in the future possibilities, you can see that it's possible to build Ideally, I would prefer if the Would it be possible to build
I'm afraid I'm still not quite understanding why you insist on separating front and back, and why mutability of the front matters so much to you. In the end, the core data-structure is always the "back", the actual memory pool with both memory and associated metadata for tracking what is available (or not), and this memory pool will be aliased -- and thus require I don't understand what the "store front" you propose is supposed to be. It seems that it's either also the back (in-line stores) or just a reference to the back (your example) and never has any (mutable) state of its own. What role is this "front" supposed to play that a
You're close, though a bit off yet as far as I understand from my (very recent) conversation with Ralf Jung on this very thread:
Therefore, you cannot, soundly, form a mutable reference to And thus the code you linked is unsound despite the use of (And I'll freely admit I had not quite anticipated that when I launched myself in this quest, I knew aliasing mattered, but had not realized the depth of it... I hope that Ralf comments above and the
Another issue with Hence, for most stores and usecases, a As noted in my prior comment, I'll try to investigate switching from ... and at this stage I'm afraid it's about the best we can do. Whenever a store must make multiple allocations, there must be an |
|
I'm going to make simpler and more focused comments (probably a single topic) so it doesn't take so long to make them. Lifetimes of pointed memoryThe lifetime depends on the store being used. That means that if we want an abstraction to know this lifetime we need this API to share it publicly. We would like to do something like: trait Store {
lifetime 'limit;
}
impl Store for Example {
lifetime 'limit = 'static;
}So then we could use Is this what you had in mind? |
I didn't have much in mind, to be honest. I would, however, develop it much more in-line with the With that in mind: pub trait StoreUpperBound<'a>: Store {
// # Safety: see `Store::resolve`.
unsafe fn resolve_bounded(&self, handle: Self::Handle) -> AccessGuard<'a> {
// Safety:
// - Per pre-conditions.
AccessGuard::new(unsafe { self.resolve(handle) })
}
}
impl<'a> StoreUpperBound<'a> ExampleStore<'a> {}And then anyone wanting to use an And if people need to adapt existing implementation, you can simply provide an adapter struct which wraps an existing store and forwards existing traits implementations + implements I would think that's all that's necessary, no? |
|
@mangelats I updated the companion repository with a |
* Changes: - Remove StoreMultiple, blending its guarantees into Store. - Introduce parallel trait StoreSingle, specifically for single allocations. - Review APIs, examples, guarantee descriptions, and justifications. * Motivation: An in-line Store must use UnsafeCell internally, which runs afoul of LLVM coarse-grained attributes (noalias, readonly, writeonly, etc...). A specialized StoreSingle can however be used for single-allocations, powering the most commonly used collections (by a wide margin), as it side-steps the woes of UnsafeCell.
|
@CAD97 I think you will particularly appreciate this new revision, as you were the one most prominently decrying the issues that Thanks to the discussions with @mangelats (once again, thanks for your feedback!), I got the idea of moving away from a single "do-it-all"
While the "duplication" of API bothered me, at first, I think it's fine because:
This means there's essentially a single API to learn, it just so happens to be expressed as two different traits. The one thing that bothers me is that I couldn't manage to provide a default implementation of |
Avoid infinite recursion in `MaxPick::clear`. Co-authored-by: Jiahao XU <Jiahao_XU@outlook.com>
The Store offers a more flexible allocation API, suitable for in-line memory store, shared memory store, compact "pointers", const/static use, and more.
Adoption of this API, and its use in standard collections, would render a number of specialized crates obsolete in full or in part, such as StackFuture.
Rendered