-
Notifications
You must be signed in to change notification settings - Fork 22
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
Supporting TCell without std or exclusion-set #48
Comments
This doesn't stop you creating two owners (two AtomicBool instances) for the same type. If you have two owners then you could get two mutable references to the same memory, which is a safety violation. So I don't think this will work. However let me know if I've misunderstood what you're proposing. |
So, two things here: the unsafe trait, and the macro. The unsafe trait means it's unsafe to implement. The unsafe guarantee you'd want on implementors is that there is no safe way to have multiple instances of the type alive simultaneously during execution, i.e. if someone implements it for a struct that can safely have multiple instances alive, it's their fault for implementing it on the type. It would be fine to safely to construct, drop, then construct again later. The macro is supposed to create a new static AtomicBool for each new owner type, with the idea being it'll collide if you try and, e.g., invoke it twice with the same struct. It needs a bit of work to be 100% safe; for one, it should probably create an inline module to put the struct in, something like mod $owner { ... }
$visibility use $owner::$owner; |
A motivating example using stakker; in single-stakker, TCell-using mode, instead of having the two checks for the Actor and Share owners, you can construct them simultaneously checked on a single static AtomicBool, where safety is guaranteed by the types only being built in |
Yes, there are various details that may need tuning and so on, but first I need to think through the central idea. Unfortunately my brain after Covid has ups and downs so I need to wait for the conditions to be right to think it through properly. Also, I need to do some checks. For example, whether the |
Having an I think the whole thing could be reduced down to an additional restriction on the marker type, i.e. This is because the only thing that we need is one static variable per static type. I looked at this before, but since static variables don't get created one-per-monomorphization, that seemed like a dead end. Actually the macro could just "bless" an existing marker type by adding the Creating (or blessing) the marker type with a macro is a change in the API, so a breaking change. The question is whether this is worth the trouble. It does remove a lot of internal complexity with the different std/no-std implementations. Speed isn't the issue here because we expect an owner to be long-lived. However a reduction in complexity is good. Also, for no-std, having everything static brings advantages. I will think more about it. |
It's a bit yes-and-no, since if you make the marker something the type parameter to But, it's version 0.x, so hey. |
I don't want to aggregate multiple owners into a single check. That seems like an unnecessary optimisation. Do you have a use-case where you expect owners to be recreated frequently in pairs? Okay, I'm finally understanding what you're proposing. So you're saying that things will still work because the new owner creation is independent from I wonder whether you see some other advantage of your CanOwnTCell approach? For example whether you see there being various different kinds of owner in the future. In which case, like what? For the approach of attaching the atomic to the marker type, this is what I've got it down to on the user-side:
This is a pretty minimal change, but means that the TCellOwner can get hold of the atomic and do its checks using that instead of using the hashset/whatever. As I said, that reduces complexity and is cleaner for no-std. |
No, it's the same type checking mechanism I do think your version of adding, e.g., a |
Thinking a bit more, the |
Okay, regarding whether or not Yes, it makes sense what you're saying about making I want to keep this crate minimal, or as minimal as possible to cover the required use-cases. So not just adding more and more features if they do the same thing. So ideally if I added this atomic-based code, I'd want to strip out the old code. An atomic connected to the marker could get rid of all the HashSet/exclusion-set code, reducing complexity and increasing speed, but requires a small code change for callers. However, I realize that using an atomic is not compatible with using a How did you anticipate handling the case where the owner cannot be created because there is another owner? (i.e. in a real life application, what would you do?) It may be that using an atomic has uses as a special case, rather than general-purpose. For the cases where you need the speed, you're probably creating and destroying owners frequently, maybe across threads, in which case you'd want to have a way to handle It's a shame this doesn't seem to be working out to be a general-purpose solution. Do you still have a use-case for the atomic approach despite the issues? |
Why isn't it? I don't see anything in static CONDVAR: std::sync::Condvar = std::sync::Condvar::new();
impl<Q: Marker> TCellOwner<Q> {
...
pub fn wait_for_new() -> Self {
let dummy_mutex = std::sync::Mutex::new(());
let guard = dummy_mutex.lock();
let _ = CONDVAR.wait_while(guard, |_| {
let flag = unsafe { Q::atomic_ref() };
flag.compare_exchange(false, true, Relaxed, Relaxed).is_err()
}).unwrap();
// At this point, the compare_exchange has returned Ok, so we set
// the flag and have sole ownership.
Self { ty: PhantomData }
}
} EDIT: actually, that Mutex can probably be a static as well, since we're just going to drop the guard immediately (i.e. it's just there so its guard can be passed to the condvar). Depends on how expensive the new/drop is vs. high-contention locking, and whether high contention is the expected case. |
Also, idea on how to make
|
The race I was imagining goes like this:
However if you recheck the atomic bool whilst holding the shared mutex then yes maybe that race is avoided, although I'm not totally sure. But anyway, now things are getting as complicated as the current solution so I don't see the advantage. I also had a look at the I'm a fan of reducing complexity, but I don't think this is working out any better. |
Maybe instead of a EDIT: A working example of this in action. |
Coming back to this one again. I think we've established that it's viable to change the API to keep a mutex or whatever, one per marker type. It is a breaking change but it looks viable. This removes the need to have However, I don't see a significant advantage in making the change. It isn't much of a simplification. The code in question is not performance-critical. (It is called once on creation of the owner -- that is all.) In addition the code we have right now is working and tested. So I don't think this change is worth doing right now. However I think if in the future I saw a genuine real-world use-case that absolutely required this, then that might give it a lot more weight and I would reconsider what we've discussed here. |
I believe qcell can support TCell without either of those features enabled, via refactoring the TCellOwner into an unsafe trait with most of the behaviour plus the current struct implementing this new trait. Then you can introduce a simple macro for creating new owner types that uses a static AtomicBool for uniqueness. Something like
This would be a breaking change, in that now
TCell
has to specify the fullTCellOwner<Q>
as the owner type, but if you keep the oldTCell<Q, T>
as a type aliastype TCell<Q, T> = TCellTraited<TCellOwner<Q>, T>;
I think you can downgrade it to just a major change.The text was updated successfully, but these errors were encountered: