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 <[email protected]> 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 <[email protected]> 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
