Merge pull request #346 from nbdd0121/raw_iter

Avoid copy in DBRawIterator::{key, value}

src/db.rs

@@ -488,53 +488,39 @@ impl<'a> DBRawIterator<'a> {
         }
     }
 
-    /// Returns a slice to the internal buffer storing the current key.
-    ///
-    /// This may be slightly more performant to use than the standard ``.key()`` method
-    /// as it does not copy the key. However, you must be careful to not use the buffer
-    /// if the iterator's seek position is ever moved by any of the seek commands or the
-    /// ``.next()`` and ``.previous()`` methods as the underlying buffer may be reused
-    /// for something else or freed entirely.
-    pub unsafe fn key_inner(&self) -> Option<&[u8]> {
+    /// Returns a slice of the current key.
+    pub fn key(&self) -> Option<&[u8]> {
         if self.valid() {
+            // Safety Note: This is safe as all methods that may invalidate the buffer returned
+            // take `&mut self`, so borrow checker will prevent use of buffer after seek.
+            unsafe {
                 let mut key_len: size_t = 0;
                 let key_len_ptr: *mut size_t = &mut key_len;
                 let key_ptr = ffi::rocksdb_iter_key(self.inner, key_len_ptr) as *const c_uchar;
 
                 Some(slice::from_raw_parts(key_ptr, key_len as usize))
+            }
         } else {
             None
         }
     }
 
-    /// Returns a copy of the current key.
-    pub fn key(&self) -> Option<Vec<u8>> {
-        unsafe { self.key_inner().map(|key| key.to_vec()) }
-    }
-
-    /// Returns a slice to the internal buffer storing the current value.
-    ///
-    /// This may be slightly more performant to use than the standard ``.value()`` method
-    /// as it does not copy the value. However, you must be careful to not use the buffer
-    /// if the iterator's seek position is ever moved by any of the seek commands or the
-    /// ``.next()`` and ``.previous()`` methods as the underlying buffer may be reused
-    /// for something else or freed entirely.
-    pub unsafe fn value_inner(&self) -> Option<&[u8]> {
+    /// Returns a slice of the current value.
+    pub fn value(&self) -> Option<&[u8]> {
         if self.valid() {
+            // Safety Note: This is safe as all methods that may invalidate the buffer returned
+            // take `&mut self`, so borrow checker will prevent use of buffer after seek.
+            unsafe {
                 let mut val_len: size_t = 0;
                 let val_len_ptr: *mut size_t = &mut val_len;
                 let val_ptr = ffi::rocksdb_iter_value(self.inner, val_len_ptr) as *const c_uchar;
 
                 Some(slice::from_raw_parts(val_ptr, val_len as usize))
+            }
         } else {
             None
         }
     }
-
-    /// Returns a copy of the current value.
-    pub fn value(&self) -> Option<Vec<u8>> {
-        unsafe { self.value_inner().map(|value| value.to_vec()) }
-    }
 }
 
 impl<'a> Drop for DBRawIterator<'a> {
@@ -623,8 +609,8 @@ impl<'a> Iterator for DBIterator<'a> {
         if self.raw.valid() {
             // .key() and .value() only ever return None if valid == false, which we've just cheked
             Some((
-                self.raw.key().unwrap().into_boxed_slice(),
-                self.raw.value().unwrap().into_boxed_slice(),
+                self.raw.key().unwrap().to_vec().into_boxed_slice(),
+                self.raw.value().unwrap().to_vec().into_boxed_slice(),
             ))
         } else {
             None
@@ -2031,6 +2017,7 @@ unsafe impl Send for ReadOptions {}
 
 // Sync is similarly safe for many types because they do not expose interior mutability, and their
 // use within the rocksdb library is generally behind a const reference
+unsafe impl<'a> Sync for DBRawIterator<'a> {}
 unsafe impl Sync for ReadOptions {}
 
 /// Vector of bytes stored in the database.

tests/test_raw_iterator.rs

@@ -32,14 +32,14 @@ pub fn test_forwards_iteration() {
         iter.seek_to_first();
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k1".to_vec()));
-        assert_eq!(iter.value(), Some(b"v1".to_vec()));
+        assert_eq!(iter.key(), Some(b"k1".as_ref()));
+        assert_eq!(iter.value(), Some(b"v1".as_ref()));
 
         iter.next();
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k2".to_vec()));
-        assert_eq!(iter.value(), Some(b"v2".to_vec()));
+        assert_eq!(iter.key(), Some(b"k2".as_ref()));
+        assert_eq!(iter.value(), Some(b"v2".as_ref()));
 
         iter.next(); // k3
         iter.next(); // k4
@@ -65,14 +65,14 @@ pub fn test_seek_last() {
         iter.seek_to_last();
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k4".to_vec()));
-        assert_eq!(iter.value(), Some(b"v4".to_vec()));
+        assert_eq!(iter.key(), Some(b"k4".as_ref()));
+        assert_eq!(iter.value(), Some(b"v4".as_ref()));
 
         iter.prev();
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k3".to_vec()));
-        assert_eq!(iter.value(), Some(b"v3".to_vec()));
+        assert_eq!(iter.key(), Some(b"k3".as_ref()));
+        assert_eq!(iter.value(), Some(b"v3".as_ref()));
 
         iter.prev(); // k2
         iter.prev(); // k1
@@ -97,15 +97,15 @@ pub fn test_seek() {
         iter.seek(b"k2");
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k2".to_vec()));
-        assert_eq!(iter.value(), Some(b"v2".to_vec()));
+        assert_eq!(iter.key(), Some(b"k2".as_ref()));
+        assert_eq!(iter.value(), Some(b"v2".as_ref()));
 
         // Check it gets the next key when the key doesn't exist
         iter.seek(b"k3");
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k4".to_vec()));
-        assert_eq!(iter.value(), Some(b"v4".to_vec()));
+        assert_eq!(iter.key(), Some(b"k4".as_ref()));
+        assert_eq!(iter.value(), Some(b"v4".as_ref()));
     }
 }
 
@@ -122,8 +122,8 @@ pub fn test_seek_to_nonexistant() {
         iter.seek(b"k2");
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k3".to_vec()));
-        assert_eq!(iter.value(), Some(b"v3".to_vec()));
+        assert_eq!(iter.key(), Some(b"k3".as_ref()));
+        assert_eq!(iter.value(), Some(b"v3".as_ref()));
     }
 }
 
@@ -140,14 +140,14 @@ pub fn test_seek_for_prev() {
         iter.seek(b"k2");
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k2".to_vec()));
-        assert_eq!(iter.value(), Some(b"v2".to_vec()));
+        assert_eq!(iter.key(), Some(b"k2".as_ref()));
+        assert_eq!(iter.value(), Some(b"v2".as_ref()));
 
         // Check it gets the previous key when the key doesn't exist
         iter.seek_for_prev(b"k3");
 
         assert_eq!(iter.valid(), true);
-        assert_eq!(iter.key(), Some(b"k2".to_vec()));
-        assert_eq!(iter.value(), Some(b"v2".to_vec()));
+        assert_eq!(iter.key(), Some(b"k2".as_ref()));
+        assert_eq!(iter.value(), Some(b"v2".as_ref()));
     }
 }