From a651b19aa52bbfacda26cedc55a367f241e342f5 Mon Sep 17 00:00:00 2001 From: Xuejie Xiao Date: Fri, 8 Feb 2019 11:58:23 +0800 Subject: [PATCH] Code review fixes --- src/db.rs | 58 +++++++++++++++++++++++++----------- src/lib.rs | 4 +-- tests/test_pinnable_slice.rs | 10 +++---- 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/src/db.rs b/src/db.rs index 430609b..d56e459 100644 --- a/src/db.rs +++ b/src/db.rs @@ -938,14 +938,20 @@ impl DB { self.get_cf_opt(cf, key.as_ref(), &ReadOptions::default()) } - pub fn get_pinned_opt>(&self, key: T, readopts: &ReadOptions) -> Result, Error> { + /// Return the value associated with a key using RocksDB's PinnableSlice + /// so as to avoid unnecessary memory copy. + pub fn get_pinned_opt>( + &self, + key: K, + readopts: &ReadOptions, + ) -> Result, Error> { if readopts.inner.is_null() { return Err(Error::new( "Unable to create RocksDB read options. \ - This is a fairly trivial call, and its \ - failure may be indicative of a \ - mis-compiled or mis-loaded RocksDB \ - library." + This is a fairly trivial call, and its \ + failure may be indicative of a \ + mis-compiled or mis-loaded RocksDB \ + library." .to_owned(), )); } @@ -966,23 +972,29 @@ impl DB { } } - pub fn get_pinned>(&self, key: T) -> Result, Error> { + /// Return the value associated with a key using RocksDB's PinnableSlice + /// so as to avoid unnecessary memory copy. Similar to get_pinned_opt but + /// leverages default options. + pub fn get_pinned>(&self, key: K) -> Result, Error> { self.get_pinned_opt(key, &ReadOptions::default()) } - pub fn get_pinned_cf_opt>( + /// Return the value associated with a key using RocksDB's PinnableSlice + /// so as to avoid unnecessary memory copy. Similar to get_pinned_opt but + /// allows specifying ColumnFamily + pub fn get_pinned_cf_opt>( &self, cf: ColumnFamily, - key: T, + key: K, readopts: &ReadOptions, ) -> Result, Error> { if readopts.inner.is_null() { return Err(Error::new( "Unable to create RocksDB read options. \ - This is a fairly trivial call, and its \ - failure may be indicative of a \ - mis-compiled or mis-loaded RocksDB \ - library." + This is a fairly trivial call, and its \ + failure may be indicative of a \ + mis-compiled or mis-loaded RocksDB \ + library." .to_owned(), )); } @@ -1004,7 +1016,14 @@ impl DB { } } - pub fn get_pinned_cf>(&self, cf: ColumnFamily, key: T) -> Result, Error> { + /// Return the value associated with a key using RocksDB's PinnableSlice + /// so as to avoid unnecessary memory copy. Similar to get_pinned_cf_opt but + /// leverages default options. + pub fn get_pinned_cf>( + &self, + cf: ColumnFamily, + key: K, + ) -> Result, Error> { self.get_pinned_cf_opt(cf, key, &ReadOptions::default()) } @@ -1667,9 +1686,14 @@ fn to_cpath>(path: P) -> Result { } } +/// Wrapper around RocksDB PinnableSlice struct. +/// +/// With a pinnable slice, we can directly leverage in-memory data within +/// RocksDB toa void unnecessary memory copies. The struct here wraps the +/// returned raw pointer and ensures proper finalization work. pub struct DBPinnableSlice<'a> { ptr: *mut ffi::rocksdb_pinnableslice_t, - db: PhantomData<&'a DB> + db: PhantomData<&'a DB>, } impl<'a> AsRef<[u8]> for DBPinnableSlice<'a> { @@ -1685,8 +1709,8 @@ impl<'a> Deref for DBPinnableSlice<'a> { fn deref(&self) -> &[u8] { unsafe { let mut val_len: size_t = 0; - let val = ffi::rocksdb_pinnableslice_value(self.ptr, &mut val_len,) as *mut u8; - slice::from_raw_parts(val, val_len,) + let val = ffi::rocksdb_pinnableslice_value(self.ptr, &mut val_len) as *mut u8; + slice::from_raw_parts(val, val_len) } } } @@ -1694,7 +1718,7 @@ impl<'a> Deref for DBPinnableSlice<'a> { impl<'a> Drop for DBPinnableSlice<'a> { fn drop(&mut self) { unsafe { - ffi::rocksdb_pinnableslice_destroy(self.ptr,); + ffi::rocksdb_pinnableslice_destroy(self.ptr); } } } diff --git a/src/lib.rs b/src/lib.rs index ec60c7c..caf2d20 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -71,8 +71,8 @@ mod slice_transform; pub use compaction_filter::Decision as CompactionDecision; pub use db::{ - new_bloom_filter, DBCompactionStyle, DBCompressionType, DBIterator, DBRawIterator, DBPinnableSlice, DBRecoveryMode, DBVector, - Direction, IteratorMode, ReadOptions, Snapshot, WriteBatch, + DBCompactionStyle, DBCompressionType, DBIterator, DBPinnableSlice, DBRawIterator, + DBRecoveryMode, DBVector, Direction, IteratorMode, ReadOptions, Snapshot, WriteBatch, }; pub use slice_transform::SliceTransform; diff --git a/tests/test_pinnable_slice.rs b/tests/test_pinnable_slice.rs index 35e0c63..be3db7a 100644 --- a/tests/test_pinnable_slice.rs +++ b/tests/test_pinnable_slice.rs @@ -3,16 +3,16 @@ extern crate rocksdb; use rocksdb::{Options, DB}; #[test] -pub fn test_pinnable_slice() { +fn test_pinnable_slice() { let path = "_rust_rocksdb_pinnable_slice_test"; let mut opts = Options::default(); - opts.create_if_missing(true,); - let db = DB::open(&opts, path,).unwrap(); + opts.create_if_missing(true); + let db = DB::open(&opts, path).unwrap(); - db.put(b"k1", b"value12345",).unwrap(); + db.put(b"k1", b"value12345").unwrap(); - let result = db.get_pinned(b"k1",); + let result = db.get_pinned(b"k1"); assert!(result.is_ok()); let value = result.unwrap();