uazu / qcell

Statically-checked alternatives to RefCell and RwLock
Apache License 2.0
356 stars 22 forks source link

T: ?Sized support #16

Closed SoniEx2 closed 3 years ago

SoniEx2 commented 3 years ago

We have this:

/// Stores multiple DataSource capable of InstanceTitle, InstanceBaseUrl and
/// RepoListUrl
#[derive(Default)]
pub struct ConfigManager {
    // conceptually the actual objects
    bases: Vec<Arc<RwLock<dyn DataSourceBase + Send + Sync>>>,
    // conceptually just views of the above objects
    titles: Vec<Option<Arc<RwLock<dyn DataSource<InstanceTitle> + Send + Sync>>>>,
    urls: Vec<Option<Arc<RwLock<dyn DataSource<InstanceBaseUrl> + Send + Sync>>>>,
    repolists: Vec<Option<Arc<RwLock<dyn DataSource<RepoListUrl> + Send + Sync>>>>,
    durations: Vec<Duration>,
    // add_source can be called after update.
    valid: usize,
}

It is our understanding that we would be able to do something like this:

/// Stores multiple DataSource capable of InstanceTitle, InstanceBaseUrl and
/// RepoListUrl
#[derive(Default)]
pub struct ConfigManager {
    owner: QCellOwner,
    // conceptually the actual objects
    bases: Vec<Arc<QCell<dyn DataSourceBase + Send + Sync>>>,
    // conceptually just views of the above objects
    titles: Vec<Option<Arc<QCell<dyn DataSource<InstanceTitle> + Send + Sync>>>>,
    urls: Vec<Option<Arc<QCell<dyn DataSource<InstanceBaseUrl> + Send + Sync>>>>,
    repolists: Vec<Option<Arc<QCell<dyn DataSource<RepoListUrl> + Send + Sync>>>>,
    durations: Vec<Duration>,
    // add_source can be called after update.
    valid: usize,
}

And this would be much faster than both RwLock and even hypothetical Arc locking, while still giving us Send+Sync. Is that accurate?

uazu commented 3 years ago

Yes, I think that looks okay so far. However, you're going to need a &mut to the ConfigManager in order to get an owner reference that's suitable for obtaining mutable access to the data stored in the cells. If access is going to be from multiple threads, then you'll need some kind of mutex or lock around the ConfigManager. So you're exchanging lots of small locks for one big lock. For your app's access patterns is that going to work out better? If that does work out better, then yes this looks good.

Are you saying you need ?Sized support for this to work? I think this will probably be okay to add. If it isn't, the compiler will complain. I'll look at it soonish.

SoniEx2 commented 3 years ago

The RwLocks are mostly a hack to have access to all the dyn Trait views we need. We actually do have an &mut to ConfigManger where we need it, which makes all these small locks pointless overhead. You should be able to easily add ?Sized support to at least QCell tho.

You don't see trait objects often in Rust, but basically, DataSourceBase is a supertrait of DataSource. DataSourceBase takes care of updates (using &mut self), while DataSource is an extensible interface for third-parties to make up their own kind of data. the ConfigManager generally just holds multiple config files (as in struct FileDataSource), but it can also use non-file-based data sources, as well as some useful wrappers. To make this work, it needs to use trait objects, and cannot use FileDataSource or other hardcoded specific sources. But you can't make a trait object that covers multiple non-marker traits (e.g. dyn DataSourceBase + DataSource<InstanceBaseUrl> + ... is invalid), nor is there a good way to have "optional" trait objects (for example, with these Options, one can register a DataSource<RepoListUrl> that doesn't provide DataSource<InstanceBaseUrl> nor DataSource<InstanceTitle>!), so you need some way to have multiple views (trait objects) over the same allocation. With this, all the Arcs point to the same underlying object, but provide access to different traits of it, and Arc maintains Send+Sync. Unfortunately Arc is read-only, even if you have a &mut to it, so something else needs to provide interior mutability, preferably either RwLock or QCell as those maintain Send+Sync. In the RwLock case this is extremely expensive due to all the locking involved, so QCell is preferable, altho currently QCell doesn't support ?Sized.

Makes sense?

uazu commented 3 years ago

Yes, it sounds fine. I tried adding ?Sized everywhere to see how it goes, and it does look okay after fixing up a few things. It needs a few tests adding to make sure, though. I can't add ?Sized on the new call, but I think that is okay. The coercion from a sized type to an unsized type will occur in the Arc constructor. (Rc, Arc, etc use CoerceUnsized, but RwLock doesn't, so for construction the coercion should work just the same as for RwLock.)

I'll finish off the change later.

uazu commented 3 years ago

Okay, it's now published as crate version 0.4.2. All cell types now support ?Sized. There are tests with Box<QCell<dyn Trait>> so I think it should work for your purposes. Could you try it, and then I can close this issue?

Also, if it works and if you have a blog, perhaps you'd like to write about it? All the focus seems to be on GhostCell, but actually that's quite hard to use. QCell despite being less advanced in theoretical terms, can still help in a lot of situations, but people don't particularly appreciate that.

SoniEx2 commented 3 years ago

It does seem to work pretty nicely! Thanks!

We had enough trouble explaining the use case in this issue as it is, doing so in a blog post is currently beyond our skill level. We'll be sure to keep it in mind tho.

uazu commented 3 years ago

Thanks for the feedback! It would be good to document this pattern somewhere, though -- at least the QCell aspect of it. I don't advertise QCell as a replacement for RwLock. I'll have a think where I can put it.