Blob DB: not using PinnableSlice move assignment

Summary:
The current implementation of PinnableSlice move assignment have an issue #3163. We are moving away from it instead of try to get the move assignment right, since it is too tricky.
Closes https://github.com/facebook/rocksdb/pull/3164

Differential Revision: D6319201

Pulled By: yiwu-arbug

fbshipit-source-id: 8f3279021f3710da4a4caa14fd238ed2df902c48

CMakeLists.txt

@@ -811,7 +811,6 @@ if(WITH_TESTS)
         util/hash_test.cc
         util/heap_test.cc
         util/rate_limiter_test.cc
-        util/slice_test.cc
         util/slice_transform_test.cc
         util/timer_queue_test.cc
         util/thread_list_test.cc

Makefile

@@ -494,7 +494,6 @@ TESTS = \
 	repair_test \
 	env_timed_test \
 	write_prepared_transaction_test \
-	slice_test \
 
 PARALLEL_TEST = \
 	backupable_db_test \
@@ -1478,9 +1477,6 @@ range_del_aggregator_test: db/range_del_aggregator_test.o db/db_test_util.o $(LI
 blob_db_test: utilities/blob_db/blob_db_test.o $(LIBOBJECTS) $(TESTHARNESS)
 	$(AM_LINK)
 
-slice_test: util/slice_test.o $(LIBOBJECTS) $(TESTHARNESS)
-	$(AM_LINK)
-
 #-------------------------------------------------
 # make install related stuff
 INSTALL_PATH ?= /usr/local

TARGETS

@@ -464,7 +464,6 @@ ROCKS_TESTS = [['arena_test', 'util/arena_test.cc', 'serial'],
  ['repair_test', 'db/repair_test.cc', 'serial'],
  ['sim_cache_test', 'utilities/simulator_cache/sim_cache_test.cc', 'serial'],
  ['skiplist_test', 'memtable/skiplist_test.cc', 'serial'],
- ['slice_test', 'util/slice_test.cc', 'serial'],
  ['slice_transform_test', 'util/slice_transform_test.cc', 'serial'],
  ['spatial_db_test', 'utilities/spatialdb/spatial_db_test.cc', 'serial'],
  ['sst_dump_test', 'tools/sst_dump_test.cc', 'serial'],

include/rocksdb/slice.h

@@ -133,31 +133,6 @@ class PinnableSlice : public Slice, public Cleanable {
   PinnableSlice(PinnableSlice&) = delete;
   PinnableSlice& operator=(PinnableSlice&) = delete;
 
-  PinnableSlice(PinnableSlice&& other) { *this = std::move(other); }
-
-  PinnableSlice& operator=(PinnableSlice&& other) {
-    if (this != &other) {
-      // cleanup itself.
-      Reset();
-
-      Slice::operator=(other);
-      Cleanable::operator=(std::move(other));
-      pinned_ = other.pinned_;
-      if (!pinned_ && other.buf_ == &other.self_space_) {
-        self_space_ = std::move(other.self_space_);
-        buf_ = &self_space_;
-        data_ = buf_->data();
-      } else {
-        buf_ = other.buf_;
-      }
-      // Re-initialize the other PinnablaeSlice.
-      other.self_space_.clear();
-      other.buf_ = &other.self_space_;
-      other.pinned_ = false;
-    }
-    return *this;
-  }
-
   inline void PinSlice(const Slice& s, CleanupFunction f, void* arg1,
                        void* arg2) {
     assert(!pinned_);

src.mk

@@ -338,7 +338,6 @@ MAIN_SOURCES =                                                    \
   util/filelock_test.cc                                                 \
   util/log_write_bench.cc                                               \
   util/rate_limiter_test.cc                                             \
-  util/slice_test.cc                                                    \
   util/slice_transform_test.cc                                          \
   util/timer_queue_test.cc                                              \
   util/thread_list_test.cc                                              \

util/slice_test.cc

@@ -1,71 +0,0 @@
-//  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/stack_trace.h"
-#include "rocksdb/slice.h"
-#include "util/testharness.h"
-
-namespace rocksdb {
-
-class SliceTest : public testing::Test {};
-
-namespace {
-void BumpCounter(void* arg1, void* arg2) {
-  (*reinterpret_cast<int*>(arg1))++;
-}
-}  // anonymous namespace
-
-TEST_F(SliceTest, PinnableSliceMoveConstruct) {
-  for (int i = 0; i < 3; i++) {
-    int orig_cleanup = 0;
-    int moved_cleanup = 0;
-    PinnableSlice* s1 = nullptr;
-    std::string external_storage;
-    switch (i) {
-      case 0:
-        s1 = new PinnableSlice();
-        *(s1->GetSelf()) = "foo";
-        s1->PinSelf();
-        s1->RegisterCleanup(BumpCounter, &moved_cleanup, nullptr);
-        break;
-      case 1:
-        s1 = new PinnableSlice(&external_storage);
-        *(s1->GetSelf()) = "foo";
-        s1->PinSelf();
-        s1->RegisterCleanup(BumpCounter, &moved_cleanup, nullptr);
-        break;
-      case 2:
-        s1 = new PinnableSlice();
-        s1->PinSlice("foo", BumpCounter, &moved_cleanup, nullptr);
-        break;
-    }
-    ASSERT_EQ("foo", s1->ToString());
-    PinnableSlice* s2 = new PinnableSlice();
-    s2->PinSelf("bar");
-    ASSERT_EQ("bar", s2->ToString());
-    s2->RegisterCleanup(BumpCounter, &orig_cleanup, nullptr);
-    *s2 = std::move(*s1);
-    ASSERT_FALSE(s1->IsPinned());
-    ASSERT_EQ("foo", s2->ToString());
-    ASSERT_EQ(1, orig_cleanup);
-    ASSERT_EQ(0, moved_cleanup);
-    delete s1;
-    // ASAN will check if it will access storage of s1, which is deleted.
-    ASSERT_EQ("foo", s2->ToString());
-    ASSERT_EQ(1, orig_cleanup);
-    ASSERT_EQ(0, moved_cleanup);
-    delete s2;
-    ASSERT_EQ(1, orig_cleanup);
-    ASSERT_EQ(1, moved_cleanup);
-  }
-}
-
-}  // namespace rocksdb
-
-int main(int argc, char** argv) {
-  rocksdb::port::InstallStackTraceHandler();
-  ::testing::InitGoogleTest(&argc, argv);
-  return RUN_ALL_TESTS();
-}

utilities/blob_db/blob_db_impl.cc

@@ -1245,19 +1245,16 @@ Status BlobDBImpl::Get(const ReadOptions& read_options,
 
   Status s;
   bool is_blob_index = false;
-  PinnableSlice index_entry;
+  s = db_impl_->GetImpl(ro, column_family, key, value,
-  s = db_impl_->GetImpl(ro, column_family, key, &index_entry,
                         nullptr /*value_found*/, nullptr /*read_callback*/,
                         &is_blob_index);
   TEST_SYNC_POINT("BlobDBImpl::Get:AfterIndexEntryGet:1");
   TEST_SYNC_POINT("BlobDBImpl::Get:AfterIndexEntryGet:2");
-  if (s.ok()) {
+  if (s.ok() && is_blob_index) {
-    if (!is_blob_index) {
+    std::string index_entry = value->ToString();
-      *value = std::move(index_entry);
+    value->Reset();
-    } else {
     s = GetBlobValue(key, index_entry, value);
   }
-  }
   if (snapshot_created) {
     db_->ReleaseSnapshot(ro.snapshot);
   }

utilities/blob_db/blob_db_test.cc

@@ -150,6 +150,18 @@ class BlobDBTest : public testing::Test {
   }
 
   void VerifyDB(DB *db, const std::map<std::string, std::string> &data) {
+    // Verify normal Get
+    auto* cfh = db->DefaultColumnFamily();
+    for (auto &p : data) {
+      PinnableSlice value_slice;
+      ASSERT_OK(db->Get(ReadOptions(), cfh, p.first, &value_slice));
+      ASSERT_EQ(p.second, value_slice.ToString());
+      std::string value;
+      ASSERT_OK(db->Get(ReadOptions(), cfh, p.first, &value));
+      ASSERT_EQ(p.second, value);
+    }
+
+    // Verify iterators
     Iterator *iter = db->NewIterator(ReadOptions());
     iter->SeekToFirst();
     for (auto &p : data) {