roachpb: stop mutating RangeDescriptor when Replicas is called

pkg/cli/debug.go

@@ -1183,7 +1183,7 @@ func removeDeadReplicas(
 				StoreID:   storeIdent.StoreID,
 				ReplicaID: desc.NextReplicaID,
 			}}
-			newDesc.SetReplicas(roachpb.MakeReplicaDescriptors(&replicas))
+			newDesc.SetReplicas(roachpb.MakeReplicaDescriptors(replicas))
 			newDesc.NextReplicaID++
 			fmt.Printf("Replica %s -> %s\n", &desc, &newDesc)
 			newDescs = append(newDescs, newDesc)

pkg/roachpb/metadata.go

@@ -126,7 +126,7 @@ func (r *RangeDescriptor) ContainsKeyRange(start, end RKey) bool {
 // Replicas returns the set of nodes/stores on which replicas of this range are
 // stored.
 func (r *RangeDescriptor) Replicas() ReplicaDescriptors {
-	return MakeReplicaDescriptors(&r.InternalReplicas)
+	return MakeReplicaDescriptors(r.InternalReplicas)
 }
 
 // SetReplicas overwrites the set of nodes/stores on which replicas of this

pkg/roachpb/metadata_replicas.go

@@ -12,7 +12,6 @@ package roachpb
 
 import (
 	"fmt"
-	"sort"
 	"strings"
 )
 
@@ -49,9 +48,8 @@ type ReplicaDescriptors struct {
 // we're trading one allocation for another. However, if the caller already has
 // the slice header on the heap (which is the common case for *RangeDescriptors)
 // then this is a net win.
-func MakeReplicaDescriptors(replicas *[]ReplicaDescriptor) ReplicaDescriptors {
-	sort.Sort((*byTypeThenReplicaID)(replicas))
-	return ReplicaDescriptors{wrapped: *replicas}
+func MakeReplicaDescriptors(replicas []ReplicaDescriptor) ReplicaDescriptors {
+	return ReplicaDescriptors{wrapped: replicas}
 }
 
 func (d ReplicaDescriptors) String() string {
@@ -71,18 +69,34 @@ func (d ReplicaDescriptors) All() []ReplicaDescriptor {
 	return d.wrapped
 }
 
-// Voters returns the voter replicas in the set.
+// Voters returns the voter replicas in the set. This may allocate, but it also
+// may return the underlying slice as a performance optimization, so it's not
+// safe to modify the returned value.
 func (d ReplicaDescriptors) Voters() []ReplicaDescriptor {
-	// Note that the wrapped replicas are sorted first by type.
+	// Fastpath, most of the time, everything is a voter, so special case that and
+	// save the alloc.
+	fastpath := true
 	for i := range d.wrapped {
-		if d.wrapped[i].GetType() == ReplicaType_LEARNER {
-			return d.wrapped[:i]
+		if d.wrapped[i].GetType() != ReplicaType_VOTER {
+			fastpath = false
+			break
 		}
 	}
-	return d.wrapped
+	if fastpath {
+		return d.wrapped
+	}
+	voters := make([]ReplicaDescriptor, 0, len(d.wrapped))
+	for i := range d.wrapped {
+		if d.wrapped[i].GetType() == ReplicaType_VOTER {
+			voters = append(voters, d.wrapped[i])
+		}
+	}
+	return voters
 }
 
-// Learners returns the learner replicas in the set.
+// Learners returns the learner replicas in the set. This may allocate, but it
+// also may return the underlying slice as a performance optimization, so it's
+// not safe to modify the returned value.
 //
 // A learner is a participant in a raft group that accepts messages but doesn't
 // vote. This means it doesn't affect raft quorum and thus doesn't affect the
@@ -167,13 +181,18 @@ func (d ReplicaDescriptors) Voters() []ReplicaDescriptor {
 //
 // For some related mega-comments, see Replica.sendSnapshot.
 func (d ReplicaDescriptors) Learners() []ReplicaDescriptor {
-	// Note that the wrapped replicas are sorted first by type.
+	// Fastpath, most of the time, everything is a voter, so special case that and
+	// save the alloc.
+	var learners []ReplicaDescriptor
 	for i := range d.wrapped {
 		if d.wrapped[i].GetType() == ReplicaType_LEARNER {
-			return d.wrapped[i:]
+			if learners == nil {
+				learners = make([]ReplicaDescriptor, 0, len(d.wrapped)-i)
+			}
+			learners = append(learners, d.wrapped[i])
 		}
 	}
-	return nil
+	return learners
 }
 
 // AsProto returns the protobuf representation of these replicas, suitable for
@@ -216,24 +235,17 @@ func (d *ReplicaDescriptors) RemoveReplica(
 	d.wrapped[idx], d.wrapped[len(d.wrapped)-1] = d.wrapped[len(d.wrapped)-1], d.wrapped[idx]
 	removed := d.wrapped[len(d.wrapped)-1]
 	d.wrapped = d.wrapped[:len(d.wrapped)-1]
-	// The swap may have broken our sortedness invariant, so re-sort.
-	sort.Sort((*byTypeThenReplicaID)(&d.wrapped))
 	return removed, true
 }
 
 // QuorumSize returns the number of voter replicas required for quorum in a raft
 // group consisting of this set of replicas.
 func (d ReplicaDescriptors) QuorumSize() int {
-	return (len(d.Voters()) / 2) + 1
-}
-
-type byTypeThenReplicaID []ReplicaDescriptor
-
-func (x *byTypeThenReplicaID) Len() int      { return len(*x) }
-func (x *byTypeThenReplicaID) Swap(i, j int) { (*x)[i], (*x)[j] = (*x)[j], (*x)[i] }
-func (x *byTypeThenReplicaID) Less(i, j int) bool {
-	if (*x)[i].GetType() == (*x)[j].GetType() {
-		return (*x)[i].ReplicaID < (*x)[j].ReplicaID
+	var numVoters int
+	for i := range d.wrapped {
+		if d.wrapped[i].GetType() == ReplicaType_VOTER {
+			numVoters++
+		}
 	}
-	return (*x)[i].GetType() < (*x)[j].GetType()
+	return (numVoters / 2) + 1
 }

pkg/roachpb/metadata_replicas_test.go

@@ -30,7 +30,7 @@ func TestVotersLearnersAll(t *testing.T) {
 		{{Type: learner}, {Type: nil}, {Type: learner}},
 	}
 	for i, test := range tests {
-		r := MakeReplicaDescriptors(&test)
+		r := MakeReplicaDescriptors(test)
 		for _, voter := range r.Voters() {
 			assert.Equal(t, ReplicaType_VOTER, voter.GetType(), "testcase %d", i)
 		}
@@ -74,7 +74,7 @@ func TestReplicaDescriptorsRemove(t *testing.T) {
 		},
 	}
 	for i, test := range tests {
-		r := MakeReplicaDescriptors(&test.replicas)
+		r := MakeReplicaDescriptors(test.replicas)
 		lenBefore := len(r.All())
 		removedDesc, ok := r.RemoveReplica(test.remove.NodeID, test.remove.StoreID)
 		assert.Equal(t, test.expected, ok, "testcase %d", i)

pkg/storage/batcheval/cmd_end_transaction.go

@@ -1112,7 +1112,7 @@ func changeReplicasTrigger(
 		desc = *change.Desc
 	} else {
 		desc = *rec.Desc()
-		desc.SetReplicas(roachpb.MakeReplicaDescriptors(&change.DeprecatedUpdatedReplicas))
+		desc.SetReplicas(roachpb.MakeReplicaDescriptors(change.DeprecatedUpdatedReplicas))
 		desc.NextReplicaID = change.DeprecatedNextReplicaID
 	}
 

pkg/storage/batcheval/cmd_lease_test.go

@@ -117,7 +117,7 @@ func TestLeaseCommandLearnerReplica(t *testing.T) {
 		{StoreID: learnerStoreID, Type: roachpb.ReplicaTypeLearner()},
 	}
 	desc := roachpb.RangeDescriptor{}
-	desc.SetReplicas(roachpb.MakeReplicaDescriptors(&replicas))
+	desc.SetReplicas(roachpb.MakeReplicaDescriptors(replicas))
 	cArgs := CommandArgs{
 		EvalCtx: &mockEvalCtx{
 			storeID: learnerStoreID,

pkg/storage/replicate_queue.go

@@ -295,8 +295,9 @@ func (rq *replicateQueue) processOneChange(
 
 	// Avoid taking action if the range has too many dead replicas to make
 	// quorum.
+	voterReplicas := desc.Replicas().Voters()
 	liveVoterReplicas, deadVoterReplicas := rq.allocator.storePool.liveAndDeadReplicas(
-		desc.RangeID, desc.Replicas().Voters())
+		desc.RangeID, voterReplicas)
 	{
 		quorum := desc.Replicas().QuorumSize()
 		if lr := len(liveVoterReplicas); lr < quorum {
@@ -315,7 +316,6 @@ func (rq *replicateQueue) processOneChange(
 	if action == AllocatorRemoveLearner {
 		return rq.removeLearner(ctx, repl, dryRun)
 	}
-	voterReplicas := desc.Replicas().Voters()
 
 	switch action {
 	case AllocatorNoop, AllocatorRangeUnavailable:

pkg/storage/store.go

@@ -2086,7 +2086,7 @@ func (s *Store) NewRangeDescriptor(
 		EndKey:        end,
 		NextReplicaID: roachpb.ReplicaID(len(repls) + 1),
 	}
-	desc.SetReplicas(roachpb.MakeReplicaDescriptors(&repls))
+	desc.SetReplicas(roachpb.MakeReplicaDescriptors(repls))
 	return desc, nil
 }
 

pkg/storage/store_bootstrap.go

@@ -160,7 +160,7 @@ func WriteInitialClusterData(
 				ReplicaID: 1,
 			},
 		}
-		desc.SetReplicas(roachpb.MakeReplicaDescriptors(&replicas))
+		desc.SetReplicas(roachpb.MakeReplicaDescriptors(replicas))
 		if err := desc.Validate(); err != nil {
 			return err
 		}

pkg/storage/store_test.go

@@ -1413,7 +1413,7 @@ func TestStoreRangeIDAllocation(t *testing.T) {
 		replicas := []roachpb.ReplicaDescriptor{{StoreID: store.StoreID()}}
 		desc, err := store.NewRangeDescriptor(context.Background(),
 			roachpb.RKey(fmt.Sprintf("%03d", i)), roachpb.RKey(fmt.Sprintf("%03d", i+1)),
-			roachpb.MakeReplicaDescriptors(&replicas))
+			roachpb.MakeReplicaDescriptors(replicas))
 		if err != nil {
 			t.Fatal(err)
 		}