From 6c72079d77ffee55ecd15856e90808bfb892980c Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Fri, 14 Mar 2014 09:54:23 -0700 Subject: [PATCH 1/5] Fix warning on Mac OS --- db/memtable.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/memtable.cc b/db/memtable.cc index 775b802d1b..f9fa66eec6 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -562,8 +562,8 @@ size_t MemTable::CountSuccessiveMergeEntries(const LookupKey& key) { const char* entry = iter->key(); uint32_t key_length = 0; const char* iter_key_ptr = GetVarint32Ptr(entry, entry + 5, &key_length); - if (!comparator_.comparator.user_comparator()->Compare( - Slice(iter_key_ptr, key_length - 8), key.user_key()) == 0) { + if (comparator_.comparator.user_comparator()->Compare( + Slice(iter_key_ptr, key_length - 8), key.user_key()) != 0) { break; } From 3c75cc15a960768dd4a1a2f518dc8a935b4600d2 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Fri, 14 Mar 2014 10:02:04 -0700 Subject: [PATCH 2/5] Fix HashSkipList and HashLinkedList SIGSEGV Summary: Original Summary: Yesterday, @ljin and I were debugging various db_stress issues. We suspected one of them happens when we concurrently call NewIterator without prefix_seek on HashSkipList. This test demonstrates it. Update: Arena is not thread-safe!! When creating a new full iterator, we *have* to create a new arena, otherwise we're doomed. Test Plan: SIGSEGV and assertion-throwing test now works! Reviewers: ljin, haobo, sdong Reviewed By: sdong CC: leveldb, ljin Differential Revision: https://reviews.facebook.net/D16857 --- db/prefix_test.cc | 37 +++++++++++++++++++++++++++++++++++++ util/arena.h | 2 ++ util/hash_linklist_rep.cc | 11 +++++++---- util/hash_skiplist_rep.cc | 14 ++++++++------ 4 files changed, 54 insertions(+), 10 deletions(-) diff --git a/db/prefix_test.cc b/db/prefix_test.cc index 0f2c54a591..89e31b60c7 100644 --- a/db/prefix_test.cc +++ b/db/prefix_test.cc @@ -369,6 +369,43 @@ TEST(PrefixTest, TestResult) { } } +TEST(PrefixTest, FullIterator) { + while (NextOptions(1000000)) { + DestroyDB(kDbName, Options()); + auto db = OpenDb(); + WriteOptions write_options; + + std::vector prefixes; + for (uint64_t i = 0; i < 100; ++i) { + prefixes.push_back(i); + } + std::random_shuffle(prefixes.begin(), prefixes.end()); + + for (auto prefix : prefixes) { + for (uint64_t i = 0; i < 200; ++i) { + TestKey test_key(prefix, i); + Slice key = TestKeyToSlice(test_key); + ASSERT_OK(db->Put(write_options, key, Slice("0"))); + } + } + + auto func = [](void* db_void) { + auto db = reinterpret_cast(db_void); + std::unique_ptr iter(db->NewIterator(ReadOptions())); + iter->SeekToFirst(); + for (int i = 0; i < 3; ++i) { + iter->Next(); + } + }; + + auto env = Env::Default(); + for (int i = 0; i < 16; ++i) { + env->StartThread(func, reinterpret_cast(db.get())); + } + env->WaitForJoin(); + } +} + TEST(PrefixTest, DynamicPrefixIterator) { while (NextOptions(FLAGS_bucket_count)) { std::cout << "*** Mem table: " << options.memtable_factory->Name() diff --git a/util/arena.h b/util/arena.h index e6963355bb..6ce5a438da 100644 --- a/util/arena.h +++ b/util/arena.h @@ -52,6 +52,8 @@ class Arena { // same size of that allocation. virtual size_t IrregularBlockNum() const { return irregular_block_num; } + size_t BlockSize() const { return kBlockSize; } + private: // Number of bytes allocated in one block const size_t kBlockSize; diff --git a/util/hash_linklist_rep.cc b/util/hash_linklist_rep.cc index e09052a3d8..f1f064fb30 100644 --- a/util/hash_linklist_rep.cc +++ b/util/hash_linklist_rep.cc @@ -141,8 +141,8 @@ class HashLinkListRep : public MemTableRep { class FullListIterator : public MemTableRep::Iterator { public: - explicit FullListIterator(FullList* list) - : iter_(list), full_list_(list) {} + explicit FullListIterator(FullList* list, Arena* arena) + : iter_(list), full_list_(list), arena_(arena) {} virtual ~FullListIterator() { } @@ -196,6 +196,7 @@ class HashLinkListRep : public MemTableRep { FullList::Iterator iter_; // To destruct with the iterator. std::unique_ptr full_list_; + std::unique_ptr arena_; std::string tmp_; // For passing to EncodeKey }; @@ -416,7 +417,9 @@ void HashLinkListRep::Get(const LookupKey& k, void* callback_args, } MemTableRep::Iterator* HashLinkListRep::GetIterator() { - auto list = new FullList(compare_, arena_); + // allocate a new arena of similar size to the one currently in use + Arena* new_arena = new Arena(arena_->BlockSize()); + auto list = new FullList(compare_, new_arena); for (size_t i = 0; i < bucket_size_; ++i) { auto bucket = GetBucket(i); if (bucket != nullptr) { @@ -426,7 +429,7 @@ MemTableRep::Iterator* HashLinkListRep::GetIterator() { } } } - return new FullListIterator(list); + return new FullListIterator(list, new_arena); } MemTableRep::Iterator* HashLinkListRep::GetPrefixIterator( diff --git a/util/hash_skiplist_rep.cc b/util/hash_skiplist_rep.cc index 307e19838a..ee92e79522 100644 --- a/util/hash_skiplist_rep.cc +++ b/util/hash_skiplist_rep.cc @@ -81,10 +81,9 @@ class HashSkipListRep : public MemTableRep { class Iterator : public MemTableRep::Iterator { public: - explicit Iterator(Bucket* list, bool own_list = true) - : list_(list), - iter_(list), - own_list_(own_list) {} + explicit Iterator(Bucket* list, bool own_list = true, + Arena* arena = nullptr) + : list_(list), iter_(list), own_list_(own_list), arena_(arena) {} virtual ~Iterator() { // if we own the list, we should also delete it @@ -163,6 +162,7 @@ class HashSkipListRep : public MemTableRep { // here we track if we own list_. If we own it, we are also // responsible for it's cleaning. This is a poor man's shared_ptr bool own_list_; + std::unique_ptr arena_; std::string tmp_; // For passing to EncodeKey }; @@ -289,7 +289,9 @@ void HashSkipListRep::Get(const LookupKey& k, void* callback_args, } MemTableRep::Iterator* HashSkipListRep::GetIterator() { - auto list = new Bucket(compare_, arena_); + // allocate a new arena of similar size to the one currently in use + Arena* new_arena = new Arena(arena_->BlockSize()); + auto list = new Bucket(compare_, new_arena); for (size_t i = 0; i < bucket_size_; ++i) { auto bucket = GetBucket(i); if (bucket != nullptr) { @@ -299,7 +301,7 @@ MemTableRep::Iterator* HashSkipListRep::GetIterator() { } } } - return new Iterator(list); + return new Iterator(list, true, new_arena); } MemTableRep::Iterator* HashSkipListRep::GetPrefixIterator(const Slice& prefix) { From f74659ac9f4a934a35e6eb54f14d586747c722a1 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Fri, 14 Mar 2014 10:13:39 -0700 Subject: [PATCH 3/5] Fix another Mac OS warning --- util/crc32c.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/util/crc32c.cc b/util/crc32c.cc index 5008dddee2..ff7991753e 100644 --- a/util/crc32c.cc +++ b/util/crc32c.cc @@ -291,9 +291,11 @@ static inline uint32_t LE_LOAD32(const uint8_t *p) { return DecodeFixed32(reinterpret_cast(p)); } +#ifdef __SSE4_2__ static inline uint64_t LE_LOAD64(const uint8_t *p) { return DecodeFixed64(reinterpret_cast(p)); } +#else static inline void Slow_CRC32(uint64_t* l, uint8_t const **p) { uint32_t c = *l ^ LE_LOAD32(*p); From 56dce9bf8eade79bda167c2722bc77c879feb9a7 Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Fri, 14 Mar 2014 10:22:37 -0700 Subject: [PATCH 4/5] unterminated conditional directive --- util/crc32c.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/crc32c.cc b/util/crc32c.cc index ff7991753e..04312d6f67 100644 --- a/util/crc32c.cc +++ b/util/crc32c.cc @@ -295,7 +295,7 @@ static inline uint32_t LE_LOAD32(const uint8_t *p) { static inline uint64_t LE_LOAD64(const uint8_t *p) { return DecodeFixed64(reinterpret_cast(p)); } -#else +#endif static inline void Slow_CRC32(uint64_t* l, uint8_t const **p) { uint32_t c = *l ^ LE_LOAD32(*p); From 2bad3cb0db7f510ccba4ef3b29437cec1d12220a Mon Sep 17 00:00:00 2001 From: Igor Canadi Date: Fri, 14 Mar 2014 13:02:20 -0700 Subject: [PATCH 5/5] Missing includes --- db/perf_context_test.cc | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/perf_context_test.cc b/db/perf_context_test.cc index 472cc719a2..a182fb5214 100644 --- a/db/perf_context_test.cc +++ b/db/perf_context_test.cc @@ -10,6 +10,8 @@ #include "rocksdb/db.h" #include "rocksdb/perf_context.h" +#include "rocksdb/slice_transform.h" +#include "rocksdb/memtablerep.h" #include "util/histogram.h" #include "util/stop_watch.h" #include "util/testharness.h"