From ed154f6dc4f5ca82f20d2f3d4383cdbb07872594 Mon Sep 17 00:00:00 2001 From: "gabor@google.com" Date: Thu, 30 Jun 2011 23:17:03 +0000 Subject: [PATCH] Fixed a snappy compression wrapper bug (passing wrong variable). Change atomic_pointer.h to prefer a memory barrier based implementation over a based implementation for the following reasons: (1) On a x86-32-bit gcc-4.4 build, was corrupting the AtomicPointer. (2) On a x86-64-bit gcc build, a based acquire-load takes ~15ns as opposed to the ~1ns for a memory-barrier based implementation. Fixes issue 9 (corruption_test fails) http://code.google.com/p/leveldb/issues/detail?id=9 Fixes issue 16 (CorruptionTest.MissingDescriptor fails) http://code.google.com/p/leveldb/issues/detail?id=16 git-svn-id: https://leveldb.googlecode.com/svn/trunk@36 62dab493-f737-651d-591e-8d6aee1b9529 --- port/atomic_pointer.h | 208 ++++++++++++++++---------------------------------- port/port_posix.h | 2 +- 2 files changed, 66 insertions(+), 144 deletions(-) diff --git a/port/atomic_pointer.h b/port/atomic_pointer.h index 3bae007..7659840 100644 --- a/port/atomic_pointer.h +++ b/port/atomic_pointer.h @@ -4,57 +4,31 @@ // AtomicPointer provides storage for a lock-free pointer. // Platform-dependent implementation of AtomicPointer: +// - If the platform provides a cheap barrier, we use it with raw pointers // - If cstdatomic is present (on newer versions of gcc, it is), we use -// a cstdatomic-based AtomicPointer -// - If it is not, we define processor-dependent AtomicWord operations, -// and then use them to build AtomicPointer -// +// a cstdatomic-based AtomicPointer. However we prefer the memory +// barrier based version, because at least on a gcc 4.4 32-bit build +// on linux, we have encountered a buggy +// implementation. Also, some implementations are much +// slower than a memory-barrier based implementation (~16ns for +// based acquire-load vs. ~1ns for a barrier based +// acquire-load). // This code is based on atomicops-internals-* in Google's perftools: // http://code.google.com/p/google-perftools/source/browse/#svn%2Ftrunk%2Fsrc%2Fbase #ifndef PORT_ATOMIC_POINTER_H_ #define PORT_ATOMIC_POINTER_H_ +#include #ifdef LEVELDB_CSTDATOMIC_PRESENT - -/////////////////////////////////////////////////////////////////////////////// -// WE HAVE -// Use a -based AtomicPointer - #include -#include - -namespace leveldb { -namespace port { - -// Storage for a lock-free pointer -class AtomicPointer { - private: - std::atomic rep_; - public: - AtomicPointer() { } - explicit AtomicPointer(void* v) : rep_(v) { } - inline void* Acquire_Load() const { - return rep_.load(std::memory_order_acquire); - } - inline void Release_Store(void* v) { - rep_.store(v, std::memory_order_release); - } - inline void* NoBarrier_Load() const { - return rep_.load(std::memory_order_relaxed); - } - inline void NoBarrier_Store(void* v) { - rep_.store(v, std::memory_order_relaxed); - } -}; - -} // namespace leveldb::port -} // namespace leveldb - -#else -/////////////////////////////////////////////////////////////////////////////// -// NO -// The entire rest of this file covers that case +#endif +#ifdef OS_WIN +#include +#endif +#ifdef OS_MACOSX +#include +#endif #if defined(_M_X64) || defined(__x86_64__) #define ARCH_CPU_X86_FAMILY 1 @@ -62,152 +36,100 @@ class AtomicPointer { #define ARCH_CPU_X86_FAMILY 1 #elif defined(__ARMEL__) #define ARCH_CPU_ARM_FAMILY 1 -#else -#warning Please add support for your architecture in atomicpointer.h #endif namespace leveldb { namespace port { -namespace internal { - -// AtomicWord is a machine-sized pointer. -typedef intptr_t AtomicWord; - -} // namespace leveldb::port::internal -} // namespace leveldb::port -} // namespace leveldb -// Include our platform specific implementation. -/////////////////////////////////////////////////////////////////////////////// +// Define MemoryBarrier() if available // Windows on x86 #if defined(OS_WIN) && defined(COMPILER_MSVC) && defined(ARCH_CPU_X86_FAMILY) - -// void MemoryBarrier(void) macro is defined in windows.h: +// windows.h already provides a MemoryBarrier(void) macro // http://msdn.microsoft.com/en-us/library/ms684208(v=vs.85).aspx -// Including windows.h here; MemoryBarrier() gets used below. -#include - -/////////////////////////////////////////////////////////////////////////////// -// Mac OS on x86 -#elif defined(OS_MACOSX) && defined(ARCH_CPU_X86_FAMILY) - -#include - -namespace leveldb { -namespace port { -namespace internal { +#define LEVELDB_HAVE_MEMORY_BARRIER +// Gcc on x86 +#elif defined(__GNUC__) && defined(ARCH_CPU_X86_FAMILY) inline void MemoryBarrier() { -#if defined(__GNUC__) && (defined(__i386__) || defined(__x86_64__)) // See http://gcc.gnu.org/ml/gcc/2003-04/msg01180.html for a discussion on // this idiom. Also see http://en.wikipedia.org/wiki/Memory_ordering. __asm__ __volatile__("" : : : "memory"); -#else - OSMemoryBarrier(); -#endif } +#define LEVELDB_HAVE_MEMORY_BARRIER -} // namespace leveldb::port::internal -} // namespace leveldb::port -} // namespace leveldb - -/////////////////////////////////////////////////////////////////////////////// -// Any x86 CPU -#elif defined(ARCH_CPU_X86_FAMILY) - -namespace leveldb { -namespace port { -namespace internal { - +// Mac OS +#elif defined(OS_MACOSX) inline void MemoryBarrier() { - __asm__ __volatile__("" : : : "memory"); + OSMemoryBarrier(); } +#define LEVELDB_HAVE_MEMORY_BARRIER -} // namespace leveldb::port::internal -} // namespace leveldb::port -} // namespace leveldb - -#undef ATOMICOPS_COMPILER_BARRIER - -/////////////////////////////////////////////////////////////////////////////// // ARM #elif defined(ARCH_CPU_ARM_FAMILY) - -namespace leveldb { -namespace port { -namespace internal { - typedef void (*LinuxKernelMemoryBarrierFunc)(void); LinuxKernelMemoryBarrierFunc pLinuxKernelMemoryBarrier __attribute__((weak)) = (LinuxKernelMemoryBarrierFunc) 0xffff0fa0; - inline void MemoryBarrier() { pLinuxKernelMemoryBarrier(); } +#define LEVELDB_HAVE_MEMORY_BARRIER -} // namespace leveldb::port::internal -} // namespace leveldb::port -} // namespace leveldb - -#else -#error "Atomic operations are not supported on your platform" #endif -/////////////////////////////////////////////////////////////////////////////// -// Implementation of AtomicPointer based on MemoryBarriers above - -namespace leveldb { -namespace port { -namespace internal { - -// Atomic operations using per-system MemoryBarrier()s - -inline AtomicWord Acquire_Load(volatile const AtomicWord* ptr) { - AtomicWord value = *ptr; - MemoryBarrier(); - return value; -} - -inline void Release_Store(volatile AtomicWord* ptr, AtomicWord value) { - MemoryBarrier(); - *ptr = value; -} - -inline AtomicWord NoBarrier_Load(volatile const AtomicWord* ptr) { - return *ptr; -} - -inline void NoBarrier_Store(volatile AtomicWord* ptr, AtomicWord value) { - *ptr = value; -} - -} // namespace leveldb::port::internal +// AtomicPointer built using platform-specific MemoryBarrier() +#if defined(LEVELDB_HAVE_MEMORY_BARRIER) +class AtomicPointer { + private: + void* rep_; + public: + AtomicPointer() { } + explicit AtomicPointer(void* p) : rep_(p) {} + inline void* NoBarrier_Load() const { return rep_; } + inline void NoBarrier_Store(void* v) { rep_ = v; } + inline void* Acquire_Load() const { + void* result = rep_; + MemoryBarrier(); + return result; + } + inline void Release_Store(void* v) { + MemoryBarrier(); + rep_ = v; + } +}; -// AtomicPointer definition for systems without . +// AtomicPointer based on +#elif defined(LEVELDB_CSTDATOMIC_PRESENT) class AtomicPointer { private: - typedef internal::AtomicWord Rep; - Rep rep_; + std::atomic rep_; public: AtomicPointer() { } - explicit AtomicPointer(void* p) : rep_(reinterpret_cast(p)) {} + explicit AtomicPointer(void* v) : rep_(v) { } inline void* Acquire_Load() const { - return reinterpret_cast(internal::Acquire_Load(&rep_)); + return rep_.load(std::memory_order_acquire); } inline void Release_Store(void* v) { - internal::Release_Store(&rep_, reinterpret_cast(v)); + rep_.store(v, std::memory_order_release); } inline void* NoBarrier_Load() const { - return reinterpret_cast(internal::NoBarrier_Load(&rep_)); + return rep_.load(std::memory_order_relaxed); } inline void NoBarrier_Store(void* v) { - internal::NoBarrier_Store(&rep_, reinterpret_cast(v)); + rep_.store(v, std::memory_order_relaxed); } }; +// We have neither MemoryBarrier(), nor +#else +#error Please implement AtomicPointer for this platform. + +#endif + +#undef LEVELDB_HAVE_MEMORY_BARRIER +#undef ARCH_CPU_X86_FAMILY +#undef ARCH_CPU_ARM_FAMILY + } // namespace leveldb::port } // namespace leveldb -#endif // LEVELDB_CSTDATOMIC_PRESENT - #endif // PORT_ATOMIC_POINTER_H_ diff --git a/port/port_posix.h b/port/port_posix.h index 3f329f0..2995026 100644 --- a/port/port_posix.h +++ b/port/port_posix.h @@ -97,7 +97,7 @@ inline bool Snappy_Uncompress(const char* input_data, size_t input_length, ::std::string* output) { #ifdef SNAPPY size_t ulength; - if (!snappy::GetUncompressedLength(input_data, ulength, &ulength)) { + if (!snappy::GetUncompressedLength(input_data, input_length, &ulength)) { return false; } output->resize(ulength);