From 4e0298f23ccf42e3c0a72bbca1cb80a43d8e5a68 Mon Sep 17 00:00:00 2001 From: kailiu Date: Thu, 30 Jan 2014 17:18:17 -0800 Subject: [PATCH] Clean up arena API Summary: Easy thing goes first. This patch moves arena to internal dir; based on which, the coming patch will deal with memtable_rep. Test Plan: make check Reviewers: haobo, sdong, dhruba CC: leveldb Differential Revision: https://reviews.facebook.net/D15615 --- HISTORY.md | 6 ++++ db/memtable.cc | 12 ++++---- db/memtable.h | 4 +-- db/plain_table_db_test.cc | 7 +++-- db/simple_table_db_test.cc | 1 + db/skiplist.h | 2 +- db/skiplist_test.cc | 14 +++++----- include/rocksdb/arena.h | 45 ------------------------------ include/rocksdb/memtablerep.h | 6 +--- table/table_test.cc | 1 + tools/sst_dump.cc | 6 ++-- util/{arena_impl.cc => arena.cc} | 21 +++++++------- util/{arena_impl.h => arena.h} | 26 ++++++++--------- util/arena_test.cc | 48 +++++++++++++++----------------- util/hash_linklist_rep.cc | 2 +- util/hash_skiplist_rep.cc | 2 +- util/vectorrep.cc | 2 +- 17 files changed, 79 insertions(+), 126 deletions(-) delete mode 100644 include/rocksdb/arena.h rename util/{arena_impl.cc => arena.cc} (82%) rename util/{arena_impl.h => arena.h} (81%) diff --git a/HISTORY.md b/HISTORY.md index 912599bc9..c8fbabd98 100644 --- a/HISTORY.md +++ b/HISTORY.md @@ -1,5 +1,11 @@ # Rocksdb Change Log +## 2.8.0 (01/28/2014) + +### Public API changes + +* Removed arena.h from public header files. + ## 2.7.0 (01/28/2014) ### Public API changes diff --git a/db/memtable.cc b/db/memtable.cc index deb3f7ad7..e47181298 100644 --- a/db/memtable.cc +++ b/db/memtable.cc @@ -17,6 +17,8 @@ #include "rocksdb/env.h" #include "rocksdb/iterator.h" #include "rocksdb/merge_operator.h" +#include "rocksdb/slice_transform.h" +#include "util/arena.h" #include "util/coding.h" #include "util/murmurhash.h" #include "util/mutexlock.h" @@ -38,9 +40,8 @@ namespace rocksdb { MemTable::MemTable(const InternalKeyComparator& cmp, const Options& options) : comparator_(cmp), refs_(0), - arena_impl_(options.arena_block_size), - table_(options.memtable_factory->CreateMemTableRep(comparator_, - &arena_impl_)), + arena_(options.arena_block_size), + table_(options.memtable_factory->CreateMemTableRep(comparator_, &arena_)), flush_in_progress_(false), flush_completed_(false), file_number_(0), @@ -61,8 +62,7 @@ MemTable::~MemTable() { } size_t MemTable::ApproximateMemoryUsage() { - return arena_impl_.ApproximateMemoryUsage() + - table_->ApproximateMemoryUsage(); + return arena_.ApproximateMemoryUsage() + table_->ApproximateMemoryUsage(); } int MemTable::KeyComparator::operator()(const char* prefix_len_key1, @@ -184,7 +184,7 @@ void MemTable::Add(SequenceNumber s, ValueType type, const size_t encoded_len = VarintLength(internal_key_size) + internal_key_size + VarintLength(val_size) + val_size; - char* buf = arena_impl_.Allocate(encoded_len); + char* buf = arena_.Allocate(encoded_len); char* p = EncodeVarint32(buf, internal_key_size); memcpy(p, key.data(), key_size); p += key_size; diff --git a/db/memtable.h b/db/memtable.h index aca4aaf16..349359f8b 100644 --- a/db/memtable.h +++ b/db/memtable.h @@ -16,7 +16,7 @@ #include "db/version_set.h" #include "rocksdb/db.h" #include "rocksdb/memtablerep.h" -#include "util/arena_impl.h" +#include "util/arena.h" #include "util/dynamic_bloom.h" namespace rocksdb { @@ -161,7 +161,7 @@ class MemTable { KeyComparator comparator_; int refs_; - ArenaImpl arena_impl_; + Arena arena_; unique_ptr table_; // These are used to manage memtable flushes to storage diff --git a/db/plain_table_db_test.cc b/db/plain_table_db_test.cc index 8f56638e0..81c0c1ff4 100644 --- a/db/plain_table_db_test.cc +++ b/db/plain_table_db_test.cc @@ -11,17 +11,18 @@ #include #include -#include "rocksdb/db.h" -#include "rocksdb/filter_policy.h" #include "db/db_impl.h" #include "db/filename.h" #include "db/version_set.h" #include "db/write_batch_internal.h" #include "rocksdb/cache.h" #include "rocksdb/compaction_filter.h" +#include "rocksdb/db.h" #include "rocksdb/env.h" -#include "rocksdb/table.h" +#include "rocksdb/filter_policy.h" #include "rocksdb/plain_table_factory.h" +#include "rocksdb/slice_transform.h" +#include "rocksdb/table.h" #include "util/hash.h" #include "util/logging.h" #include "util/mutexlock.h" diff --git a/db/simple_table_db_test.cc b/db/simple_table_db_test.cc index 0f3b89d9b..34bdb5f60 100644 --- a/db/simple_table_db_test.cc +++ b/db/simple_table_db_test.cc @@ -31,6 +31,7 @@ using std::unique_ptr; +// IS THIS FILE STILL NEEDED? namespace rocksdb { // SimpleTable is a simple table format for UNIT TEST ONLY. It is not built diff --git a/db/skiplist.h b/db/skiplist.h index d6c81688e..e713fe42a 100644 --- a/db/skiplist.h +++ b/db/skiplist.h @@ -34,8 +34,8 @@ #include #include #include "port/port.h" +#include "util/arena.h" #include "util/random.h" -#include "rocksdb/arena.h" namespace rocksdb { diff --git a/db/skiplist_test.cc b/db/skiplist_test.cc index dcbaf0abb..b87ddcbb0 100644 --- a/db/skiplist_test.cc +++ b/db/skiplist_test.cc @@ -10,7 +10,7 @@ #include "db/skiplist.h" #include #include "rocksdb/env.h" -#include "util/arena_impl.h" +#include "util/arena.h" #include "util/hash.h" #include "util/random.h" #include "util/testharness.h" @@ -34,9 +34,9 @@ struct TestComparator { class SkipTest { }; TEST(SkipTest, Empty) { - ArenaImpl arena_impl; + Arena arena; TestComparator cmp; - SkipList list(cmp, &arena_impl); + SkipList list(cmp, &arena); ASSERT_TRUE(!list.Contains(10)); SkipList::Iterator iter(&list); @@ -54,9 +54,9 @@ TEST(SkipTest, InsertAndLookup) { const int R = 5000; Random rnd(1000); std::set keys; - ArenaImpl arena_impl; + Arena arena; TestComparator cmp; - SkipList list(cmp, &arena_impl); + SkipList list(cmp, &arena); for (int i = 0; i < N; i++) { Key key = rnd.Next() % R; if (keys.insert(key).second) { @@ -209,14 +209,14 @@ class ConcurrentTest { // Current state of the test State current_; - ArenaImpl arena_impl_; + Arena arena_; // SkipList is not protected by mu_. We just use a single writer // thread to modify it. SkipList list_; public: - ConcurrentTest() : list_(TestComparator(), &arena_impl_) { } + ConcurrentTest() : list_(TestComparator(), &arena_) {} // REQUIRES: External synchronization void WriteStep(Random* rnd) { diff --git a/include/rocksdb/arena.h b/include/rocksdb/arena.h deleted file mode 100644 index 642b61408..000000000 --- a/include/rocksdb/arena.h +++ /dev/null @@ -1,45 +0,0 @@ -// Copyright (c) 2013, Facebook, Inc. All rights reserved. -// This source code is licensed under the BSD-style license found in the -// LICENSE file in the root directory of this source tree. An additional grant -// of patent rights can be found in the PATENTS file in the same directory. -// Copyright (c) 2011 The LevelDB Authors. All rights reserved. -// Use of this source code is governed by a BSD-style license that can be -// found in the LICENSE file. See the AUTHORS file for names of contributors. -// -// Arena class defines memory allocation methods. It's used by memtable and -// skiplist. - -#ifndef STORAGE_ROCKSDB_INCLUDE_ARENA_H_ -#define STORAGE_ROCKSDB_INCLUDE_ARENA_H_ - -#include -#include - -namespace rocksdb { - -class Arena { - public: - Arena() {}; - virtual ~Arena() {}; - - // Return a pointer to a newly allocated memory block of "bytes" bytes. - virtual char* Allocate(size_t bytes) = 0; - - // Allocate memory with the normal alignment guarantees provided by malloc. - virtual char* AllocateAligned(size_t bytes) = 0; - - // Returns an estimate of the total memory used by arena. - virtual const size_t ApproximateMemoryUsage() = 0; - - // Returns the total number of bytes in all blocks allocated so far. - virtual const size_t MemoryAllocatedBytes() = 0; - - private: - // No copying allowed - Arena(const Arena&); - void operator=(const Arena&); -}; - -} // namespace rocksdb - -#endif // STORAGE_ROCKSDB_INCLUDE_ARENA_H_ diff --git a/include/rocksdb/memtablerep.h b/include/rocksdb/memtablerep.h index d5641cb78..e9a41aedd 100644 --- a/include/rocksdb/memtablerep.h +++ b/include/rocksdb/memtablerep.h @@ -33,11 +33,9 @@ // iteration over the entire collection is rare since doing so requires all the // keys to be copied into a sorted data structure. -#ifndef STORAGE_ROCKSDB_DB_MEMTABLEREP_H_ -#define STORAGE_ROCKSDB_DB_MEMTABLEREP_H_ +#pragma once #include -#include "rocksdb/slice_transform.h" namespace rocksdb { @@ -199,5 +197,3 @@ extern MemTableRepFactory* NewHashLinkListRepFactory( const SliceTransform* transform, size_t bucket_count = 50000); } - -#endif // STORAGE_ROCKSDB_DB_MEMTABLEREP_H_ diff --git a/table/table_test.cc b/table/table_test.cc index 4f53ec4da..d165fd2f2 100644 --- a/table/table_test.cc +++ b/table/table_test.cc @@ -23,6 +23,7 @@ #include "rocksdb/plain_table_factory.h" #include "rocksdb/env.h" #include "rocksdb/iterator.h" +#include "rocksdb/slice_transform.h" #include "rocksdb/memtablerep.h" #include "table/meta_blocks.h" #include "rocksdb/plain_table_factory.h" diff --git a/tools/sst_dump.cc b/tools/sst_dump.cc index ba586aca1..1c6b9cfc1 100644 --- a/tools/sst_dump.cc +++ b/tools/sst_dump.cc @@ -291,9 +291,9 @@ int main(int argc, char** argv) { "Table Properties:\n" "------------------------------\n" " %s", table_properties.ToString("\n ", ": ").c_str()); - fprintf(stdout, "# deleted keys: %lu\n", - rocksdb::GetDeletedKeys( - table_properties.user_collected_properties)); + fprintf(stdout, "# deleted keys: %zd\n", + rocksdb::GetDeletedKeys( + table_properties.user_collected_properties)); } } } diff --git a/util/arena_impl.cc b/util/arena.cc similarity index 82% rename from util/arena_impl.cc rename to util/arena.cc index 5125e2364..dffc8b88e 100644 --- a/util/arena_impl.cc +++ b/util/arena.cc @@ -7,19 +7,19 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. -#include "util/arena_impl.h" +#include "util/arena.h" #include namespace rocksdb { -const size_t ArenaImpl::kMinBlockSize = 4096; -const size_t ArenaImpl::kMaxBlockSize = 2 << 30; +const size_t Arena::kMinBlockSize = 4096; +const size_t Arena::kMaxBlockSize = 2 << 30; static const int kAlignUnit = sizeof(void*); size_t OptimizeBlockSize(size_t block_size) { // Make sure block_size is in optimal range - block_size = std::max(ArenaImpl::kMinBlockSize, block_size); - block_size = std::min(ArenaImpl::kMaxBlockSize, block_size); + block_size = std::max(Arena::kMinBlockSize, block_size); + block_size = std::min(Arena::kMaxBlockSize, block_size); // make sure block_size is the multiple of kAlignUnit if (block_size % kAlignUnit != 0) { @@ -29,19 +29,18 @@ size_t OptimizeBlockSize(size_t block_size) { return block_size; } -ArenaImpl::ArenaImpl(size_t block_size) - : kBlockSize(OptimizeBlockSize(block_size)) { +Arena::Arena(size_t block_size) : kBlockSize(OptimizeBlockSize(block_size)) { assert(kBlockSize >= kMinBlockSize && kBlockSize <= kMaxBlockSize && kBlockSize % kAlignUnit == 0); } -ArenaImpl::~ArenaImpl() { +Arena::~Arena() { for (const auto& block : blocks_) { delete[] block; } } -char* ArenaImpl::AllocateFallback(size_t bytes, bool aligned) { +char* Arena::AllocateFallback(size_t bytes, bool aligned) { if (bytes > kBlockSize / 4) { // Object is more than a quarter of our block size. Allocate it separately // to avoid wasting too much space in leftover bytes. @@ -63,7 +62,7 @@ char* ArenaImpl::AllocateFallback(size_t bytes, bool aligned) { } } -char* ArenaImpl::AllocateAligned(size_t bytes) { +char* Arena::AllocateAligned(size_t bytes) { assert((kAlignUnit & (kAlignUnit - 1)) == 0); // Pointer size should be a power of 2 size_t current_mod = @@ -83,7 +82,7 @@ char* ArenaImpl::AllocateAligned(size_t bytes) { return result; } -char* ArenaImpl::AllocateNewBlock(size_t block_bytes) { +char* Arena::AllocateNewBlock(size_t block_bytes) { char* block = new char[block_bytes]; blocks_memory_ += block_bytes; blocks_.push_back(block); diff --git a/util/arena_impl.h b/util/arena.h similarity index 81% rename from util/arena_impl.h rename to util/arena.h index 538385ccc..4c45417f4 100644 --- a/util/arena_impl.h +++ b/util/arena.h @@ -7,7 +7,7 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. -// ArenaImpl is an implementation of Arena class. For a request of small size, +// Arena is an implementation of Arena class. For a request of small size, // it allocates a block with pre-defined block size. For a request of big // size, it uses malloc to directly get the requested size. @@ -16,37 +16,35 @@ #include #include #include -#include "rocksdb/arena.h" +#include "util/arena.h" namespace rocksdb { -class ArenaImpl : public Arena { +class Arena { public: // No copying allowed - ArenaImpl(const ArenaImpl&) = delete; - void operator=(const ArenaImpl&) = delete; + Arena(const Arena&) = delete; + void operator=(const Arena&) = delete; static const size_t kMinBlockSize; static const size_t kMaxBlockSize; - explicit ArenaImpl(size_t block_size = kMinBlockSize); - virtual ~ArenaImpl(); + explicit Arena(size_t block_size = kMinBlockSize); + ~Arena(); - virtual char* Allocate(size_t bytes) override; + char* Allocate(size_t bytes); - virtual char* AllocateAligned(size_t bytes) override; + char* AllocateAligned(size_t bytes); // Returns an estimate of the total memory usage of data allocated // by the arena (exclude the space allocated but not yet used for future // allocations). - virtual const size_t ApproximateMemoryUsage() { + const size_t ApproximateMemoryUsage() { return blocks_memory_ + blocks_.capacity() * sizeof(char*) - alloc_bytes_remaining_; } - virtual const size_t MemoryAllocatedBytes() override { - return blocks_memory_; - } + const size_t MemoryAllocatedBytes() { return blocks_memory_; } private: // Number of bytes allocated in one block @@ -72,7 +70,7 @@ class ArenaImpl : public Arena { size_t blocks_memory_ = 0; }; -inline char* ArenaImpl::Allocate(size_t bytes) { +inline char* Arena::Allocate(size_t bytes) { // The semantics of what to return are a bit messy if we allow // 0-byte allocations, so we disallow them here (we don't need // them for our internal use). diff --git a/util/arena_test.cc b/util/arena_test.cc index ca6dfc99d..1b2b53175 100644 --- a/util/arena_test.cc +++ b/util/arena_test.cc @@ -7,34 +7,32 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. See the AUTHORS file for names of contributors. -#include "util/arena_impl.h" +#include "util/arena.h" #include "util/random.h" #include "util/testharness.h" namespace rocksdb { -class ArenaImplTest { }; +class ArenaTest {}; -TEST(ArenaImplTest, Empty) { - ArenaImpl arena0; -} +TEST(ArenaTest, Empty) { Arena arena0; } -TEST(ArenaImplTest, MemoryAllocatedBytes) { +TEST(ArenaTest, MemoryAllocatedBytes) { const int N = 17; - size_t req_sz; //requested size + size_t req_sz; // requested size size_t bsz = 8192; // block size size_t expected_memory_allocated; - ArenaImpl arena_impl(bsz); + Arena arena(bsz); // requested size > quarter of a block: // allocate requested size separately req_sz = 3001; for (int i = 0; i < N; i++) { - arena_impl.Allocate(req_sz); + arena.Allocate(req_sz); } expected_memory_allocated = req_sz * N; - ASSERT_EQ(arena_impl.MemoryAllocatedBytes(), expected_memory_allocated); + ASSERT_EQ(arena.MemoryAllocatedBytes(), expected_memory_allocated); // requested size < quarter of a block: // allocate a block with the default size, then try to use unused part @@ -42,28 +40,28 @@ TEST(ArenaImplTest, MemoryAllocatedBytes) { // Allocate(99) call. All the remaining calls won't lead to new allocation. req_sz = 99; for (int i = 0; i < N; i++) { - arena_impl.Allocate(req_sz); + arena.Allocate(req_sz); } expected_memory_allocated += bsz; - ASSERT_EQ(arena_impl.MemoryAllocatedBytes(), expected_memory_allocated); + ASSERT_EQ(arena.MemoryAllocatedBytes(), expected_memory_allocated); // requested size > quarter of a block: // allocate requested size separately req_sz = 99999999; for (int i = 0; i < N; i++) { - arena_impl.Allocate(req_sz); + arena.Allocate(req_sz); } expected_memory_allocated += req_sz * N; - ASSERT_EQ(arena_impl.MemoryAllocatedBytes(), expected_memory_allocated); + ASSERT_EQ(arena.MemoryAllocatedBytes(), expected_memory_allocated); } // Make sure we didn't count the allocate but not used memory space in // Arena::ApproximateMemoryUsage() -TEST(ArenaImplTest, ApproximateMemoryUsageTest) { +TEST(ArenaTest, ApproximateMemoryUsageTest) { const size_t kBlockSize = 4096; const size_t kEntrySize = kBlockSize / 8; - const size_t kZero = 0; - ArenaImpl arena(kBlockSize); + const size_t kZero = 0; + Arena arena(kBlockSize); ASSERT_EQ(kZero, arena.ApproximateMemoryUsage()); auto num_blocks = kBlockSize / kEntrySize; @@ -83,9 +81,9 @@ TEST(ArenaImplTest, ApproximateMemoryUsageTest) { ASSERT_GT(usage, mem_usage); } -TEST(ArenaImplTest, Simple) { +TEST(ArenaTest, Simple) { std::vector> allocated; - ArenaImpl arena_impl; + Arena arena; const int N = 100000; size_t bytes = 0; Random rnd(301); @@ -104,9 +102,9 @@ TEST(ArenaImplTest, Simple) { } char* r; if (rnd.OneIn(10)) { - r = arena_impl.AllocateAligned(s); + r = arena.AllocateAligned(s); } else { - r = arena_impl.Allocate(s); + r = arena.Allocate(s); } for (unsigned int b = 0; b < s; b++) { @@ -115,9 +113,9 @@ TEST(ArenaImplTest, Simple) { } bytes += s; allocated.push_back(std::make_pair(s, r)); - ASSERT_GE(arena_impl.ApproximateMemoryUsage(), bytes); + ASSERT_GE(arena.ApproximateMemoryUsage(), bytes); if (i > N / 10) { - ASSERT_LE(arena_impl.ApproximateMemoryUsage(), bytes * 1.10); + ASSERT_LE(arena.ApproximateMemoryUsage(), bytes * 1.10); } } for (unsigned int i = 0; i < allocated.size(); i++) { @@ -132,6 +130,4 @@ TEST(ArenaImplTest, Simple) { } // namespace rocksdb -int main(int argc, char** argv) { - return rocksdb::test::RunAllTests(); -} +int main(int argc, char** argv) { return rocksdb::test::RunAllTests(); } diff --git a/util/hash_linklist_rep.cc b/util/hash_linklist_rep.cc index 844907a28..83f0f3d5a 100644 --- a/util/hash_linklist_rep.cc +++ b/util/hash_linklist_rep.cc @@ -7,7 +7,7 @@ #include "util/hash_linklist_rep.h" #include "rocksdb/memtablerep.h" -#include "rocksdb/arena.h" +#include "util/arena.h" #include "rocksdb/slice.h" #include "rocksdb/slice_transform.h" #include "port/port.h" diff --git a/util/hash_skiplist_rep.cc b/util/hash_skiplist_rep.cc index 845137a4c..aa070bc8b 100644 --- a/util/hash_skiplist_rep.cc +++ b/util/hash_skiplist_rep.cc @@ -7,7 +7,7 @@ #include "util/hash_skiplist_rep.h" #include "rocksdb/memtablerep.h" -#include "rocksdb/arena.h" +#include "util/arena.h" #include "rocksdb/slice.h" #include "rocksdb/slice_transform.h" #include "port/port.h" diff --git a/util/vectorrep.cc b/util/vectorrep.cc index bd2f0873d..4b8b3d552 100644 --- a/util/vectorrep.cc +++ b/util/vectorrep.cc @@ -11,7 +11,7 @@ #include #include -#include "rocksdb/arena.h" +#include "util/arena.h" #include "db/memtable.h" #include "port/port.h" #include "util/mutexlock.h"