From fffa3aed6282549c8843b2d225cdf4eb83c87fbe Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 28 Nov 2019 17:22:36 +0100 Subject: [PATCH 1/7] Use "pinned" gets to avoid allocations Needs benchmarks to prove it actually matters. --- kvdb-rocksdb/src/iter.rs | 4 ++-- kvdb-rocksdb/src/lib.rs | 22 ++++++++++++---------- 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index 52934e1a8..67537514c 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -113,7 +113,7 @@ impl<'a> IterationHandler for &'a DBAndColumns { || self.db.iterator(IteratorMode::Start), |c| { self.db - .iterator_cf(self.get_cf(c as usize), IteratorMode::Start) + .iterator_cf(self.get_colf(c as usize), IteratorMode::Start) .expect("iterator params are valid; qed") }, ) @@ -122,7 +122,7 @@ impl<'a> IterationHandler for &'a DBAndColumns { 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.get_colf(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..ff2912020 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/kvdb-rocksdb/src/lib.rs @@ -226,7 +226,8 @@ struct DBAndColumns { } impl DBAndColumns { - fn get_cf(&self, i: usize) -> &ColumnFamily { + #[inline] + fn get_colf(&self, i: usize) -> &ColumnFamily { self.db.cf_handle(&self.column_names[i]).expect("the specified column name is correct; qed") } } @@ -404,6 +405,7 @@ impl Database { DBTransaction::new() } + #[inline] fn to_overlay_column(col: Option) -> usize { col.map_or(0, |c| (c + 1) as usize) } @@ -438,7 +440,7 @@ impl Database { match *state { KeyState::Delete => { if c > 0 { - let cf = cfs.get_cf(c - 1); + let cf = cfs.get_colf(c - 1); batch.delete_cf(cf, key).map_err(other_io_err)?; } else { batch.delete(key).map_err(other_io_err)?; @@ -446,7 +448,7 @@ impl Database { } KeyState::Insert(ref value) => { if c > 0 { - let cf = cfs.get_cf(c - 1); + let cf = cfs.get_colf(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 +499,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.get_colf(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.get_colf(c as usize), &key).map_err(other_io_err)?, }, } } @@ -527,10 +529,10 @@ 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.get_colf(c as usize), key, &self.read_opts) .map(|r| r.map(|v| DBValue::from_slice(&v))) }, ) @@ -650,8 +652,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 +665,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 }) => { From 88dd216d1acc6d4fad9f899ff39f12b7e790befd Mon Sep 17 00:00:00 2001 From: David Palm Date: Thu, 28 Nov 2019 21:01:22 +0100 Subject: [PATCH 2/7] Fix test --- kvdb-rocksdb/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/kvdb-rocksdb/src/lib.rs b/kvdb-rocksdb/src/lib.rs index ff2912020..2887d755f 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/kvdb-rocksdb/src/lib.rs @@ -850,7 +850,7 @@ mod tests { } #[test] - fn drop_columns() { + fn remove_columns() { let config = DatabaseConfig::default(); let config_5 = DatabaseConfig::with_columns(Some(5)); @@ -862,7 +862,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); } } From 835122ee358b823cd3cbd706da965e7bc00c7ff6 Mon Sep 17 00:00:00 2001 From: David Palm Date: Fri, 29 Nov 2019 11:55:38 +0100 Subject: [PATCH 3/7] Rename `get_colf` to just `cf` Add todos to measure `#[inline]` --- kvdb-rocksdb/src/iter.rs | 4 ++-- kvdb-rocksdb/src/lib.rs | 18 +++++++++--------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index 67537514c..4e639936e 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -113,7 +113,7 @@ impl<'a> IterationHandler for &'a DBAndColumns { || self.db.iterator(IteratorMode::Start), |c| { self.db - .iterator_cf(self.get_colf(c as usize), IteratorMode::Start) + .iterator_cf(self.cf(c as usize), IteratorMode::Start) .expect("iterator params are valid; qed") }, ) @@ -122,7 +122,7 @@ impl<'a> IterationHandler for &'a DBAndColumns { 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_colf(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 2887d755f..5af475667 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/kvdb-rocksdb/src/lib.rs @@ -226,8 +226,8 @@ struct DBAndColumns { } impl DBAndColumns { - #[inline] - fn get_colf(&self, i: usize) -> &ColumnFamily { + #[inline] // todo[dvdplm] measure + fn cf(&self, i: usize) -> &ColumnFamily { self.db.cf_handle(&self.column_names[i]).expect("the specified column name is correct; qed") } } @@ -249,7 +249,7 @@ pub struct Database { flushing_lock: Mutex, } -#[inline] +#[inline] // todo[dvdplm] measure fn check_for_corruption>(path: P, res: result::Result) -> io::Result { if let Err(ref s) = res { if is_corrupted(s) { @@ -405,7 +405,7 @@ impl Database { DBTransaction::new() } - #[inline] + #[inline] // todo[dvdplm] measure fn to_overlay_column(col: Option) -> usize { col.map_or(0, |c| (c + 1) as usize) } @@ -440,7 +440,7 @@ impl Database { match *state { KeyState::Delete => { if c > 0 { - let cf = cfs.get_colf(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)?; @@ -448,7 +448,7 @@ impl Database { } KeyState::Insert(ref value) => { if c > 0 { - let cf = cfs.get_colf(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)?; @@ -499,11 +499,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_colf(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_colf(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)?, }, } } @@ -532,7 +532,7 @@ impl Database { || cfs.db.get_pinned_opt(key, &self.read_opts).map(|r| r.map(|v| DBValue::from_slice(&v))), |c| { cfs.db - .get_pinned_cf_opt(cfs.get_colf(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))) }, ) From 8cc3f5b8899f40a2cb6a9c5aeb122bbbc160999e Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 4 Dec 2019 12:25:20 +0100 Subject: [PATCH 4/7] Formatting --- kvdb-rocksdb/src/iter.rs | 6 +----- kvdb-rocksdb/src/lib.rs | 6 +++++- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/kvdb-rocksdb/src/iter.rs b/kvdb-rocksdb/src/iter.rs index 4e639936e..079563aad 100644 --- a/kvdb-rocksdb/src/iter.rs +++ b/kvdb-rocksdb/src/iter.rs @@ -111,11 +111,7 @@ 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.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"), ) } diff --git a/kvdb-rocksdb/src/lib.rs b/kvdb-rocksdb/src/lib.rs index 5af475667..824f751b6 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/kvdb-rocksdb/src/lib.rs @@ -529,7 +529,11 @@ impl Database { Some(&KeyState::Delete) => Ok(None), None => col .map_or_else( - || cfs.db.get_pinned_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_pinned_cf_opt(cfs.cf(c as usize), key, &self.read_opts) From 1f1611d6261b80a2adbf23553cf62c899dc0ddc4 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 4 Dec 2019 14:14:36 +0100 Subject: [PATCH 5/7] Using #[inline] does not help read perf --- kvdb-rocksdb/src/lib.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/kvdb-rocksdb/src/lib.rs b/kvdb-rocksdb/src/lib.rs index 824f751b6..df7b9f833 100644 --- a/kvdb-rocksdb/src/lib.rs +++ b/kvdb-rocksdb/src/lib.rs @@ -226,7 +226,6 @@ struct DBAndColumns { } impl DBAndColumns { - #[inline] // todo[dvdplm] measure fn cf(&self, i: usize) -> &ColumnFamily { self.db.cf_handle(&self.column_names[i]).expect("the specified column name is correct; qed") } @@ -249,7 +248,7 @@ pub struct Database { flushing_lock: Mutex, } -#[inline] // todo[dvdplm] measure +#[inline] fn check_for_corruption>(path: P, res: result::Result) -> io::Result { if let Err(ref s) = res { if is_corrupted(s) { @@ -405,7 +404,6 @@ impl Database { DBTransaction::new() } - #[inline] // todo[dvdplm] measure fn to_overlay_column(col: Option) -> usize { col.map_or(0, |c| (c + 1) as usize) } From d3a716a301fbed8a3d13846d66ab05db1c3a34d1 Mon Sep 17 00:00:00 2001 From: David Palm Date: Wed, 4 Dec 2019 14:24:01 +0100 Subject: [PATCH 6/7] Add Changelog --- kvdb-rocksdb/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kvdb-rocksdb/CHANGELOG.md b/kvdb-rocksdb/CHANGELOG.md index 7e6565d65..9f72a344b 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()` +- rename `drop_column` to `remove_last_column` +- rename `get_cf` to `cf` ## [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) From af57335db07468c5e666e2b17dd621280d8bf038 Mon Sep 17 00:00:00 2001 From: David Date: Wed, 4 Dec 2019 16:54:47 +0100 Subject: [PATCH 7/7] Update CHANGELOG.md --- kvdb-rocksdb/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/kvdb-rocksdb/CHANGELOG.md b/kvdb-rocksdb/CHANGELOG.md index 9f72a344b..d673404cb 100644 --- a/kvdb-rocksdb/CHANGELOG.md +++ b/kvdb-rocksdb/CHANGELOG.md @@ -5,9 +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()` -- rename `drop_column` to `remove_last_column` -- rename `get_cf` to `cf` +- 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)