From a09549015d82d4db556cb7e3b07004e8a515ec94 Mon Sep 17 00:00:00 2001 From: David Date: Thu, 5 Dec 2019 10:52:58 +0100 Subject: [PATCH] [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 | 3 +++ kvdb-rocksdb/src/iter.rs | 8 ++------ kvdb-rocksdb/src/lib.rs | 28 ++++++++++++++++------------ 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/kvdb-rocksdb/CHANGELOG.md b/kvdb-rocksdb/CHANGELOG.md index 7e6565d65..d673404cb 100644 --- a/kvdb-rocksdb/CHANGELOG.md +++ b/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) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index 52934e1a8..079563aad 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -111,18 +111,14 @@ impl<'a> IterationHandler for &'a DBAndColumns { fn iter(&self, col: Option) -> 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, 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"), ) } } diff --git a/kvdb-rocksdb/src/lib.rs b/kvdb-rocksdb/src/lib.rs index 3a0905273..df7b9f833 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/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,7 +438,7 @@ 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)?; @@ -446,7 +446,7 @@ impl Database { } 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); } }