Make cf_handle lock-free

[iSynaptic]

The cf_handle function is often called very frequently and it's implementation requires acquiring a read-lock to an underlying map. If this is called in a hot-path, it can have performance implications. We should investigate switching this to an approach that doesn't require a lock. For example, we could make create_cf and drop_cf and any other function that mutates the internal map require a mutable reference to DB and make the caller responsible for acquiring any locks in order to mutate the DB.

[hjiayz]

like this?

use std::collections::HashMap;

#[derive(Debug)]
struct ColumnFamily(i32);

struct DB {
    cfs:HashMap<i32,ColumnFamily>,
}

impl DB {
    fn new()->DB{
        let mut cfs = HashMap::new();
        cfs.insert(1,ColumnFamily(1));
        cfs.insert(2,ColumnFamily(2));
        DB{
            cfs
        }
    }
    fn get(&self,key:i32)->&ColumnFamily{
        self.cfs.get(&key).unwrap()
    }
    fn drop_key(&mut self,key:i32){
        self.cfs.remove(&key);
    }
}

fn main() {
    let mut f = DB::new();
    let a = f.get(1);
    println!("{:?}",a);
    f.drop_key(1);
    let c = f.get(2);
    println!("{:?}",c);
}

[ordian]

FWIW, in paritech's fork cf_handle is Copy, which is not technically not safe (OTOH one would argue it's hard to misuse), but avoids using locks.

[iSynaptic]

@hjiayz Yes, very similar to that.

[dvdplm]

When I tried making this change (i.e. take &mut self when creating/dropping column families) I ran into problems with the way ColumnFamily is defined. It will store a mutable reference in the PhantomData which makes code like this fail:

#[test]
fn property_cf_test() {
    let n = DBPath::new("_rust_rocksdb_property_cf_test");
    {
        let opts = Options::default();
        let mut db = DB::open_default(&n).unwrap(); // Notice the `mut`
        let cf = db.create_cf("cf1", &opts).unwrap();
        let value = db.property_value_cf(cf, "rocksdb.stats").unwrap().unwrap();

        assert!(value.contains("Stats"));
    }
}

The error happen when making a method call on db that takes &self, such as db.property_value_cf() and passing in a ColumnFamily (which stores a &mut in the db field):

error[E0502]: cannot borrow `db` as immutable because it is also borrowed as mutable
  --> tests/test_property.rs:39:21
   |
38 |         let cf = db.create_cf("cf1", &opts).unwrap();
   |                  -- mutable borrow occurs here
39 |         let value = db.property_value_cf(cf, "rocksdb.stats").unwrap().unwrap();
   |                     ^^                   -- mutable borrow later used here
   |                     |
   |                     immutable borrow occurs here

I can't think of a very elegant way around this, but one option is to not return a ColumnFamily from create_cf and instead let consuming code fetch it with a call tocf_handle():

#[test]
fn property_cf_test() {
    let n = DBPath::new("_rust_rocksdb_property_cf_test");
    {
        let opts = Options::default();
        let mut db = DB::open_default(&n).unwrap();
        db.create_cf("cf1", &opts).unwrap();
        let cf = db.cf_handle("cf1").unwrap(); // Get the CF
        let value = db.property_value_cf(cf, "rocksdb.stats").unwrap().unwrap();

        assert!(value.contains("Stats"));
    }
}

Would changing create_cf() to return Result<(), Error> be an acceptable change?

[iSynaptic]

@dvdplm Adding lifetimes to ColumnFamily has ended up being a bit of a nightmare from an ergonomic perspective. I'd like to pursue an approach that doesn't need a lifetime but also doesn't run the risk of using a column family pointer after it's been deleted. That change will likely impact how we go about removing locks from cf_handle - you probably won't see this issue.

[aleksuss]

Agree with @iSynaptic. Obviously returning a reference to ColumnFamily is a safe way only in this case.

[dvdplm]

Maybe #298 can be merged then?

[aleksuss]

#314

[ordian]

Is it resolved by #314? Any plans to release if so?

[aleksuss]

Yes, it is. We plan to release it in near future.

[bkchr]

@aleksuss what is the status of releasing it? :)

[aleksuss]

@bkchr I think we will do it next week.