kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject [3/3] kudu git commit: meta_cache: improve multi-threaded scalability
Date Fri, 30 Mar 2018 21:16:07 GMT
meta_cache: improve multi-threaded scalability

When profiling a multi-threaded 'kudu perf loadgen' I found the
bottleneck was on the rw_spinlock acquisition in
MetaCache::LookupTabletByKeyFastPath as well as some contention in
RemoteTablet::stale().

This fixes the first by switching to a percpu_rwlock and the second by
using an atomic bool instead of a spinlock-protected bool.

This moved LookupTabletByKeyFastPath from the first line of the profile
(14% of CPU) down to the 8th line (3% of CPU).

Change-Id: I38c8a94ea177bfa4f4e2048355464f76a5daa2ba
Reviewed-on: http://gerrit.cloudera.org:8080/9760
Reviewed-by: Adar Dembo <adar@cloudera.com>
Tested-by: Todd Lipcon <todd@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/kudu/commit/4ebfbe41
Tree: http://git-wip-us.apache.org/repos/asf/kudu/tree/4ebfbe41
Diff: http://git-wip-us.apache.org/repos/asf/kudu/diff/4ebfbe41

Branch: refs/heads/master
Commit: 4ebfbe4182d7b9ccbe0bed1b2a3746467c42f05f
Parents: e36a674
Author: Todd Lipcon <todd@apache.org>
Authored: Thu Mar 22 10:55:27 2018 -0700
Committer: Todd Lipcon <todd@apache.org>
Committed: Fri Mar 30 21:06:44 2018 +0000

----------------------------------------------------------------------
 src/kudu/client/meta_cache.cc | 13 +++++--------
 src/kudu/client/meta_cache.h  |  9 +++++----
 src/kudu/util/locks.h         | 20 +++++++++++++++-----
 3 files changed, 25 insertions(+), 17 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/kudu/blob/4ebfbe41/src/kudu/client/meta_cache.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/meta_cache.cc b/src/kudu/client/meta_cache.cc
index aa77741..32f96b2 100644
--- a/src/kudu/client/meta_cache.cc
+++ b/src/kudu/client/meta_cache.cc
@@ -201,12 +201,10 @@ void RemoteTablet::Refresh(const TabletServerMap& tservers,
 }
 
 void RemoteTablet::MarkStale() {
-  std::lock_guard<simple_spinlock> l(lock_);
   stale_ = true;
 }
 
 bool RemoteTablet::stale() const {
-  std::lock_guard<simple_spinlock> l(lock_);
   return stale_;
 }
 
@@ -820,7 +818,7 @@ Status MetaCache::ProcessLookupResponse(const LookupRpc& rpc,
   MonoTime expiration_time = MonoTime::Now() +
       MonoDelta::FromMilliseconds(rpc.resp().ttl_millis());
 
-  std::lock_guard<rw_spinlock> l(lock_);
+  std::lock_guard<percpu_rwlock> l(lock_);
   TabletMap& tablets_by_key = LookupOrInsert(&tablets_by_table_and_key_,
                                              rpc.table_id(), TabletMap());
 
@@ -961,7 +959,7 @@ Status MetaCache::ProcessLookupResponse(const LookupRpc& rpc,
 bool MetaCache::LookupEntryByKeyFastPath(const KuduTable* table,
                                          const string& partition_key,
                                          MetaCacheEntry* entry) {
-  shared_lock<rw_spinlock> l(lock_);
+  shared_lock<rw_spinlock> l(lock_.get_lock());
   const TabletMap* tablets = FindOrNull(tablets_by_table_and_key_, table->id());
   if (PREDICT_FALSE(!tablets)) {
     // No cache available for this table.
@@ -1013,7 +1011,7 @@ Status MetaCache::DoFastPathLookup(const KuduTable* table,
 
 void MetaCache::ClearNonCoveredRangeEntries(const std::string& table_id) {
   VLOG(3) << "Clearing non-covered range entries of table " << table_id;
-  std::lock_guard<rw_spinlock> l(lock_);
+  std::lock_guard<percpu_rwlock> l(lock_);
 
   TabletMap* tablets = FindOrNull(tablets_by_table_and_key_, table_id);
   if (PREDICT_FALSE(!tablets)) {
@@ -1032,7 +1030,7 @@ void MetaCache::ClearNonCoveredRangeEntries(const std::string& table_id)
{
 
 void MetaCache::ClearCache() {
   VLOG(3) << "Clearing cache";
-  std::lock_guard<rw_spinlock> l(lock_);
+  std::lock_guard<percpu_rwlock> l(lock_);
   STLDeleteValues(&ts_cache_);
   tablets_by_id_.clear();
   tablets_by_table_and_key_.clear();
@@ -1068,10 +1066,9 @@ void MetaCache::LookupTabletByKey(const KuduTable* table,
 void MetaCache::MarkTSFailed(RemoteTabletServer* ts,
                              const Status& status) {
   LOG(INFO) << "Marking tablet server " << ts->ToString() << " as failed.";
-  shared_lock<rw_spinlock> l(lock_);
-
   Status ts_status = status.CloneAndPrepend("TS failed");
 
+  shared_lock<rw_spinlock> l(lock_.get_lock());
   // TODO: replace with a ts->tablet multimap for faster lookup?
   for (const auto& tablet : tablets_by_id_) {
     // We just loop on all tablets; if a tablet does not have a replica on this

http://git-wip-us.apache.org/repos/asf/kudu/blob/4ebfbe41/src/kudu/client/meta_cache.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/meta_cache.h b/src/kudu/client/meta_cache.h
index 34232bc..fafd84b 100644
--- a/src/kudu/client/meta_cache.h
+++ b/src/kudu/client/meta_cache.h
@@ -19,6 +19,7 @@
 #ifndef KUDU_CLIENT_META_CACHE_H
 #define KUDU_CLIENT_META_CACHE_H
 
+#include <atomic>
 #include <map>
 #include <memory>
 #include <set>
@@ -257,9 +258,9 @@ class RemoteTablet : public RefCountedThreadSafe<RemoteTablet> {
   const std::string tablet_id_;
   const Partition partition_;
 
-  // All non-const members are protected by 'lock_'.
-  mutable simple_spinlock lock_;
-  bool stale_;
+  std::atomic<bool> stale_;
+
+  mutable simple_spinlock lock_; // Protects replicas_.
   std::vector<RemoteReplica> replicas_;
 
   DISALLOW_COPY_AND_ASSIGN(RemoteTablet);
@@ -461,7 +462,7 @@ class MetaCache : public RefCountedThreadSafe<MetaCache> {
 
   KuduClient* client_;
 
-  rw_spinlock lock_;
+  percpu_rwlock lock_;
 
   // Cache of Tablet Server locations: TS UUID -> RemoteTabletServer*.
   //

http://git-wip-us.apache.org/repos/asf/kudu/blob/4ebfbe41/src/kudu/util/locks.h
----------------------------------------------------------------------
diff --git a/src/kudu/util/locks.h b/src/kudu/util/locks.h
index 51467b9..f70955c 100644
--- a/src/kudu/util/locks.h
+++ b/src/kudu/util/locks.h
@@ -132,9 +132,12 @@ class rw_spinlock {
 // A reader-writer lock implementation which is biased for use cases where
 // the write lock is taken infrequently, but the read lock is used often.
 //
-// Internally, this creates N underlying mutexes, one per CPU. When a thread
-// wants to lock in read (shared) mode, it locks only its own CPU's mutex. When it
-// wants to lock in write (exclusive) mode, it locks all CPU's mutexes.
+// Internally, this creates N underlying reader-writer locks, one per CPU. When a thread
+// wants to lock in read (shared) mode, it locks only its own CPU's lock in read
+// mode. When it wants to lock in write (exclusive) mode, it locks all CPUs' rwlocks in
+// write mode. The use of reader-writer locks ensures that, even if a thread gets
+// preempted when holding one of the per-CPU locks in read mode, the next thread
+// scheduled onto that CPU will not need to block on the first thread.
 //
 // This means that in the read-mostly case, different readers will not cause any
 // cacheline contention.
@@ -144,14 +147,14 @@ class rw_spinlock {
 //
 //   // Lock shared:
 //   {
-//     boost::shared_lock<rw_spinlock> lock(mylock.get_lock());
+//     kudu::shared_lock<rw_spinlock> lock(mylock.get_lock());
 //     ...
 //   }
 //
 //   // Lock exclusive:
 //
 //   {
-//     boost::lock_guard<percpu_rwlock> lock(mylock);
+//     std::lock_guard<percpu_rwlock> lock(mylock);
 //     ...
 //   }
 class percpu_rwlock {
@@ -207,6 +210,13 @@ class percpu_rwlock {
     return false;
   }
 
+  bool is_write_locked() const {
+    for (int i = 0; i < n_cpus_; i++) {
+      if (!locks_[i].lock.is_write_locked()) return false;
+    }
+    return true;
+  }
+
   void lock() {
     for (int i = 0; i < n_cpus_; i++) {
       locks_[i].lock.lock();


Mime
View raw message