From b7d2f2ef5a7d63a487a95cec9d1aac7ab4c1e63d Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Tue, 23 Mar 2021 12:05:40 -0400 Subject: [PATCH] kvccl: re-order enterprise check in canSendToFollower Fixes #62447. In #62447, Erik found that #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. --- pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go b/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go index dfe701a72a01..8d4695a17a5b 100644 --- a/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go +++ b/pkg/ccl/kvccl/kvfollowerreadsccl/followerreads.go @@ -124,9 +124,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 { @@ -181,9 +182,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