Skip to content

Commit

Permalink
kv: acquire Replica shared mutex in tryGetOrCreateReplica
Browse files Browse the repository at this point in the history
This commit switches the common path in `tryGetOrCreateReplica` from
grabbing the Replica mutex in an exclusive mode to grabbing it in a
shared mode. The common path in `tryGetOrCreateReplica` does not mutate
the Replica, so there's no need for the stronger lock.

The locking in `setLastReplicaDescriptors` showed up in a mutex profile
under a write-heavy workload. It was responsible for **2.06%** of mutex
wait time. Grabbing the mutex was probably also slowing down Raft
request processing, as the exclusive lock acquisition had to wait for
other read locks to be dropped.
  • Loading branch information
nvanbenschoten committed Dec 31, 2021
1 parent 3471dc5 commit 36e3163
Showing 1 changed file with 11 additions and 11 deletions.
22 changes: 11 additions & 11 deletions pkg/kv/kvserver/store_create_replica.go
Original file line number Diff line number Diff line change
Expand Up @@ -81,18 +81,18 @@ func (s *Store) tryGetOrCreateReplica(
// The common case: look up an existing (initialized) replica.
if repl, ok := s.mu.replicasByRangeID.Load(rangeID); ok {
repl.raftMu.Lock() // not unlocked on success
repl.mu.Lock()
repl.mu.RLock()

// The current replica is removed, go back around.
if repl.mu.destroyStatus.Removed() {
repl.mu.Unlock()
repl.mu.RUnlock()
repl.raftMu.Unlock()
return nil, false, errRetry
}

// Drop messages from replicas we know to be too old.
if fromReplicaIsTooOld(repl, creatingReplica) {
repl.mu.Unlock()
if fromReplicaIsTooOldRLocked(repl, creatingReplica) {
repl.mu.RUnlock()
repl.raftMu.Unlock()
return nil, false, roachpb.NewReplicaTooOldError(creatingReplica.ReplicaID)
}
Expand All @@ -104,7 +104,7 @@ func (s *Store) tryGetOrCreateReplica(
replicaID, repl)
}

repl.mu.Unlock()
repl.mu.RUnlock()
if err := s.removeReplicaRaftMuLocked(ctx, repl, replicaID, RemoveOptions{
DestroyData: true,
}); err != nil {
Expand All @@ -113,7 +113,7 @@ func (s *Store) tryGetOrCreateReplica(
repl.raftMu.Unlock()
return nil, false, errRetry
}
defer repl.mu.Unlock()
defer repl.mu.RUnlock()

if repl.mu.replicaID > replicaID {
// The sender is behind and is sending to an old replica.
Expand Down Expand Up @@ -238,11 +238,11 @@ func (s *Store) tryGetOrCreateReplica(
return repl, true, nil
}

// isFromReplicaTooOld returns an true if the creatingReplica is deemed to be
// a member of the range which has been removed.
// Assumes toReplica.mu is held.
func fromReplicaIsTooOld(toReplica *Replica, fromReplica *roachpb.ReplicaDescriptor) bool {
toReplica.mu.AssertHeld()
// fromReplicaIsTooOldRLocked returns true if the creatingReplica is deemed to
// be a member of the range which has been removed.
// Assumes toReplica.mu is locked for (at least) reading.
func fromReplicaIsTooOldRLocked(toReplica *Replica, fromReplica *roachpb.ReplicaDescriptor) bool {
toReplica.mu.AssertRHeld()
if fromReplica == nil {
return false
}
Expand Down

0 comments on commit 36e3163

Please sign in to comment.