Skip to content

Commit

Permalink
kv: access HLC clock once per Replica tick
Browse files Browse the repository at this point in the history
This is a small refactor that eliminates a call to `HLC.Now` on each replica
tick. Due to the issue fixed by the following commit, we were ticking 1000s of
uninitialized replicas in fast succession and as a result, these clock accesses
were a major source of mutex contention. We shouldn't be ticking uninitialized
replicas, but since we now know that this is an expensive part of Replica.tick,
we might as well make it cheaper.
  • Loading branch information
nvanbenschoten committed Jan 10, 2022
1 parent aabce20 commit 376f550
Show file tree
Hide file tree
Showing 4 changed files with 13 additions and 7 deletions.
6 changes: 4 additions & 2 deletions pkg/kv/kvserver/replica.go
Original file line number Diff line number Diff line change
Expand Up @@ -1752,11 +1752,13 @@ func (r *Replica) maybeWatchForMergeLocked(ctx context.Context) (bool, error) {
// facilitates quick command application (requests generally need to make it to
// both the lease holder and the raft leader before being applied by other
// replicas).
func (r *Replica) maybeTransferRaftLeadershipToLeaseholderLocked(ctx context.Context) {
func (r *Replica) maybeTransferRaftLeadershipToLeaseholderLocked(
ctx context.Context, now hlc.ClockTimestamp,
) {
if r.store.TestingKnobs().DisableLeaderFollowsLeaseholder {
return
}
status := r.leaseStatusAtRLocked(ctx, r.Clock().NowAsClockTimestamp())
status := r.leaseStatusAtRLocked(ctx, now)
if !status.IsValid() || status.OwnedBy(r.StoreID()) {
return
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/replica_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -474,7 +474,7 @@ func (r *Replica) leasePostApplyLocked(
// If we're the current raft leader, may want to transfer the leadership to
// the new leaseholder. Note that this condition is also checked periodically
// when ticking the replica.
r.maybeTransferRaftLeadershipToLeaseholderLocked(ctx)
r.maybeTransferRaftLeadershipToLeaseholderLocked(ctx, now)

// Notify the store that a lease change occurred and it may need to
// gossip the updated store descriptor (with updated capacity).
Expand Down
6 changes: 4 additions & 2 deletions pkg/kv/kvserver/replica_raft.go
Original file line number Diff line number Diff line change
Expand Up @@ -979,11 +979,13 @@ func (r *Replica) tick(ctx context.Context, livenessMap liveness.IsLiveMap) (boo
if r.mu.quiescent {
return false, nil
}
if r.maybeQuiesceLocked(ctx, livenessMap) {

now := r.store.Clock().NowAsClockTimestamp()
if r.maybeQuiesceLocked(ctx, now, livenessMap) {
return false, nil
}

r.maybeTransferRaftLeadershipToLeaseholderLocked(ctx)
r.maybeTransferRaftLeadershipToLeaseholderLocked(ctx, now)

// For followers, we update lastUpdateTimes when we step a message from them
// into the local Raft group. The leader won't hit that path, so we update
Expand Down
6 changes: 4 additions & 2 deletions pkg/kv/kvserver/replica_raft_quiesce.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,8 +150,10 @@ func (r *Replica) unquiesceAndWakeLeaderLocked() {
// would quiesce. The fallout from this situation are undesirable raft
// elections which will cause throughput hiccups to the range, but not
// correctness issues.
func (r *Replica) maybeQuiesceLocked(ctx context.Context, livenessMap liveness.IsLiveMap) bool {
status, lagging, ok := shouldReplicaQuiesce(ctx, r, r.store.Clock().NowAsClockTimestamp(), livenessMap)
func (r *Replica) maybeQuiesceLocked(
ctx context.Context, now hlc.ClockTimestamp, livenessMap liveness.IsLiveMap,
) bool {
status, lagging, ok := shouldReplicaQuiesce(ctx, r, now, livenessMap)
if !ok {
return false
}
Expand Down

0 comments on commit 376f550

Please sign in to comment.