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

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 18effaa0667daf52fb5fa34509208fe4cd6617e6..2d99cdf944572c4514727e5ded121d107c6e2ee9 100644 (file)
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -802,7 +802,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

diff --git a/Makefile b/Makefile
index c5ba1012444b99ad3bae8d10ece80540f94e0594..5a89f6bf79d5b77ad4b46becff3cee30ec27ecf9 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -477,7 +477,6 @@ TESTS = \
        object_registry_test \
        repair_test \
        env_timed_test \
-       slice_test \
 
 PARALLEL_TEST = \
        backupable_db_test \
@@ -1436,9 +1435,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

diff --git a/TARGETS b/TARGETS
index c9631b25ae1bf448582fa3809a4e75124043a7d0..ac85eab93c7d83b0c1de7c6a58d61d71f3114bf8 100644 (file)
--- a/TARGETS
+++ b/TARGETS
@@ -463,7 +463,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'],

diff --git a/include/rocksdb/slice.h b/include/rocksdb/slice.h
index 38e7efb65656c70c8cfd4f57ee3b911ba28e94e1..4f24c8a2217c80858521b10ea52bccdab3c44071 100644 (file)
--- a/include/rocksdb/slice.h
+++ b/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_);

diff --git a/src.mk b/src.mk
index bb08721df95f7bc5e9203a861e567c00dc8aff85..5bd5236fa165470c914d2603e0f433ffd9b5af64 100644 (file)
--- a/src.mk
+++ b/src.mk
@@ -326,7 +326,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                                              \

diff --git a/util/slice_test.cc b/util/slice_test.cc
deleted file mode 100644 (file)
index 33903ec..0000000
--- a/util/slice_test.cc
+++ /dev/null
@@ -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();
-}

diff --git a/utilities/blob_db/blob_db_impl.cc b/utilities/blob_db/blob_db_impl.cc
index 1b13513922a60b7f0571acaf10ed597732ea3a47..23f173fd9a9c5d044eb41d4d4872dbd78e6cabfc 100644 (file)
--- a/utilities/blob_db/blob_db_impl.cc
+++ b/utilities/blob_db/blob_db_impl.cc
@@ -1245,17 +1245,14 @@ 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, &index_entry,
+  s = db_impl_->GetImpl(ro, column_family, key, value,
                         nullptr /*value_found*/, &is_blob_index);
   TEST_SYNC_POINT("BlobDBImpl::Get:AfterIndexEntryGet:1");
   TEST_SYNC_POINT("BlobDBImpl::Get:AfterIndexEntryGet:2");
-  if (s.ok()) {
-    if (!is_blob_index) {
-      *value = std::move(index_entry);
-    } else {
-      s = GetBlobValue(key, index_entry, value);
-    }
+  if (s.ok() && is_blob_index) {
+    std::string index_entry = value->ToString();
+    value->Reset();
+    s = GetBlobValue(key, index_entry, value);
   }
   if (snapshot_created) {
     db_->ReleaseSnapshot(ro.snapshot);

diff --git a/utilities/blob_db/blob_db_test.cc b/utilities/blob_db/blob_db_test.cc
index 9f627061a2e4552fdd86f539f876cd7ee2fbfef3..03396eed3894cef44c26db40e62911d20e91f755 100644 (file)
--- a/utilities/blob_db/blob_db_test.cc
+++ b/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) {