From 0025a36409b2e3263a2909798a2c8c16942b9bae Mon Sep 17 00:00:00 2001 From: Aaron Gao Date: Mon, 26 Jun 2017 15:13:35 -0700 Subject: [PATCH] revert perf_context and io_stats to __thread Summary: https://github.com/facebook/rocksdb/pull/2380 introduces a regression by replacing __thread with ThreadLocalPtr. Revert the thread local implementation back. Closes https://github.com/facebook/rocksdb/pull/2485 Differential Revision: D5308050 Pulled By: lightmark fbshipit-source-id: 2676e9c22edf76e8133d3f4c50e2711e11a95480 --- include/rocksdb/perf_context.h | 1 - monitoring/iostats_context.cc | 16 ++++----- monitoring/perf_context.cc | 23 ++++++------ monitoring/perf_context_imp.h | 5 ++- util/thread_local_test.cc | 65 +++++++++++++++++----------------- 5 files changed, 56 insertions(+), 54 deletions(-) diff --git a/include/rocksdb/perf_context.h b/include/rocksdb/perf_context.h index fa0dc426c..ff9449a54 100644 --- a/include/rocksdb/perf_context.h +++ b/include/rocksdb/perf_context.h @@ -154,7 +154,6 @@ struct PerfContext { // if defined(NPERF_CONTEXT), then the pointer is not thread-local PerfContext* get_perf_context(); - } #endif diff --git a/monitoring/iostats_context.cc b/monitoring/iostats_context.cc index 921fd7e10..019622a43 100644 --- a/monitoring/iostats_context.cc +++ b/monitoring/iostats_context.cc @@ -10,16 +10,16 @@ namespace rocksdb { -ThreadLocalPtr iostats_context([](void* ptr) { - auto* p = static_cast(ptr); - delete p; - }); +#ifdef ROCKSDB_SUPPORT_THREAD_LOCAL +__thread IOStatsContext iostats_context; +#endif IOStatsContext* get_iostats_context() { - if (iostats_context.Get() == nullptr) { - iostats_context.Reset(static_cast(new IOStatsContext())); - } - return static_cast(iostats_context.Get()); +#ifdef ROCKSDB_SUPPORT_THREAD_LOCAL + return &iostats_context; +#else + return nullptr; +#endif } void IOStatsContext::Reset() { diff --git a/monitoring/perf_context.cc b/monitoring/perf_context.cc index 5cf8f3eb7..452d6ff84 100644 --- a/monitoring/perf_context.cc +++ b/monitoring/perf_context.cc @@ -8,27 +8,28 @@ #include #include "monitoring/perf_context_imp.h" -#include "util/thread_local.h" namespace rocksdb { -#ifdef NPERF_CONTEXT +#if defined(NPERF_CONTEXT) || !defined(ROCKSDB_SUPPORT_THREAD_LOCAL) PerfContext perf_context; #else -ThreadLocalPtr perf_context([](void* ptr) { - auto* p = static_cast(ptr); - delete p; - }); +#if defined(OS_SOLARIS) +__thread PerfContext perf_context_; +#else +__thread PerfContext perf_context; +#endif #endif PerfContext* get_perf_context() { -#ifdef NPERF_CONTEXT +#if defined(NPERF_CONTEXT) || !defined(ROCKSDB_SUPPORT_THREAD_LOCAL) return &perf_context; #else - if (perf_context.Get() == nullptr) { - perf_context.Reset(static_cast(new PerfContext())); - } - return static_cast(perf_context.Get()); +#if defined(OS_SOLARIS) + return &perf_context_; +#else + return &perf_context; +#endif #endif } diff --git a/monitoring/perf_context_imp.h b/monitoring/perf_context_imp.h index e7817205e..6371e1e1d 100644 --- a/monitoring/perf_context_imp.h +++ b/monitoring/perf_context_imp.h @@ -44,7 +44,10 @@ namespace rocksdb { #define PERF_TIMER_MEASURE(metric) perf_step_timer_##metric.Measure(); // Increase metric value -#define PERF_COUNTER_ADD(metric, value) get_perf_context()->metric += value; +#define PERF_COUNTER_ADD(metric, value) \ + if (perf_level >= PerfLevel::kEnableCount) { \ + get_perf_context()->metric += value; \ + } #endif diff --git a/util/thread_local_test.cc b/util/thread_local_test.cc index 77e71b092..2cdf093fe 100644 --- a/util/thread_local_test.cc +++ b/util/thread_local_test.cc @@ -67,54 +67,53 @@ TEST_F(ThreadLocalTest, UniqueIdTest) { port::Mutex mu; port::CondVar cv(&mu); - // perf_context and iostats_context take 2 ids - ASSERT_EQ(IDChecker::PeekId(), 2u); + ASSERT_EQ(IDChecker::PeekId(), 0u); // New ThreadLocal instance bumps id by 1 { - // Id used 2 + // Id used 0 Params p1(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 3u); - // Id used 3 + ASSERT_EQ(IDChecker::PeekId(), 1u); + // Id used 1 Params p2(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 4u); - // Id used 4 + ASSERT_EQ(IDChecker::PeekId(), 2u); + // Id used 2 Params p3(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 5u); - // Id used 5 + ASSERT_EQ(IDChecker::PeekId(), 3u); + // Id used 3 Params p4(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 6u); + ASSERT_EQ(IDChecker::PeekId(), 4u); } - // id 5, 4, 3, 2 are in the free queue in order - ASSERT_EQ(IDChecker::PeekId(), 2u); + // id 3, 2, 1, 0 are in the free queue in order + ASSERT_EQ(IDChecker::PeekId(), 0u); - // pick up 2 + // pick up 0 Params p1(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 3u); - // pick up 3 + ASSERT_EQ(IDChecker::PeekId(), 1u); + // pick up 1 Params* p2 = new Params(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 4u); - // pick up 4 + ASSERT_EQ(IDChecker::PeekId(), 2u); + // pick up 2 Params p3(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 5u); - // return up 3 + ASSERT_EQ(IDChecker::PeekId(), 3u); + // return up 1 delete p2; + ASSERT_EQ(IDChecker::PeekId(), 1u); + // Now we have 3, 1 in queue + // pick up 1 + Params p4(&mu, &cv, nullptr, 1u); ASSERT_EQ(IDChecker::PeekId(), 3u); - // Now we have 4, 2 in queue // pick up 3 - Params p4(&mu, &cv, nullptr, 1u); - ASSERT_EQ(IDChecker::PeekId(), 5u); - // pick up 5 Params p5(&mu, &cv, nullptr, 1u); // next new id - ASSERT_EQ(IDChecker::PeekId(), 6u); + ASSERT_EQ(IDChecker::PeekId(), 4u); // After exit, id sequence in queue: - // 5, 4, 3, 2(, 1, 0) + // 3, 1, 2, 0 } #endif // __clang_analyzer__ TEST_F(ThreadLocalTest, SequentialReadWriteTest) { - // global id list carries over 5, 4, 3, 2 - ASSERT_EQ(IDChecker::PeekId(), 2u); + // global id list carries over 3, 1, 2, 0 + ASSERT_EQ(IDChecker::PeekId(), 0u); port::Mutex mu; port::CondVar cv(&mu); @@ -144,7 +143,7 @@ TEST_F(ThreadLocalTest, SequentialReadWriteTest) { }; for (int iter = 0; iter < 1024; ++iter) { - ASSERT_EQ(IDChecker::PeekId(), 3u); + ASSERT_EQ(IDChecker::PeekId(), 1u); // Another new thread, read/write should not see value from previous thread env_->StartThread(func, static_cast(&p)); mu.Lock(); @@ -152,13 +151,13 @@ TEST_F(ThreadLocalTest, SequentialReadWriteTest) { cv.Wait(); } mu.Unlock(); - ASSERT_EQ(IDChecker::PeekId(), 3u); + ASSERT_EQ(IDChecker::PeekId(), 1u); } } TEST_F(ThreadLocalTest, ConcurrentReadWriteTest) { - // global id list carries over 5, 4, 3, 2 - ASSERT_EQ(IDChecker::PeekId(), 2u); + // global id list carries over 3, 1, 2, 0 + ASSERT_EQ(IDChecker::PeekId(), 0u); ThreadLocalPtr tls2; port::Mutex mu1; @@ -239,11 +238,11 @@ TEST_F(ThreadLocalTest, ConcurrentReadWriteTest) { } mu2.Unlock(); - ASSERT_EQ(IDChecker::PeekId(), 5u); + ASSERT_EQ(IDChecker::PeekId(), 3u); } TEST_F(ThreadLocalTest, Unref) { - ASSERT_EQ(IDChecker::PeekId(), 2u); + ASSERT_EQ(IDChecker::PeekId(), 0u); auto unref = [](void* ptr) { auto& p = *static_cast(ptr);