[kvdb-rocksdb] Use "pinned" gets to avoid allocations (#274)

* Use "pinned" gets to avoid allocations

Needs benchmarks to prove it actually matters.

* Fix test

* Rename `get_colf` to just `cf`
Add todos to measure `#[inline]`

* Formatting

* Using #[inline] does not help read perf

* Add Changelog

* Update CHANGELOG.md

kvdb-rocksdb/CHANGELOG.md

@@ -5,6 +5,9 @@ The format is based on [Keep a Changelog].
 [Keep a Changelog]: http://keepachangelog.com/en/1.0.0/
 
 ## [Unreleased]
+- use `get_pinned` API to save one allocation for each call to `get()` (See [PR #274](https://github.com/paritytech/parity-common/pull/274) for details)
+- rename `drop_column` to `remove_last_column` (See [PR #274](https://github.com/paritytech/parity-common/pull/274) for details)
+- rename `get_cf` to `cf` (See [PR #274](https://github.com/paritytech/parity-common/pull/274) for details)
 
 ## [0.2.0] - 2019-11-28
 - Switched away from using [parity-rocksdb](https://crates.io/crates/parity-rocksdb) in favour of upstream [rust-rocksdb](https://crates.io/crates/rocksdb) (see [PR #257](https://github.com/paritytech/parity-common/pull/257) for details)

kvdb-rocksdb/src/iter.rs

@@ -111,18 +111,14 @@ impl<'a> IterationHandler for &'a DBAndColumns {
 	fn iter(&self, col: Option<u32>) -> Self::Iterator {
 		col.map_or_else(
 			|| self.db.iterator(IteratorMode::Start),
-			|c| {
-				self.db
-					.iterator_cf(self.get_cf(c as usize), IteratorMode::Start)
-					.expect("iterator params are valid; qed")
-			},
+			|c| self.db.iterator_cf(self.cf(c as usize), IteratorMode::Start).expect("iterator params are valid; qed"),
 		)
 	}
 
 	fn iter_from_prefix(&self, col: Option<u32>, prefix: &[u8]) -> Self::Iterator {
 		col.map_or_else(
 			|| self.db.prefix_iterator(prefix),
-			|c| self.db.prefix_iterator_cf(self.get_cf(c as usize), prefix).expect("iterator params are valid; qed"),
+			|c| self.db.prefix_iterator_cf(self.cf(c as usize), prefix).expect("iterator params are valid; qed"),
 		)
 	}
 }

kvdb-rocksdb/src/lib.rs

@@ -226,7 +226,7 @@ struct DBAndColumns {
 }
 
 impl DBAndColumns {
-	fn get_cf(&self, i: usize) -> &ColumnFamily {
+	fn cf(&self, i: usize) -> &ColumnFamily {
 		self.db.cf_handle(&self.column_names[i]).expect("the specified column name is correct; qed")
 	}
 }
@@ -438,15 +438,15 @@ impl Database {
 							match *state {
 								KeyState::Delete => {
 									if c > 0 {
-										let cf = cfs.get_cf(c - 1);
+										let cf = cfs.cf(c - 1);
 										batch.delete_cf(cf, key).map_err(other_io_err)?;
 									} else {
 										batch.delete(key).map_err(other_io_err)?;
 									}
 								}
 								KeyState::Insert(ref value) => {
 									if c > 0 {
-										let cf = cfs.get_cf(c - 1);
+										let cf = cfs.cf(c - 1);
 										batch.put_cf(cf, key, value).map_err(other_io_err)?;
 									} else {
 										batch.put(key, value).map_err(other_io_err)?;
@@ -497,11 +497,11 @@ impl Database {
 					match op {
 						DBOp::Insert { col, key, value } => match col {
 							None => batch.put(&key, &value).map_err(other_io_err)?,
-							Some(c) => batch.put_cf(cfs.get_cf(c as usize), &key, &value).map_err(other_io_err)?,
+							Some(c) => batch.put_cf(cfs.cf(c as usize), &key, &value).map_err(other_io_err)?,
 						},
 						DBOp::Delete { col, key } => match col {
 							None => batch.delete(&key).map_err(other_io_err)?,
-							Some(c) => batch.delete_cf(cfs.get_cf(c as usize), &key).map_err(other_io_err)?,
+							Some(c) => batch.delete_cf(cfs.cf(c as usize), &key).map_err(other_io_err)?,
 						},
 					}
 				}
@@ -527,10 +527,14 @@ impl Database {
 							Some(&KeyState::Delete) => Ok(None),
 							None => col
 								.map_or_else(
-									|| cfs.db.get_opt(key, &self.read_opts).map(|r| r.map(|v| DBValue::from_slice(&v))),
+									|| {
+										cfs.db
+											.get_pinned_opt(key, &self.read_opts)
+											.map(|r| r.map(|v| DBValue::from_slice(&v)))
+									},
 									|c| {
 										cfs.db
-											.get_cf_opt(cfs.get_cf(c as usize), key, &self.read_opts)
+											.get_pinned_cf_opt(cfs.cf(c as usize), key, &self.read_opts)
 											.map(|r| r.map(|v| DBValue::from_slice(&v)))
 									},
 								)
@@ -650,8 +654,8 @@ impl Database {
 			.unwrap_or(0)
 	}
 
-	/// Drop a column family.
-	pub fn drop_column(&self) -> io::Result<()> {
+	/// Remove the last column family in the database. The deletion is definitive.
+	pub fn remove_last_column(&self) -> io::Result<()> {
 		match *self.db.write() {
 			Some(DBAndColumns { ref mut db, ref mut column_names }) => {
 				if let Some(name) = column_names.pop() {
@@ -663,7 +667,7 @@ impl Database {
 		}
 	}
 
-	/// Add a column family.
+	/// Add a new column family to the DB.
 	pub fn add_column(&self) -> io::Result<()> {
 		match *self.db.write() {
 			Some(DBAndColumns { ref mut db, ref mut column_names }) => {
@@ -848,7 +852,7 @@ mod tests {
 	}
 
 	#[test]
-	fn drop_columns() {
+	fn remove_columns() {
 		let config = DatabaseConfig::default();
 		let config_5 = DatabaseConfig::with_columns(Some(5));
 
@@ -860,7 +864,7 @@ mod tests {
 			assert_eq!(db.num_columns(), 5);
 
 			for i in (0..5).rev() {
-				db.drop_column().unwrap();
+				db.remove_last_column().unwrap();
 				assert_eq!(db.num_columns(), i);
 			}
 		}