kv,server,roachpb: avoid error overhead for x-locality comparison #113229

kvoli · 2023-10-27T17:05:13Z

Cross locality traffic instrumentation was added to raft, snapshots and batch requests to quantify the amount of cross region/zone traffic. Errors would be returned from CompareWithLocality when the region, or zone locality flags were set in an unsupported manner according to our documentation. These error allocations added overhead (cpu/mem) when hit.

Alter CompareWithLocality to return booleans in place of an error to reduce overhead.

Resolves: #111148
Resolves: #111142
Informs: #111561
Release note: None

cockroach-teamcity · 2023-10-27T17:05:23Z

This change is

Cross locality traffic instrumentation was added to raft, snapshots and batch requests to quantify the amount of cross region/zone traffic. Errors would be returned from `CompareWithLocality` when the region, or zone locality flags were set in an unsupported manner according to our documentation. These error allocations added overhead (cpu/mem) when hit. Alter `CompareWithLocality` to return booleans in place of an error to reduce overhead. Resolves: cockroachdb#111148 Resolves: cockroachdb#111142 Informs: cockroachdb#111561 Release note: None

kvoli · 2023-10-30T13:14:16Z

Benchmarking the comparison of this commit cherry-picked on relase-23.2 and the predecessor:

$ benchdiff --old=90ddb8a960dd11da7c72286268cb42fd656dd3b3  --new=684b9cab0c342ed385c5968c8da4fd5eb46af597 --run=BenchmarkColBatchScan --memprofile --count=20 ./pkg/sql/colflow
checking out '90ddb8a'
building benchmark binaries for '90ddb8a' 1/1 /
checking out '684b9ca'
building benchmark binaries for '684b9ca' 1/1 |

  pkg=1/1 iter=20/20 cockroachdb/cockroach/pkg/sql/colflow \

name                        old time/op    new time/op    delta
ColBatchScan/rows=16-24        123µs ± 6%      76µs ± 5%  -38.27%  (p=0.000 n=19+20)
ColBatchScan/rows=256-24       221µs ± 3%     174µs ± 5%  -21.18%  (p=0.000 n=19+18)
ColBatchScan/rows=4096-24     1.44ms ± 1%    1.41ms ± 1%   -2.66%  (p=0.000 n=19+19)
ColBatchScan/rows=65536-24    22.1ms ± 1%    22.2ms ± 1%     ~     (p=0.201 n=19+19)

name                        old speed      new speed      delta
ColBatchScan/rows=16-24     2.08MB/s ± 6%  3.37MB/s ± 5%  +62.01%  (p=0.000 n=19+20)
ColBatchScan/rows=256-24    18.5MB/s ± 3%  23.5MB/s ± 5%  +26.91%  (p=0.000 n=19+18)
ColBatchScan/rows=4096-24   45.4MB/s ± 1%  46.6MB/s ± 1%   +2.73%  (p=0.000 n=19+19)
ColBatchScan/rows=65536-24  47.4MB/s ± 1%  47.2MB/s ± 1%     ~     (p=0.199 n=19+19)

name                        old alloc/op   new alloc/op   delta
ColBatchScan/rows=16-24       11.3kB ± 1%     7.8kB ± 1%  -31.19%  (p=0.000 n=18+20)
ColBatchScan/rows=256-24      29.5kB ± 0%    27.8kB ± 0%   -5.73%  (p=0.000 n=20+19)
ColBatchScan/rows=65536-24    2.18MB ± 0%    2.20MB ± 1%   +0.86%  (p=0.000 n=19+18)
ColBatchScan/rows=4096-24      192kB ± 0%     204kB ± 7%   +6.32%  (p=0.000 n=18+20)

name                        old allocs/op  new allocs/op  delta
ColBatchScan/rows=16-24          161 ± 1%       108 ± 0%  -32.83%  (p=0.000 n=18+20)
ColBatchScan/rows=256-24         178 ± 0%       157 ± 0%  -11.65%  (p=0.000 n=20+19)
ColBatchScan/rows=4096-24        213 ± 0%       292 ±34%  +36.97%  (p=0.001 n=17+20)
ColBatchScan/rows=65536-24       279 ± 7%       419 ±24%  +50.39%  (p=0.000 n=19+18)

pav-kv

Thanks for the fix.

pav-kv · 2023-10-30T14:41:52Z

pkg/roachpb/metadata.go

@@ -707,21 +707,23 @@ func (l Locality) CompareWithLocality(

 	if !hasRegion || !hasRegionOther {
 		isCrossRegion = false
-		regionErr = errors.Errorf("localities must have a valid region tier key for cross-region comparison")


Alternatively, have you considered making these errors.New instead of errors.Errorf? I wonder if the allocation behaviour is any different in this case. Probably not much better.

I didn't consider that, I think it might help moderately, but they both seem to end up at errutil.NewWithDepthf(), with depth=1.

pav-kv · 2023-10-30T14:50:56Z

pkg/roachpb/metadata.go

@@ -698,7 +698,7 @@ func (l Locality) getFirstRegionFirstZone() (
 // iteration.
 func (l Locality) CompareWithLocality(


Side note (irrelevant to this PR): this function seems unnecessarily complex for what it does. It tries to return something meaningful even in error cases, which is a recipe for combinatorial boost and bugs. The boolean logic is hard to follow.

Ack, I agree. I considered moving the validation/error logic into a separate function, which would be called on node start / periodically / v(5) return path.

We can clean this up in the future.

kvoli · 2023-10-30T17:21:11Z

TYFTR

bors r=pavelkalinnikov

craig · 2023-10-30T18:10:04Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-10-30T18:10:16Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error setting reviewers, but backport branch blathers/backport-release-23.2-113229 is ready: POST https://api.github.com/repos/cockroachdb/cockroach/pulls/113302/requested_reviewers: 422 Reviews may only be requested from collaborators. One or more of the teams you specified is not a collaborator of the cockroachdb/cockroach repository. []

Backport to branch 23.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

kvoli · 2023-10-30T18:16:01Z

blathers backport 23.2

blathers-crl · 2023-10-30T18:16:05Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating backport branch refs/heads/blathers/backport-release-23.2-113229: POST https://api.github.com/repos/cockroachdb/cockroach/git/refs: 422 Reference already exists []

Backport to branch 23.2 failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

kvoli self-assigned this Oct 27, 2023

kvoli added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Oct 27, 2023

kvoli force-pushed the 231027.x-locality-metric-perf branch from b86e5c0 to a760335 Compare October 27, 2023 17:39

kvoli marked this pull request as ready for review October 30, 2023 14:10

kvoli requested review from a team as code owners October 30, 2023 14:10

kvoli requested a review from erikgrinaker October 30, 2023 14:10

erikgrinaker requested review from pav-kv and removed request for erikgrinaker October 30, 2023 14:35

pav-kv approved these changes Oct 30, 2023

View reviewed changes

craig bot merged commit dd2628e into cockroachdb:master Oct 30, 2023
3 checks passed

blathers-crl bot mentioned this pull request Oct 30, 2023

release-23.2: kv,server,roachpb: avoid error overhead for x-locality comparison #113302

Merged

kvoli removed request for a team October 30, 2023 18:15

pav-kv mentioned this pull request Oct 31, 2023

kvserver: 23.2 replication microbenchmark regressions #111561

Closed

7 tasks

arulajmani mentioned this pull request Dec 13, 2023

kv: triage microbenchmark regressions for v23.2 #114310

Closed

17 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kv,server,roachpb: avoid error overhead for x-locality comparison #113229

kv,server,roachpb: avoid error overhead for x-locality comparison #113229

kvoli commented Oct 27, 2023

cockroach-teamcity commented Oct 27, 2023

kvoli commented Oct 30, 2023

pav-kv left a comment

pav-kv Oct 30, 2023

kvoli Oct 30, 2023

pav-kv Oct 30, 2023

kvoli Oct 30, 2023

kvoli commented Oct 30, 2023

craig bot commented Oct 30, 2023

blathers-crl bot commented Oct 30, 2023

kvoli commented Oct 30, 2023

blathers-crl bot commented Oct 30, 2023

		@@ -698,7 +698,7 @@ func (l Locality) getFirstRegionFirstZone() (
		// iteration.
		func (l Locality) CompareWithLocality(

kv,server,roachpb: avoid error overhead for x-locality comparison #113229

kv,server,roachpb: avoid error overhead for x-locality comparison #113229

Conversation

kvoli commented Oct 27, 2023

cockroach-teamcity commented Oct 27, 2023

kvoli commented Oct 30, 2023

pav-kv left a comment

Choose a reason for hiding this comment

pav-kv Oct 30, 2023

Choose a reason for hiding this comment

kvoli Oct 30, 2023

Choose a reason for hiding this comment

pav-kv Oct 30, 2023

Choose a reason for hiding this comment

kvoli Oct 30, 2023

Choose a reason for hiding this comment

kvoli commented Oct 30, 2023

craig bot commented Oct 30, 2023

blathers-crl bot commented Oct 30, 2023

kvoli commented Oct 30, 2023

blathers-crl bot commented Oct 30, 2023