Support move semantics for PinnableSlice (#6374)

Summary:
It's logically correct for PinnableSlice to support move semantics to transfer ownership of the pinned memory region. This PR adds both move constructor and move assignment to PinnableSlice.
Pull Request resolved: https://github.com/facebook/rocksdb/pull/6374

Test Plan:
A set of unit tests for the move semantics are added in slice_test.
So `make slice_test && ./slice_test`.

Differential Revision: D19739254

Pulled By: cheng-chang

fbshipit-source-id: f898bd811bb05b2d87384ec58b645e9915e8e0b1

CMakeLists.txt

@@ -1033,6 +1033,7 @@ if(WITH_TESTS)
         util/random_test.cc
         util/rate_limiter_test.cc
         util/repeatable_thread_test.cc
+        util/slice_test.cc
         util/slice_transform_test.cc
         util/timer_queue_test.cc
         util/thread_list_test.cc

Makefile

@@ -503,6 +503,7 @@ TESTS = \
 	data_block_hash_index_test \
 	cache_test \
 	corruption_test \
+	slice_test \
 	slice_transform_test \
 	dbformat_test \
 	fault_injection_test \
@@ -1291,6 +1292,9 @@ corruption_test: db/corruption_test.o db/db_test_util.o $(LIBOBJECTS) $(TESTHARN
 crc32c_test: util/crc32c_test.o $(LIBOBJECTS) $(TESTHARNESS)
 	$(AM_LINK)
 
+slice_test: util/slice_test.o $(LIBOBJECTS) $(TESTHARNESS)
+	$(AM_LINK)
+
 slice_transform_test: util/slice_transform_test.o $(LIBOBJECTS) $(TESTHARNESS)
 	$(AM_LINK)
 

TARGETS

@@ -1309,6 +1309,13 @@ ROCKS_TESTS = [
         [],
         [],
     ],
+    [
+        "slice_test",
+        "util/slice_test.cc",
+        "serial",
+        [],
+        [],
+    ],
     [
         "slice_transform_test",
         "util/slice_transform_test.cc",

include/rocksdb/slice.h

@@ -147,6 +147,9 @@ class PinnableSlice : public Slice, public Cleanable {
   PinnableSlice() { buf_ = &self_space_; }
   explicit PinnableSlice(std::string* buf) { buf_ = buf; }
 
+  PinnableSlice(PinnableSlice&& other);
+  PinnableSlice& operator=(PinnableSlice&& other);
+
   // No copy constructor and copy assignment allowed.
   PinnableSlice(PinnableSlice&) = delete;
   PinnableSlice& operator=(PinnableSlice&) = delete;
@@ -214,7 +217,7 @@ class PinnableSlice : public Slice, public Cleanable {
 
   inline std::string* GetSelf() { return buf_; }
 
-  inline bool IsPinned() { return pinned_; }
+  inline bool IsPinned() const { return pinned_; }
 
  private:
   friend class PinnableSlice4Test;

src.mk

@@ -424,6 +424,7 @@ MAIN_SOURCES =                                                          \
   util/rate_limiter_test.cc                                             \
   util/random_test.cc                                                   \
   util/repeatable_thread_test.cc                                        \
+  util/slice_test.cc                                                    \
   util/slice_transform_test.cc                                          \
   util/timer_queue_test.cc                                              \
   util/thread_list_test.cc                                              \

util/slice.cc

@@ -209,4 +209,35 @@ const SliceTransform* NewNoopTransform() {
   return new NoopTransform;
 }
 
+PinnableSlice::PinnableSlice(PinnableSlice&& other) {
+  *this = std::move(other);
+}
+
+PinnableSlice& PinnableSlice::operator=(PinnableSlice&& other) {
+  if (this != &other) {
+    Cleanable::Reset();
+    Cleanable::operator=(std::move(other));
+    size_ = other.size_;
+    pinned_ = other.pinned_;
+    if (pinned_) {
+      data_ = other.data_;
+      // When it's pinned, buf should no longer be of use.
+    } else {
+      if (other.buf_ == &other.self_space_) {
+        self_space_ = std::move(other.self_space_);
+        buf_ = &self_space_;
+        data_ = buf_->data();
+      } else {
+        buf_ = other.buf_;
+        data_ = other.data_;
+      }
+    }
+    other.self_space_.clear();
+    other.buf_ = &other.self_space_;
+    other.pinned_ = false;
+    other.PinSelf();
+  }
+  return *this;
+}
+
 }  // namespace rocksdb

util/slice_test.cc

@@ -0,0 +1,175 @@
+//  Copyright (c) 2011-present, Facebook, Inc.  All rights reserved.
+//  This source code is licensed under both the GPLv2 (found in the
+//  COPYING file in the root directory) and Apache 2.0 License
+//  (found in the LICENSE.Apache file in the root directory).
+
+#include "port/port.h"
+#include "port/stack_trace.h"
+#include "rocksdb/slice.h"
+#include "test_util/testharness.h"
+#include "test_util/testutil.h"
+
+namespace rocksdb {
+
+// Use this to keep track of the cleanups that were actually performed
+void Multiplier(void* arg1, void* arg2) {
+  int* res = reinterpret_cast<int*>(arg1);
+  int* num = reinterpret_cast<int*>(arg2);
+  *res *= *num;
+}
+
+class PinnableSliceTest : public testing::Test {
+ public:
+  void AssertEmpty(const PinnableSlice& slice) {
+    ASSERT_EQ(0, slice.size());
+    ASSERT_FALSE(slice.IsPinned());
+  }
+
+  void AssertSameData(const std::string& expected,
+                      const PinnableSlice& slice) {
+    std::string got;
+    got.assign(slice.data(), slice.size());
+    ASSERT_EQ(expected, got);
+  }
+
+  // Asserts that pinnable is in a clean state after being moved to
+  // another PinnableSlice.
+  // It asserts by trying to pin the slice.
+  void AssertCleanState(PinnableSlice& pinnable, const Slice& slice) {
+    AssertEmpty(pinnable);
+
+    pinnable.PinSelf(slice);
+    AssertSameData(slice.ToString(), pinnable);
+
+    int res = 1;
+    int n2 = 2;
+    pinnable.PinSlice(slice, Multiplier, &res, &n2);
+    AssertSameData(slice.ToString(), pinnable);
+    ASSERT_EQ(1, res);
+    pinnable.Reset();
+    ASSERT_EQ(2, res);
+  }
+};
+
+TEST_F(PinnableSliceTest, Move) {
+  int n2 = 2;
+  int res = 1;
+  const std::string const_str1 = "123";
+  const std::string const_str2 = "ABC";
+  Slice slice1(const_str1);
+  Slice slice2(const_str2);
+
+  {
+    // Test move constructor on a pinned slice.
+    res = 1;
+    PinnableSlice v1;
+    v1.PinSlice(slice1, Multiplier, &res, &n2);
+    PinnableSlice v2(std::move(v1));
+
+    // Since v1's Cleanable has been moved to v2,
+    // no cleanup should happen in Reset.
+    v1.Reset();
+    ASSERT_EQ(1, res);
+
+    AssertSameData(const_str1, v2);
+    AssertCleanState(v1, slice2);
+  }
+  // v2 is cleaned up.
+  ASSERT_EQ(2, res);
+
+  {
+    // Test move constructor on an unpinned slice.
+    PinnableSlice v1;
+    v1.PinSelf(slice1);
+    PinnableSlice v2(std::move(v1));
+
+    AssertSameData(const_str1, v2);
+    AssertCleanState(v1, slice2);
+  }
+
+  {
+    // Test move assignment from a pinned slice to
+    // another pinned slice.
+    res = 1;
+    PinnableSlice v1;
+    v1.PinSlice(slice1, Multiplier, &res, &n2);
+    PinnableSlice v2;
+    v2.PinSlice(slice2, Multiplier, &res, &n2);
+    v2 = std::move(v1);
+
+    // v2's Cleanable will be Reset before moving
+    // anything from v1.
+    ASSERT_EQ(2, res);
+    // Since v1's Cleanable has been moved to v2,
+    // no cleanup should happen in Reset.
+    v1.Reset();
+    ASSERT_EQ(2, res);
+
+    AssertSameData(const_str1, v2);
+    AssertCleanState(v1, slice2);
+  }
+  // The Cleanable moved from v1 to v2 will be Reset.
+  ASSERT_EQ(4, res);
+
+  {
+    // Test move assignment from a pinned slice to
+    // an unpinned slice.
+    res = 1;
+    PinnableSlice v1;
+    v1.PinSlice(slice1, Multiplier, &res, &n2);
+    PinnableSlice v2;
+    v2.PinSelf(slice2);
+    v2 = std::move(v1);
+
+    // Since v1's Cleanable has been moved to v2,
+    // no cleanup should happen in Reset.
+    v1.Reset();
+    ASSERT_EQ(1, res);
+
+    AssertSameData(const_str1, v2);
+    AssertCleanState(v1, slice2);
+  }
+  // The Cleanable moved from v1 to v2 will be Reset.
+  ASSERT_EQ(2, res);
+
+  {
+    // Test move assignment from an upinned slice to
+    // another unpinned slice.
+    PinnableSlice v1;
+    v1.PinSelf(slice1);
+    PinnableSlice v2;
+    v2.PinSelf(slice2);
+    v2 = std::move(v1);
+
+    AssertSameData(const_str1, v2);
+    AssertCleanState(v1, slice2);
+  }
+
+  {
+    // Test move assignment from an upinned slice to
+    // a pinned slice.
+    res = 1;
+    PinnableSlice v1;
+    v1.PinSelf(slice1);
+    PinnableSlice v2;
+    v2.PinSlice(slice2, Multiplier, &res, &n2);
+    v2 = std::move(v1);
+
+    // v2's Cleanable will be Reset before moving
+    // anything from v1.
+    ASSERT_EQ(2, res);
+
+    AssertSameData(const_str1, v2);
+    AssertCleanState(v1, slice2);
+  }
+  // No Cleanable is moved from v1 to v2, so no more cleanup.
+  ASSERT_EQ(2, res);
+}
+
+}  // namespace rocksdb
+
+int main(int argc, char** argv) {
+  rocksdb::port::InstallStackTraceHandler();
+  ::testing::InitGoogleTest(&argc, argv);
+  return RUN_ALL_TESTS();
+}