diff --git a/CMakeLists.txt b/CMakeLists.txt index 017fe8675..139866229 100644 --- a/CMakeLists.txt +++ b/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 diff --git a/Makefile b/Makefile index bad05c1b5..24731770f 100644 --- a/Makefile +++ b/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) diff --git a/TARGETS b/TARGETS index 2f298a46f..a98130afe 100644 --- a/TARGETS +++ b/TARGETS @@ -1309,6 +1309,13 @@ ROCKS_TESTS = [ [], [], ], + [ + "slice_test", + "util/slice_test.cc", + "serial", + [], + [], + ], [ "slice_transform_test", "util/slice_transform_test.cc", diff --git a/include/rocksdb/slice.h b/include/rocksdb/slice.h index 00412bf3c..03581b149 100644 --- a/include/rocksdb/slice.h +++ b/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; diff --git a/src.mk b/src.mk index db91fc417..06468861b 100644 --- a/src.mk +++ b/src.mk @@ -29,7 +29,7 @@ LIB_SOURCES = \ db/db_info_dumper.cc \ db/db_iter.cc \ db/dbformat.cc \ - db/error_handler.cc \ + db/error_handler.cc \ db/event_helpers.cc \ db/experimental.cc \ db/external_sst_file_ingestion_job.cc \ @@ -69,7 +69,7 @@ LIB_SOURCES = \ env/env_hdfs.cc \ env/env_posix.cc \ env/file_system.cc \ - env/fs_posix.cc \ + env/fs_posix.cc \ env/io_posix.cc \ env/mock_env.cc \ file/delete_scheduler.cc \ @@ -127,14 +127,14 @@ LIB_SOURCES = \ table/block_based/data_block_hash_index.cc \ table/block_based/data_block_footer.cc \ table/block_based/filter_block_reader_common.cc \ - table/block_based/filter_policy.cc \ + table/block_based/filter_policy.cc \ table/block_based/flush_block_policy.cc \ table/block_based/full_filter_block.cc \ table/block_based/index_builder.cc \ table/block_based/parsed_full_filter_block.cc \ table/block_based/partitioned_filter_block.cc \ table/block_based/uncompression_dict_reader.cc \ - table/block_fetcher.cc \ + table/block_fetcher.cc \ table/cuckoo/cuckoo_table_builder.cc \ table/cuckoo/cuckoo_table_factory.cc \ table/cuckoo/cuckoo_table_reader.cc \ @@ -200,7 +200,7 @@ LIB_SOURCES = \ utilities/memory/memory_util.cc \ utilities/merge_operators/max.cc \ utilities/merge_operators/put.cc \ - utilities/merge_operators/sortlist.cc \ + utilities/merge_operators/sortlist.cc \ utilities/merge_operators/string_append/stringappend.cc \ utilities/merge_operators/string_append/stringappend2.cc \ utilities/merge_operators/uint64add.cc \ @@ -355,7 +355,7 @@ MAIN_SOURCES = \ db/memtable_list_test.cc \ db/merge_helper_test.cc \ db/merge_test.cc \ - db/obsolete_files_test.cc \ + db/obsolete_files_test.cc \ db/options_settable_test.cc \ db/options_file_test.cc \ db/perf_context_test.cc \ @@ -412,7 +412,7 @@ MAIN_SOURCES = \ tools/ldb_cmd_test.cc \ tools/reduce_levels_test.cc \ tools/sst_dump_test.cc \ - tools/trace_analyzer_test.cc \ + tools/trace_analyzer_test.cc \ trace_replay/block_cache_tracer_test.cc \ util/autovector_test.cc \ util/bloom_test.cc \ @@ -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 \ diff --git a/util/slice.cc b/util/slice.cc index 5e23ae0a3..165da96e1 100644 --- a/util/slice.cc +++ b/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 diff --git a/util/slice_test.cc b/util/slice_test.cc new file mode 100644 index 000000000..8493b1f43 --- /dev/null +++ b/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(arg1); + int* num = reinterpret_cast(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(); +}