From 05709fb43eea34936c9f535edcb74d5e91a0b495 Mon Sep 17 00:00:00 2001 From: costan Date: Mon, 10 Sep 2018 15:38:12 -0700 Subject: [PATCH] Remove InitOnce from the port API. This is not an API-breaking change, because it reduces the API that the leveldb embedder must implement. The project will build just fine against ports that still implement InitOnce. C++11 guarantees thread-safe initialization of static variables inside functions. This is a more restricted form of std::call_once or pthread_once_t (e.g., single call site), so the compiler might be able to generate better code [1]. Equally important, having less code in port_example.h makes it easier to port to other platforms. Due to the change above, this CL introduces a new approach for storing the singleton BytewiseComparatorImpl instance returned by BytewiseComparator(). The new approach avoids a dynamic memory allocation, which eliminates the false positive from LeakSanitizer reported in https://github.com/google/leveldb/issues/200 [1] https://stackoverflow.com/a/27206650/ ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=212348004 --- CMakeLists.txt | 2 ++ port/port_example.h | 10 ---------- port/port_stdcxx.h | 8 -------- util/comparator.cc | 17 ++++++---------- util/no_destructor.h | 47 ++++++++++++++++++++++++++++++++++++++++++++ util/no_destructor_test.cc | 49 ++++++++++++++++++++++++++++++++++++++++++++++ 6 files changed, 104 insertions(+), 29 deletions(-) create mode 100644 util/no_destructor.h create mode 100644 util/no_destructor_test.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index d49c31e..36d6cbd 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -149,6 +149,7 @@ target_sources(leveldb "${PROJECT_SOURCE_DIR}/util/logging.cc" "${PROJECT_SOURCE_DIR}/util/logging.h" "${PROJECT_SOURCE_DIR}/util/mutexlock.h" + "${PROJECT_SOURCE_DIR}/util/no_destructor.h" "${PROJECT_SOURCE_DIR}/util/options.cc" "${PROJECT_SOURCE_DIR}/util/random.h" "${PROJECT_SOURCE_DIR}/util/status.cc" @@ -278,6 +279,7 @@ if(LEVELDB_BUILD_TESTS) leveldb_test("${PROJECT_SOURCE_DIR}/util/env_test.cc") leveldb_test("${PROJECT_SOURCE_DIR}/util/status_test.cc") + leveldb_test("${PROJECT_SOURCE_DIR}/util/no_destructor_test.cc") if(NOT BUILD_SHARED_LIBS) leveldb_test("${PROJECT_SOURCE_DIR}/db/autocompact_test.cc") diff --git a/port/port_example.h b/port/port_example.h index 88fc9cb..9c648c3 100644 --- a/port/port_example.h +++ b/port/port_example.h @@ -62,16 +62,6 @@ class CondVar { void SignallAll(); }; -// Thread-safe initialization. -// Used as follows: -// static port::OnceType init_control = LEVELDB_ONCE_INIT; -// static void Initializer() { ... do something ...; } -// ... -// port::InitOnce(&init_control, &Initializer); -typedef intptr_t OnceType; -#define LEVELDB_ONCE_INIT 0 -void InitOnce(port::OnceType*, void (*initializer)()); - // A type that holds a pointer that can be read or written atomically // (i.e., without word-tearing.) class AtomicPointer { diff --git a/port/port_stdcxx.h b/port/port_stdcxx.h index 4e58cba..4713e26 100644 --- a/port/port_stdcxx.h +++ b/port/port_stdcxx.h @@ -84,14 +84,6 @@ class CondVar { Mutex* const mu_; }; -using OnceType = std::once_flag; -#define LEVELDB_ONCE_INIT {} - -// Thinly wraps std::call_once. -inline void InitOnce(OnceType* once, void (*initializer)()) { - std::call_once(*once, *initializer); -} - inline bool Snappy_Compress(const char* input, size_t length, ::std::string* output) { #if HAVE_SNAPPY diff --git a/util/comparator.cc b/util/comparator.cc index 4b7b572..e1e2963 100644 --- a/util/comparator.cc +++ b/util/comparator.cc @@ -3,11 +3,13 @@ // found in the LICENSE file. See the AUTHORS file for names of contributors. #include -#include +#include +#include + #include "leveldb/comparator.h" #include "leveldb/slice.h" -#include "port/port.h" #include "util/logging.h" +#include "util/no_destructor.h" namespace leveldb { @@ -66,16 +68,9 @@ class BytewiseComparatorImpl : public Comparator { }; } // namespace -static port::OnceType once = LEVELDB_ONCE_INIT; -static const Comparator* bytewise; - -static void InitModule() { - bytewise = new BytewiseComparatorImpl; -} - const Comparator* BytewiseComparator() { - port::InitOnce(&once, InitModule); - return bytewise; + static NoDestructor singleton; + return singleton.get(); } } // namespace leveldb diff --git a/util/no_destructor.h b/util/no_destructor.h new file mode 100644 index 0000000..4827e45 --- /dev/null +++ b/util/no_destructor.h @@ -0,0 +1,47 @@ +// Copyright (c) 2018 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. + +#ifndef STORAGE_LEVELDB_UTIL_NO_DESTRUCTOR_H_ +#define STORAGE_LEVELDB_UTIL_NO_DESTRUCTOR_H_ + +#include +#include + +namespace leveldb { + +// Wraps an instance whose destructor is never called. +// +// This is intended for use with function-level static variables. +template +class NoDestructor { + public: + template + explicit NoDestructor(ConstructorArgTypes&&... constructor_args) { + static_assert(sizeof(instance_storage_) >= sizeof(InstanceType), + "instance_storage_ is not large enough to hold the instance"); + static_assert( + alignof(decltype(instance_storage_)) >= alignof(InstanceType), + "instance_storage_ does not meet the instance's alignment requirement"); + new (&instance_storage_) InstanceType( + std::forward(constructor_args)...); + } + + ~NoDestructor() = default; + + NoDestructor(const NoDestructor&) = delete; + NoDestructor& operator=(const NoDestructor&) = delete; + + InstanceType* get() { + return reinterpret_cast(&instance_storage_); + } + + private: + typename + std::aligned_storage::type + instance_storage_; +}; + +} // namespace leveldb + +#endif // STORAGE_LEVELDB_UTIL_NO_DESTRUCTOR_H_ diff --git a/util/no_destructor_test.cc b/util/no_destructor_test.cc new file mode 100644 index 0000000..7ce2631 --- /dev/null +++ b/util/no_destructor_test.cc @@ -0,0 +1,49 @@ +// Copyright (c) 2018 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. + +#include +#include +#include + +#include "util/no_destructor.h" +#include "util/testharness.h" + +namespace leveldb { + +namespace { + +struct DoNotDestruct { + public: + DoNotDestruct(uint32_t a, uint64_t b) : a(a), b(b) {} + ~DoNotDestruct() { std::abort(); } + + // Used to check constructor argument forwarding. + uint32_t a; + uint64_t b; +}; + +constexpr const uint32_t kGoldenA = 0xdeadbeef; +constexpr const uint64_t kGoldenB = 0xaabbccddeeffaabb; + +} // namespace + +class NoDestructorTest { }; + +TEST(NoDestructorTest, StackInstance) { + NoDestructor instance(kGoldenA, kGoldenB); + ASSERT_EQ(kGoldenA, instance.get()->a); + ASSERT_EQ(kGoldenB, instance.get()->b); +} + +TEST(NoDestructorTest, StaticInstance) { + static NoDestructor instance(kGoldenA, kGoldenB); + ASSERT_EQ(kGoldenA, instance.get()->a); + ASSERT_EQ(kGoldenB, instance.get()->b); +} + +} // namespace leveldb + +int main(int argc, char** argv) { + return leveldb::test::RunAllTests(); +}