How to improve some completely unnecessary use of locks

help
1

We currently 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,
}

(note: no Clone) and it's used like this:

        /// Adds the given combined `DataSource` to this `ConfigManager`.
        pub fn add_source<T>(&mut self, source: T)
        where
            T: DataSource<InstanceTitle>,
            T: DataSource<InstanceBaseUrl>,
            T: DataSource<RepoListUrl>,
            T: Send + Sync + 'static,
        {
            let arc = Arc::new(RwLock::new(source));
            self.bases.push(arc.clone());
            self.titles.push(Some(arc.clone()));
            self.urls.push(Some(arc.clone()));
            self.repolists.push(Some(arc));
        }

        // and some other code that uses &mut self, but it doesn't
        // add/remove from the vecs so there's no point. it does get write
        // locks to the things tho - while there are no read locks around.

and unfortunately these locks are extremely unnecessary, their only purpose is to satisfy the borrow checker (a RefCell would do fine but RefCell is not Send+Sync) and they're there because Weak is useless (even tho Weak doesn't give you access to the data and Arc can provide locking of Weaks, Arc::get_mut won't let you unless you have no Weaks around). arguably even Arc is unnecessary here but it's being used for Send+Sync.

Anyway this code is sad and unnecessarily expensive and we just wish we had a better way of doing this.

2

Perhaps, if you are able to modify your Base trait, to include methods which get Option<&dyn Trait> for each other trait you have, you could do away with the separate lists entirely?

You say they are just views, so perhaps it's better to just have a single list of sources and get "views" to them live, instead of precomputing (and therefore involving shared ownership)?

1 Like
3

But we don't wanna hardcode these. The whole system is supposed to be modular and flexible and extensible and whatnot. Having the Base trait provide those would kinda defeat the purpose of having the DataSource trait...

4

It's hard to tell, without a deeper understanding of your architecture, but I would still consider a way to eliminate the precomputed views - there may be a way to do so without restricting the flexibility of the design.

5

another alternative might be an alternative Cell type, there are a few implementations floating around out there that allow you to compile-time check borrows via external handles, I found the qcell crate with a quick search:

https://docs.rs/qcell/0.4.1/qcell/

These may resolve at least the performance concerns around Mutex.

1 Like
6

?Sized tho.

7

Store indexes into the bases vector instead of sharing it with an Arc?

1 Like
8

And how do you get the vtables then?

9

This would only be sound if you could have Arc<Invariant<T>> (e.g. Arc<UnsafeCell<T>> except the Cell types are useless for this due to not being Send+Sync). Ah well.

10

Define a trait like this:

trait DataSourceHelper: Send + Sync {
    fn as_instance_title(&self) -> &dyn DataSource<InstanceTitle>;
    fn as_base_url(&self) -> &dyn DataSource<InstanceBaseUrl>;
    fn as_repo_list_url(&self) -> &dyn DataSource<RepoListUrl>;
    
    fn as_instance_title_mut(&mut self) -> &mut dyn DataSource<InstanceTitle>;
    fn as_base_url_mut(&mut self) -> &mut dyn DataSource<InstanceBaseUrl>;
    fn as_repo_list_url_mut(&mut self) -> &mut dyn DataSource<RepoListUrl>;
}

impl<T> DataSourceHelper for T
where
    T: DataSource<InstanceTitle>,
    T: DataSource<InstanceBaseUrl>,
    T: DataSource<RepoListUrl>,
    T: Send + Sync,
{
    fn as_instance_title(&self) -> &dyn DataSource<InstanceTitle> {
        self
    }
    fn as_base_url(&self) -> &dyn DataSource<InstanceBaseUrl> {
        self
    }
    fn as_repo_list_url(&self) -> &dyn DataSource<RepoListUrl> {
        self
    }
    
    fn as_instance_title_mut(&mut self) -> &mut dyn DataSource<InstanceTitle> {
        self
    }
    fn as_base_url_mut(&mut self) -> &mut dyn DataSource<InstanceBaseUrl> {
        self
    }
    fn as_repo_list_url_mut(&mut self) -> &mut dyn DataSource<RepoListUrl> {
        self
    }
}

Then store an Box<dyn DataSourceHelper>.

You can also add the traits as sub-classes of your helper trait and call the desired methods directly.

1 Like
11

We just went and asked for ?Sized, thanks! T: ?Sized support · Issue #16 · uazu/qcell · GitHub

1 Like
12

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.