Skip to content

Commit

Permalink
kvccl: re-order enterprise check in canSendToFollower
Browse files Browse the repository at this point in the history
Fixes cockroachdb#62447.

In cockroachdb#62447, Erik found that cockroachdb#59571 had re-ordered the call to
`utilccl.CheckEnterpriseEnabled` to occur before checking the batch in
`canSendToFollower`, instead of after. This added an error allocation
into the hot path of all reads, which showed up in CPU profiles and
caused an 8% performance regression on `kv95`. This commit fixes this by
moving the enterprise check back out of the hot-path for all non-stale
read-only batches.

A follow up to this PR would be to make `utilccl.CheckEnterpriseEnabled`
cheaper by avoiding the error allocation for callers that don't need an
error. This work is not done in this commit.
  • Loading branch information
nvanbenschoten committed Mar 25, 2021
1 parent 71bcdc3 commit 59782f8
Showing 1 changed file with 8 additions and 6 deletions.
14 changes: 8 additions & 6 deletions pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,10 @@ func canSendToFollower(
ctPolicy roachpb.RangeClosedTimestampPolicy,
ba roachpb.BatchRequest,
) bool {
return checkFollowerReadsEnabled(clusterID, st) &&
kvserver.BatchCanBeEvaluatedOnFollower(ba) &&
closedTimestampLikelySufficient(st, clock, ctPolicy, ba.Txn.RequiredFrontier())
return kvserver.BatchCanBeEvaluatedOnFollower(ba) &&
closedTimestampLikelySufficient(st, clock, ctPolicy, ba.Txn.RequiredFrontier()) &&
// NOTE: this call can be expensive, so perform it last. See #62447.
checkFollowerReadsEnabled(clusterID, st)
}

type followerReadOracle struct {
Expand Down Expand Up @@ -188,9 +189,10 @@ func (o *followerReadOracle) useClosestOracle(
// sent to the correct replicas once canSendToFollower is checked for each
// BatchRequests in the DistSender. This would hurt performance, but would
// not violate correctness.
return checkFollowerReadsEnabled(o.clusterID.Get(), o.st) &&
txn != nil &&
closedTimestampLikelySufficient(o.st, o.clock, ctPolicy, txn.RequiredFrontier())
return txn != nil &&
closedTimestampLikelySufficient(o.st, o.clock, ctPolicy, txn.RequiredFrontier()) &&
// NOTE: this call can be expensive, so perform it last. See #62447.
checkFollowerReadsEnabled(o.clusterID.Get(), o.st)
}

// followerReadOraclePolicy is a leaseholder choosing policy that detects
Expand Down

0 comments on commit 59782f8

Please sign in to comment.