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 386de6caeb19dd490654abff1195e11b5d9625ca Author: Todd Lipcon <[email protected]> AuthorDate: Thu Apr 23 10:54:41 2020 -0700 transaction: avoid locking for TransactionState::timestamp_ This field is already synchronized externally for mutations by the transaction lifecycle (we never have a case where multiple threads are trying to concurrently assign a timestamp to a transaction). The timestamp might be concurrently read (eg by ToString() for the /transactions web page) but an atomic variable is sufficient for this use case. I saw a couple percent of the Apply thread's CPU going to managing this locking, which should go away with this change. Change-Id: I2c6bc8b85a066426bcf4bc2d1c058bc75c1c38ec Reviewed-on: http://gerrit.cloudera.org:8080/15791 Reviewed-by: Alexey Serbin <[email protected]> Tested-by: Kudu Jenkins --- src/kudu/common/timestamp.h | 4 ++-- src/kudu/tablet/ops/op.cc | 3 ++- src/kudu/tablet/ops/op.h | 20 ++++++++++---------- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/src/kudu/common/timestamp.h b/src/kudu/common/timestamp.h index 4cf045a..43d2f78 100644 --- a/src/kudu/common/timestamp.h +++ b/src/kudu/common/timestamp.h @@ -33,9 +33,9 @@ class Timestamp { public: typedef uint64_t val_type; - Timestamp() : v(kInvalidTimestamp.v) {} + Timestamp() noexcept : v(kInvalidTimestamp.v) {} - explicit Timestamp(uint64_t val) : v(val) {} + explicit Timestamp(uint64_t val) noexcept : v(val) {} // Decode a timestamp from the given input slice. // Mutates the slice to point after the decoded timestamp. diff --git a/src/kudu/tablet/ops/op.cc b/src/kudu/tablet/ops/op.cc index 212da44..78be15d 100644 --- a/src/kudu/tablet/ops/op.cc +++ b/src/kudu/tablet/ops/op.cc @@ -31,7 +31,8 @@ Op::Op(DriverType type, OpType op_type) OpState::OpState(TabletReplica* tablet_replica) : tablet_replica_(tablet_replica), - completion_clbk_(new OpCompletionCallback()), + completion_clbk_(new OpCompletionCallback), + timestamp_(Timestamp()), timestamp_error_(0), arena_(1024), external_consistency_mode_(CLIENT_PROPAGATED) { diff --git a/src/kudu/tablet/ops/op.h b/src/kudu/tablet/ops/op.h index f6aa439..d7f0a70 100644 --- a/src/kudu/tablet/ops/op.h +++ b/src/kudu/tablet/ops/op.h @@ -16,10 +16,10 @@ // under the License. #pragma once +#include <atomic> #include <cstddef> #include <cstdint> #include <memory> -#include <mutex> #include <string> #include <utility> @@ -50,9 +50,9 @@ class Message; namespace kudu { namespace tablet { -class TabletReplica; class OpCompletionCallback; class OpState; +class TabletReplica; // All metrics associated with a Op. struct OpMetrics { @@ -227,20 +227,18 @@ class OpState { // Sets the timestamp for the op virtual void set_timestamp(const Timestamp& timestamp) { // make sure we set the timestamp only once - std::lock_guard<simple_spinlock> l(op_state_lock_); DCHECK_EQ(timestamp_, Timestamp::kInvalidTimestamp); timestamp_ = timestamp; } Timestamp timestamp() const { - std::lock_guard<simple_spinlock> l(op_state_lock_); - DCHECK(timestamp_ != Timestamp::kInvalidTimestamp); - return timestamp_; + Timestamp t = timestamp_.load(); + DCHECK(t != Timestamp::kInvalidTimestamp); + return t; } bool has_timestamp() const { - std::lock_guard<simple_spinlock> l(op_state_lock_); - return timestamp_ != Timestamp::kInvalidTimestamp; + return timestamp_.load() != Timestamp::kInvalidTimestamp; } consensus::OpId* mutable_op_id() { @@ -285,8 +283,10 @@ class OpState { AutoReleasePool pool_; - // This op's timestamp. Protected by op_state_lock_. - Timestamp timestamp_; + // This operation's timestamp. + // This is only set once during the operation lifecycle, using external synchronization. + // However, it may be concurrently read by ToString(), etc, so it's atomic. + std::atomic<Timestamp> timestamp_; // The clock error when timestamp_ was read. uint64_t timestamp_error_;
