Closed SoniEx2 closed 1 year ago
This isn't going to work. As you say, the owner is the cell itself. However to borrow mutably from the cell you need a &mut
on the owner. But behind an Arc
you can only get a &
reference. So to be correct (i.e. sound), your cell-type would only be capable of ro
type borrows, i.e. immutable borrows. To get a &mut
behind an Arc
, you need a Mutex
or RwLock
. So this intrusive-cell isn't helping at all. You might as well just have an Arc<Mutex<T>>
.
The whole point of these cell types is to temporarily "transfer" mutability from one thing to something else that has only immutable access. In this case (plain Arc
), you don't have anywhere with mutable access available. Does this make sense?
This is sound. It's just kinda (read: very) weird.
The mutability would be held by the "original Arc
". We manage the "original Arc
" by encapsulating an Arc::new_cyclic
inside an !Clone
type - the lack of Clone
prevents making new "owners" that hold the same Arc
, such that no two IntrusiveQCellOwner
ever point to the same object.
I'm still having trouble figuring out what you're trying to do.
So I think what you're saying is that the "original Arc" is the owner, and so accessing through the "original Arc" would allow modification of the cell contents. You say you're not going to allow cloning, but in your example code you have clone_inner
. Those Arc
clones are going to be useless, as you can't get access (not even immutable access) to the inside of the cell without having access to the owner (the "original Arc"). If you did allow immutable access through the Arc clones, then that would be a soundness failure, because somewhere else might be doing mutable access through the "original Arc".
However if the only Arc will be the "original Arc" (i.e. cloning is forbidden), then it doesn't need to be an Arc
-- it could just be a Box
. Still perhaps I have not really understood what you're trying to do.
(Also, if the "original Arc" needs to be the owner, then you either need to Box it or Pin it, so that its address is stable.)
Perhaps I would understand better if you could explain what advantage this approach has compared to the original code which used a QCellOwner.
The clone_inner
is -> Arc<QCell<...>>
, not -> IntrusiveQCellOwner
. The advantage is that this avoids an unnecessary allocation that's only used for the QCellOwnerID
. Also note that Arc::as_ptr
is usize
-aligned.
Arc::as_ptr
gives the address of the data inside the Arc (i.e. the data pointed to), not the address of the Arc itself (the pointer). The code as above is not going to work, but I'm trying to understand what you're trying to do to see if there is a way it could work.
If you try to code up the IntrusiveQCellOwner
using safe code and QCellOwner
maybe that would make more sense of things. It would mean that you couldn't access the T
from an Arc<QCell<T>>
without also having a reference to the IntrusiveQCellOwner
. If this isn't what you expect, then I suspect that you've overlooked some problems with this arrangement.
The code as above is going to work. And the data inside the Arc
is a QCell
- which has a QCellOwnerID
in it so it's never zero-sized anyway.
In fact, the get_inner
just needs to be:
fn get_inner(&self) -> &T {
self.ro(&self.inner)
}
The mut_inner
can't just be:
fn mut_inner(&mut self) -> &mut T {
self.rw(&mut self.inner)
}
because self.rw
borrows &mut self
. Except this isn't unsound, it just requires copying the fn rw
into mut_inner
, as rw
doesn't actually touch the QCell
contents. Note that you can't just clone the Arc
, as in:
fn mut_inner(&mut self) -> &mut T {
self.rw(&mut self.inner.clone())
}
because the cloned Arc
would get dropped at the end of the scope - there's no lifetime that satisfies this.
The idea seems (probably) sound to me. But it might be a bit much new API for fairly little benefit (saving a single allocation). It’s also quite Arc
-specific (without declaring so in the naming).
I can see that you can put an owner in IntrusiveQCellOwner
and make access through the IntrusiveQCellOwner
work. The problem is the Arc<QCell<T>>
returned by clone_inner()
. Those QCells can't be accessed without accessing them through the IntrusiveQCellOwner
. But in Resource
, you're keeping them right next to one another. So you do have the owner available. So I can kind of see how this would work now.
So this would be equivalent to putting a QCellOwner
or a QCellOwnerPinned
in the Resource
. The optimisation will save a usize
in memory (compared to QCellOwnerPinned
).
If this was going to be made into a general-purpose owner type, it's not necessary to use QCell
. It could be its own type. So you'd have ArcOwner<T>
and ArcCell<T>
. The T
in the ArcCell<T>
can't be accessed without having a &mut
or &
on the ArcOwner<T>
. This checks that the as_ptr()
on the two Arcs points to the same data. So there is no ID overhead. This is mostly useless (if you have access to the ArcOwner then why do you need to access through the ArcCell?) but in your case you're getting different vtables, which is the only thing you're really interested in.
Really the ideal situation would be to keep a Box<T>
and a bunch of vtable pointers, and reconstruct the dyn pointers. But there is no safe or approved way to do this yet, although deconstructing and reconstructing a fat pointer is commonly done in various crates. For example I use it in Stakker to store a FnOnce in a flat list.
Anyway, I think ArcOwner and ArcCell as I've described are too special-purpose to add to this crate. I can't think of any use apart from this work-around for not being able to store multiple vtables in any other way. Or can you see a more general-purpose way of implementing this that might be useful for other things?
The original IntrusiveQCellOwner
fully supports being used with "unrelated" QCell
s. Yes, our use-case is purely to carry multiple VTables around, and clone_inner
is pretty much limited/designed for that use-case, but the rest of the API isn't.
We do understand if this would be a better fit for a separate crate.
It’s also quite
Arc
-specific (without declaring so in the naming).
It's hard to make APIs like these generic. Something that could support Arc
, Rc
and Box
would be nice, but IntrusiveQCellOwner<X<QCell<T>>>
is... verbose... and you don't want to support arbitrary Deref
. you'd want some sort of unsafe trait DerefPinned: Deref
or something.
Personally I'd skip straight to the desired end-point, i.e. try to store vtable pointers directly. I had a quick look to see if there had been any progress, and actually yes there has! Simon Sapin's RFC proposal was accepted last year. Discussion about the implementation is progressing here and there seems to be a preliminary implementation here. So maybe you can implement something using this crate. If it's of interest, my own code in Stakker that deconstructs and reconstructs fat pointers is here.
We can't think of a way to carry Option<VTable>
's with a BaseObject
that isn't inherently unsound. Arc<QCell>
works. the thing is that regardless of how you do it, you still need to decouple the underlying mutability from each of the VTables, like QCell
.
honestly closing this as we believe it's handled by #37 / selfref
We don't suppose it'd be possible to have something along these lines?
The main use-case would be the same as #16 except without a separate "owner".