[kudu] 02/02: tablet: avoid locking for Tablet::state checks

[al...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

alexey pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/kudu.git

commit 4482781a1d46ec3460704313fee2f65c84c9bfbc
Author: Todd Lipcon <to...@apache.org>
AuthorDate: Thu Apr 23 10:58:15 2020 -0700

    tablet: avoid locking for Tablet::state checks
    
    Tablet::state_ is frequently checked during the process of applying
    writes (eg once per row operation). The handling of this lock was taking
    ~7% of the CPU on the Apply thread in a tpch_real_world test run. This
    patch changes it to use an atomic instead, which should remove that CPU
    bottleneck since the state rarely changes.
    
    Change-Id: If0dde4d060caa12c1831f3a19b92ecacf3861b64
    Reviewed-on: http://gerrit.cloudera.org:8080/15792
    Reviewed-by: Alexey Serbin <as...@cloudera.com>
    Tested-by: Kudu Jenkins
---
 src/kudu/tablet/tablet.cc | 31 +++++++++++++++++--------------
 src/kudu/tablet/tablet.h  | 18 +++++++-----------
 2 files changed, 24 insertions(+), 25 deletions(-)

diff --git a/src/kudu/tablet/tablet.cc b/src/kudu/tablet/tablet.cc
index 8aedf8a..cd992c9 100644
--- a/src/kudu/tablet/tablet.cc
+++ b/src/kudu/tablet/tablet.cc
@@ -273,9 +273,9 @@ Tablet::~Tablet() {
 // Returns an error if the Tablet has been stopped, i.e. is 'kStopped' or
 // 'kShutdown', and otherwise checks that 'expected_state' matches 'state_'.
 #define RETURN_IF_STOPPED_OR_CHECK_STATE(expected_state) do { \
-  std::lock_guard<simple_spinlock> l(state_lock_); \
-  RETURN_NOT_OK(CheckHasNotBeenStoppedUnlocked()); \
-  CHECK_EQ(expected_state, state_); \
+  State _local_state; \
+  RETURN_NOT_OK(CheckHasNotBeenStopped(&_local_state)); \
+  CHECK_EQ(expected_state, _local_state); \
 } while (0)
 
 Status Tablet::Open() {
@@ -926,15 +926,18 @@ Status Tablet::BulkCheckPresence(const IOContext* io_context, WriteOpState* op_s
 }
 
 bool Tablet::HasBeenStopped() const {
-  std::lock_guard<simple_spinlock> l(state_lock_);
-  return state_ == kStopped || state_ == kShutdown;
+  State s = state_.load();
+  return s == kStopped || s == kShutdown;
 }
 
-Status Tablet::CheckHasNotBeenStoppedUnlocked() const {
-  DCHECK(state_lock_.is_locked());
-  if (PREDICT_FALSE(state_ == kStopped || state_ == kShutdown)) {
+Status Tablet::CheckHasNotBeenStopped(State* cur_state) const {
+  State s = state_.load();
+  if (PREDICT_FALSE(s == kStopped || s == kShutdown)) {
     return Status::IllegalState("Tablet has been stopped");
   }
+  if (cur_state) {
+    *cur_state = s;
+  }
   return Status::OK();
 }
 
@@ -975,10 +978,10 @@ Status Tablet::ApplyRowOperation(const IOContext* io_context,
   }
 
   {
-    std::lock_guard<simple_spinlock> l(state_lock_);
-    RETURN_NOT_OK_PREPEND(CheckHasNotBeenStoppedUnlocked(),
+    State s;
+    RETURN_NOT_OK_PREPEND(CheckHasNotBeenStopped(&s),
         Substitute("Apply of $0 exited early", op_state->ToString()));
-    CHECK(state_ == kOpen || state_ == kBootstrapping);
+    CHECK(s == kOpen || s == kBootstrapping);
   }
   DCHECK(row_op->has_row_lock()) << "RowOp must hold the row lock.";
   DCHECK(op_state != nullptr) << "must have a WriteOpState";
@@ -1179,9 +1182,9 @@ Status Tablet::ReplaceMemRowSetUnlocked(RowSetsInCompaction *compaction,
 Status Tablet::FlushInternal(const RowSetsInCompaction& input,
                              const shared_ptr<MemRowSet>& old_ms) {
   {
-    std::lock_guard<simple_spinlock> l(state_lock_);
-    RETURN_NOT_OK(CheckHasNotBeenStoppedUnlocked());
-    CHECK(state_ == kOpen || state_ == kBootstrapping);
+    State s;
+    RETURN_NOT_OK(CheckHasNotBeenStopped(&s));
+    CHECK(s == kOpen || s == kBootstrapping);
   }
 
   // Step 1. Freeze the old memrowset by blocking readers and swapping
diff --git a/src/kudu/tablet/tablet.h b/src/kudu/tablet/tablet.h
index efe901d..459b62c 100644
--- a/src/kudu/tablet/tablet.h
+++ b/src/kudu/tablet/tablet.h
@@ -16,6 +16,7 @@
 // under the License.
 #pragma once
 
+#include <atomic>
 #include <cstddef>
 #include <cstdint>
 #include <iosfwd>
@@ -540,14 +541,8 @@ class Tablet {
   }
 
   // Returns an error if the tablet is in the 'kStopped' or 'kShutdown' state.
-  // Must be called while 'state_lock_' is held.
-  Status CheckHasNotBeenStoppedUnlocked() const;
-
-  // Returns an error if the tablet is in the 'kStopped' or 'kShutdown' state.
-  Status CheckHasNotBeenStopped() const {
-    std::lock_guard<simple_spinlock> l(state_lock_);
-    return CheckHasNotBeenStoppedUnlocked();
-  }
+  // Sets *cur_state to the current state, which may be useful for later assertions.
+  Status CheckHasNotBeenStopped(State* cur_state = nullptr) const;
 
   Status FlushUnlocked();
 
@@ -765,12 +760,13 @@ class Tablet {
   // started earlier completes after the one started later.
   mutable Semaphore rowsets_flush_sem_;
 
-  // Lock protecting access to 'state_' and 'maintenance_ops_'.
+  // Lock protecting access to mutate 'state_' and all access to 'maintenance_ops_'.
   // If taken with any other locks, this must be taken last, i.e. no locks can
-  // be acquired while holding this this.
+  // be acquired while holding this lock.
   mutable simple_spinlock state_lock_;
 
-  State state_;
+  // Protected by state_lock_ for transitions, but may be read without holding a lock.
+  std::atomic<State> state_;
 
   // Fake lock used to ensure calls to RegisterMaintenanceOps and
   // UnregisterMaintenanceOps don't overlap. This serves to ensure that only