From 48983084c954f010327d728dfd1b86dbd51b5bda Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Fri, 13 Dec 2024 22:12:28 -0500 Subject: [PATCH 1/3] kv: add TestBatch{Request|Response}EmptySize This commit adds a pair of tests to measure the size of empty BatchRequest and BatchResponse messages when serialized. Epic: None Release note: None --- pkg/kv/kvpb/api_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/pkg/kv/kvpb/api_test.go b/pkg/kv/kvpb/api_test.go index 00ad467cc4cb..2f6459280b80 100644 --- a/pkg/kv/kvpb/api_test.go +++ b/pkg/kv/kvpb/api_test.go @@ -432,3 +432,13 @@ func TestRequestHeaderRoundTrip(t *testing.T) { require.Equal(t, exp, rh.KVNemesisSeq.Get()) } + +func TestBatchRequestEmptySize(t *testing.T) { + ba := &BatchRequest{} + require.Equal(t, 28, ba.Size()) +} + +func TestBatchResponseEmptySize(t *testing.T) { + br := &BatchResponse{} + require.Equal(t, 6, br.Size()) +} From 12dc32ec4ec557d83444c3ed344847587dc8dfbf Mon Sep 17 00:00:00 2001 From: Matt Whelan Date: Mon, 16 Dec 2024 08:49:06 -0800 Subject: [PATCH 2/3] metrics: address race in MetricVec collection code path The *Vec metrics employ a RWLock to control concurrent access to the set of labels they maintain, but we were reading that set without holding the lock during metric collections. Fixes: #137396 Release note: None --- pkg/util/metric/metric.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/pkg/util/metric/metric.go b/pkg/util/metric/metric.go index 4ffd6706c0c2..a7162f211a81 100644 --- a/pkg/util/metric/metric.go +++ b/pkg/util/metric/metric.go @@ -1361,6 +1361,8 @@ func (gv *GaugeVec) GetType() *prometheusgo.MetricType { // ToPrometheusMetrics implements PrometheusExportable. func (gv *GaugeVec) ToPrometheusMetrics() []*prometheusgo.Metric { + gv.RLock() + defer gv.RUnlock() metrics := make([]*prometheusgo.Metric, 0, len(gv.encounteredLabelValues)) for _, labels := range gv.encounteredLabelValues { @@ -1462,6 +1464,8 @@ func (cv *CounterVec) GetType() *prometheusgo.MetricType { // ToPrometheusMetrics implements PrometheusExportable. func (cv *CounterVec) ToPrometheusMetrics() []*prometheusgo.Metric { + cv.RLock() + defer cv.RUnlock() metrics := make([]*prometheusgo.Metric, 0, len(cv.encounteredLabelValues)) for _, labels := range cv.encounteredLabelValues { @@ -1542,6 +1546,8 @@ func (hv *HistogramVec) GetType() *prometheusgo.MetricType { // ToPrometheusMetrics implements PrometheusExportable. func (hv *HistogramVec) ToPrometheusMetrics() []*prometheusgo.Metric { + hv.RLock() + defer hv.RUnlock() metrics := make([]*prometheusgo.Metric, 0, len(hv.encounteredLabelValues)) for _, labels := range hv.encounteredLabelValues { From cabfad2333ec41950ee9c49bddd306cb9d253cca Mon Sep 17 00:00:00 2001 From: Nathan VanBenschoten Date: Fri, 13 Dec 2024 22:19:59 -0500 Subject: [PATCH 3/3] protobuf: don't serialize zero-valued std duration MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Picks up https://github.com/cockroachdb/gogoproto/pull/10. > This commit updates the serialization of `time.Duration` (see > `gogo.stdduration`) to avoid serializing the default (zero) value in proto3. In > Go, `time.Duration` is a typedef of int64, so it was surprising that unlike > int64, the zero value of time.Duration fields were being put on the wire. > > While here, we also fix `DurationFromProto` to not cause its input to escape to > the heap by passing it through `fmt.Errorf`. This function is called from > `StdDurationUnmarshal` and its unintentional heap allocation what alerted me to > the zero-value serialization issue. Performance improvement on sysbench microbenchmarks: ``` name old time/op new time/op delta Sysbench/SQL/1node_remote/oltp_read_only-10 3.09ms ±12% 3.08ms ± 9% ~ (p=0.905 n=9+10) Sysbench/SQL/1node_remote/oltp_write_only-10 1.97ms ± 4% 1.96ms ± 3% ~ (p=0.278 n=9+10) Sysbench/SQL/1node_remote/oltp_read_write-10 5.73ms ± 1% 5.73ms ± 1% ~ (p=1.000 n=7+8) Sysbench/SQL/1node_remote/oltp_point_select-10 180µs ± 6% 181µs ±14% ~ (p=0.853 n=10+10) Sysbench/KV/1node_remote/oltp_read_only-10 736µs ± 3% 729µs ± 3% ~ (p=0.190 n=10+10) Sysbench/KV/1node_remote/oltp_write_only-10 594µs ± 2% 587µs ± 4% ~ (p=0.211 n=9+10) Sysbench/KV/1node_remote/oltp_read_write-10 1.44ms ± 6% 1.42ms ± 3% ~ (p=0.156 n=10+9) Sysbench/KV/1node_remote/oltp_point_select-10 33.7µs ±11% 34.9µs ± 9% ~ (p=0.247 n=10+10) name old alloc/op new alloc/op delta Sysbench/KV/1node_remote/oltp_point_select-10 6.69kB ± 1% 6.61kB ± 1% -1.18% (p=0.001 n=10+9) Sysbench/SQL/1node_remote/oltp_point_select-10 30.0kB ± 0% 29.7kB ± 1% -0.87% (p=0.000 n=8+10) Sysbench/SQL/1node_remote/oltp_write_only-10 476kB ± 0% 474kB ± 0% -0.61% (p=0.000 n=10+9) Sysbench/KV/1node_remote/oltp_write_only-10 230kB ± 0% 229kB ± 0% -0.53% (p=0.000 n=10+10) Sysbench/SQL/1node_remote/oltp_read_write-10 1.62MB ± 0% 1.62MB ± 0% -0.34% (p=0.000 n=10+10) Sysbench/KV/1node_remote/oltp_read_only-10 647kB ± 0% 645kB ± 0% -0.25% (p=0.000 n=10+10) Sysbench/KV/1node_remote/oltp_read_write-10 872kB ± 0% 870kB ± 0% -0.25% (p=0.000 n=10+9) Sysbench/SQL/1node_remote/oltp_read_only-10 1.15MB ± 0% 1.15MB ± 0% -0.23% (p=0.000 n=10+10) name old allocs/op new allocs/op delta Sysbench/KV/1node_remote/oltp_point_select-10 57.0 ± 0% 55.0 ± 0% -3.51% (p=0.000 n=10+9) Sysbench/KV/1node_remote/oltp_read_only-10 1.83k ± 0% 1.80k ± 0% -1.54% (p=0.000 n=10+10) Sysbench/KV/1node_remote/oltp_read_write-10 3.33k ± 0% 3.29k ± 0% -1.43% (p=0.000 n=10+9) Sysbench/SQL/1node_remote/oltp_write_only-10 3.79k ± 0% 3.74k ± 0% -1.38% (p=0.000 n=10+9) Sysbench/KV/1node_remote/oltp_write_only-10 1.51k ± 0% 1.49k ± 0% -1.37% (p=0.000 n=10+10) Sysbench/SQL/1node_remote/oltp_read_write-10 8.40k ± 0% 8.31k ± 0% -1.13% (p=0.000 n=10+10) Sysbench/SQL/1node_remote/oltp_point_select-10 253 ± 0% 251 ± 1% -1.03% (p=0.000 n=10+10) Sysbench/SQL/1node_remote/oltp_read_only-10 4.60k ± 0% 4.56k ± 0% -0.89% (p=0.000 n=10+10) ``` Epic: None Release note: None --- DEPS.bzl | 6 +++--- build/bazelutil/distdir_files.bzl | 2 +- go.mod | 2 +- go.sum | 4 ++-- pkg/kv/kvpb/api_test.go | 2 +- .../testdata/lock_table/add_discovered | 2 +- .../concurrency/testdata/lock_table/basic | 4 ++-- .../concurrency/testdata/lock_table/dup_access | 2 +- .../concurrency/testdata/lock_table/query | 18 +++++++++--------- 9 files changed, 21 insertions(+), 21 deletions(-) diff --git a/DEPS.bzl b/DEPS.bzl index f7b076a97edf..ab008d04cbc9 100644 --- a/DEPS.bzl +++ b/DEPS.bzl @@ -3880,10 +3880,10 @@ def go_deps(): patches = [ "@com_github_cockroachdb_cockroach//build/patches:com_github_gogo_protobuf.patch", ], - sha256 = "dcd238e23ee4363de82379741ef09b6583947fcd4d8d6a4616ce1516cc8945b3", - strip_prefix = "github.com/cockroachdb/gogoproto@v1.3.3-0.20241214030254-fb9ffcbc9553", + sha256 = "bf052c9a7f9e23fb3ec7e9f3b7201cfc264c18ed6da0d662952d276dbc339003", + strip_prefix = "github.com/cockroachdb/gogoproto@v1.3.3-0.20241216150617-2358cdb156a1", urls = [ - "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gogoproto/com_github_cockroachdb_gogoproto-v1.3.3-0.20241214030254-fb9ffcbc9553.zip", + "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gogoproto/com_github_cockroachdb_gogoproto-v1.3.3-0.20241216150617-2358cdb156a1.zip", ], ) go_repository( diff --git a/build/bazelutil/distdir_files.bzl b/build/bazelutil/distdir_files.bzl index 4899e47a6278..570381d3e8ae 100644 --- a/build/bazelutil/distdir_files.bzl +++ b/build/bazelutil/distdir_files.bzl @@ -350,7 +350,7 @@ DISTDIR_FILES = { "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/datadriven/com_github_cockroachdb_datadriven-v1.0.3-0.20240530155848-7682d40af056.zip": "f4cb70fec2b2904a56bfbda6a6c8bf9ea1d568a5994ecdb825f770671119b63b", "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/errors/com_github_cockroachdb_errors-v1.11.3.zip": "d11ed59d96afef2d1f0ce56892839c62ff5c0cbca8dff0aaefeaef7eb190e73c", "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/go-test-teamcity/com_github_cockroachdb_go_test_teamcity-v0.0.0-20191211140407-cff980ad0a55.zip": "bac30148e525b79d004da84d16453ddd2d5cd20528e9187f1d7dac708335674b", - "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gogoproto/com_github_cockroachdb_gogoproto-v1.3.3-0.20241214030254-fb9ffcbc9553.zip": "dcd238e23ee4363de82379741ef09b6583947fcd4d8d6a4616ce1516cc8945b3", + "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gogoproto/com_github_cockroachdb_gogoproto-v1.3.3-0.20241216150617-2358cdb156a1.zip": "bf052c9a7f9e23fb3ec7e9f3b7201cfc264c18ed6da0d662952d276dbc339003", "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gostdlib/com_github_cockroachdb_gostdlib-v1.19.0.zip": "c4d516bcfe8c07b6fc09b8a9a07a95065b36c2855627cb3514e40c98f872b69e", "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/logtags/com_github_cockroachdb_logtags-v0.0.0-20241205023844-89a8856d99be.zip": "ff8b3d36873cfff78669000d194cbaa96520549a62408c52b538380d953b99d7", "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/metamorphic/com_github_cockroachdb_metamorphic-v0.0.0-20231108215700-4ba948b56895.zip": "28c8cf42192951b69378cf537be5a9a43f2aeb35542908cc4fe5f689505853ea", diff --git a/go.mod b/go.mod index 706078af39c2..60bb57b2adc3 100644 --- a/go.mod +++ b/go.mod @@ -483,4 +483,4 @@ replace github.com/docker/docker => github.com/moby/moby v24.0.6+incompatible replace golang.org/x/time => github.com/cockroachdb/x-time v0.3.1-0.20230525123634-71747adb5d5c -replace github.com/gogo/protobuf => github.com/cockroachdb/gogoproto v1.3.3-0.20241214030254-fb9ffcbc9553 +replace github.com/gogo/protobuf => github.com/cockroachdb/gogoproto v1.3.3-0.20241216150617-2358cdb156a1 diff --git a/go.sum b/go.sum index 5d023732195b..0135495104f2 100644 --- a/go.sum +++ b/go.sum @@ -536,8 +536,8 @@ github.com/cockroachdb/errors v1.11.3 h1:5bA+k2Y6r+oz/6Z/RFlNeVCesGARKuC6YymtcDr github.com/cockroachdb/errors v1.11.3/go.mod h1:m4UIW4CDjx+R5cybPsNrRbreomiFqt8o1h1wUVazSd8= github.com/cockroachdb/go-test-teamcity v0.0.0-20191211140407-cff980ad0a55 h1:YqzBA7tf8Gv8Oz0BbBsPenqkyjiohS7EUIwi7p1QJCU= github.com/cockroachdb/go-test-teamcity v0.0.0-20191211140407-cff980ad0a55/go.mod h1:QqVqNIiRhLqJXif5C9wbM4JydBhrAF2WDMxkv5xkyxQ= -github.com/cockroachdb/gogoproto v1.3.3-0.20241214030254-fb9ffcbc9553 h1:qVbAx/t2EU//lHyR4LCDgHoZStlYnUuBeMYejUo/ado= -github.com/cockroachdb/gogoproto v1.3.3-0.20241214030254-fb9ffcbc9553/go.mod h1:wIrkfXJOUk95vmhcdlmaou6FpqbEExZWBM7EAtKDvYg= +github.com/cockroachdb/gogoproto v1.3.3-0.20241216150617-2358cdb156a1 h1:heM4wOIqC8Q5OH6rVCo9WoYqRKp1IrJfeqt8KYo12vY= +github.com/cockroachdb/gogoproto v1.3.3-0.20241216150617-2358cdb156a1/go.mod h1:wIrkfXJOUk95vmhcdlmaou6FpqbEExZWBM7EAtKDvYg= github.com/cockroachdb/gostdlib v1.19.0 h1:cSISxkVnTlWhTkyple/T6NXzOi5659FkhxvUgZv+Eb0= github.com/cockroachdb/gostdlib v1.19.0/go.mod h1:+dqqpARXbE/gRDEhCak6dm0l14AaTymPZUKMfURjBtY= github.com/cockroachdb/logtags v0.0.0-20211118104740-dabe8e521a4f/go.mod h1:Vz9DsVWQQhf3vs21MhPMZpMGSht7O/2vFW2xusFUVOs= diff --git a/pkg/kv/kvpb/api_test.go b/pkg/kv/kvpb/api_test.go index 2f6459280b80..a010873c7256 100644 --- a/pkg/kv/kvpb/api_test.go +++ b/pkg/kv/kvpb/api_test.go @@ -435,7 +435,7 @@ func TestRequestHeaderRoundTrip(t *testing.T) { func TestBatchRequestEmptySize(t *testing.T) { ba := &BatchRequest{} - require.Equal(t, 28, ba.Size()) + require.Equal(t, 22, ba.Size()) } func TestBatchResponseEmptySize(t *testing.T) { diff --git a/pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered b/pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered index bbc8f2f31afd..b14589187b20 100644 --- a/pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered +++ b/pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered @@ -75,7 +75,7 @@ num=1 # When uncontented=false one or more active waiters marks the lock as contented. query span=a,c ---- -num locks: 1, bytes returned: 85, resume reason: RESUME_UNKNOWN, resume span: +num locks: 1, bytes returned: 79, resume reason: RESUME_UNKNOWN, resume span: locks: range_id=3 key="a" holder= durability=Unreplicated duration=0s waiters: diff --git a/pkg/kv/kvserver/concurrency/testdata/lock_table/basic b/pkg/kv/kvserver/concurrency/testdata/lock_table/basic index d518382f4e5c..24b298dab172 100644 --- a/pkg/kv/kvserver/concurrency/testdata/lock_table/basic +++ b/pkg/kv/kvserver/concurrency/testdata/lock_table/basic @@ -702,7 +702,7 @@ topklocksbywaitduration: query ---- -num locks: 3, bytes returned: 288, resume reason: RESUME_UNKNOWN, resume span: +num locks: 3, bytes returned: 282, resume reason: RESUME_UNKNOWN, resume span: locks: range_id=3 key="a" holder=00000000-0000-0000-0000-000000000003 durability=Replicated duration=2.65s waiters: @@ -867,7 +867,7 @@ topklocksbywaitduration: query ---- -num locks: 3, bytes returned: 268, resume reason: RESUME_UNKNOWN, resume span: +num locks: 3, bytes returned: 258, resume reason: RESUME_UNKNOWN, resume span: locks: range_id=3 key="b" holder= durability=Unreplicated duration=0s waiters: diff --git a/pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access b/pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access index 3b061a61173f..9e5cd8d63bde 100644 --- a/pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access +++ b/pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access @@ -223,7 +223,7 @@ num=3 query span=a,d ---- -num locks: 1, bytes returned: 77, resume reason: RESUME_UNKNOWN, resume span: +num locks: 1, bytes returned: 73, resume reason: RESUME_UNKNOWN, resume span: locks: range_id=3 key="c" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s waiters: diff --git a/pkg/kv/kvserver/concurrency/testdata/lock_table/query b/pkg/kv/kvserver/concurrency/testdata/lock_table/query index 01e80fcdf0e1..bc35cb834891 100644 --- a/pkg/kv/kvserver/concurrency/testdata/lock_table/query +++ b/pkg/kv/kvserver/concurrency/testdata/lock_table/query @@ -45,7 +45,7 @@ num locks: 0, bytes returned: 0, resume reason: RESUME_UNKNOWN, resume span: +num locks: 1, bytes returned: 39, resume reason: RESUME_UNKNOWN, resume span: locks: range_id=3 key="c" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s @@ -82,14 +82,14 @@ num=3 query span=a,f max-locks=2 uncontended ---- -num locks: 2, bytes returned: 82, resume reason: RESUME_KEY_LIMIT, resume span: {e-f} +num locks: 2, bytes returned: 78, resume reason: RESUME_KEY_LIMIT, resume span: {e-f} locks: range_id=3 key="b" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s range_id=3 key="c" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s query span=a,f max-bytes=50 uncontended ---- -num locks: 1, bytes returned: 41, resume reason: RESUME_BYTE_LIMIT, resume span: {c-f} +num locks: 1, bytes returned: 39, resume reason: RESUME_BYTE_LIMIT, resume span: {c-f} locks: range_id=3 key="b" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s @@ -97,7 +97,7 @@ num locks: 1, bytes returned: 41, resume reason: RESUME_BYTE_LIMIT, resume span: query span=a,f max-bytes=10 uncontended ---- -num locks: 1, bytes returned: 41, resume reason: RESUME_BYTE_LIMIT, resume span: {c-f} +num locks: 1, bytes returned: 39, resume reason: RESUME_BYTE_LIMIT, resume span: {c-f} locks: range_id=3 key="b" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s @@ -238,13 +238,13 @@ num=4 query span=a,f uncontended max-bytes=50 ---- -num locks: 1, bytes returned: 41, resume reason: RESUME_BYTE_LIMIT, resume span: {b-f} +num locks: 1, bytes returned: 39, resume reason: RESUME_BYTE_LIMIT, resume span: {b-f} locks: range_id=3 key="a" holder=00000000-0000-0000-0000-000000000003 durability=Unreplicated duration=0s query span=a,f uncontended max-locks=2 uncontended ---- -num locks: 1, bytes returned: 41, resume reason: RESUME_KEY_LIMIT, resume span: {b-f} +num locks: 1, bytes returned: 39, resume reason: RESUME_KEY_LIMIT, resume span: {b-f} locks: range_id=3 key="a" holder=00000000-0000-0000-0000-000000000003 durability=Unreplicated duration=0s @@ -252,7 +252,7 @@ num locks: 1, bytes returned: 41, resume reason: RESUME_KEY_LIMIT, resume span: # Bumping max locks up to 3 should return all locks on key b. query span=a,f uncontended max-locks=3 uncontended ---- -num locks: 3, bytes returned: 123, resume reason: RESUME_KEY_LIMIT, resume span: {c-f} +num locks: 3, bytes returned: 117, resume reason: RESUME_KEY_LIMIT, resume span: {c-f} locks: range_id=3 key="a" holder=00000000-0000-0000-0000-000000000003 durability=Unreplicated duration=0s range_id=3 key="b" holder=00000000-0000-0000-0000-000000000003 durability=Unreplicated duration=0s @@ -262,7 +262,7 @@ num locks: 3, bytes returned: 123, resume reason: RESUME_KEY_LIMIT, resume span: # locks, which is 2) should return all locks even though it goes over the limit. query span=b,f uncontended max-locks=1 uncontended ---- -num locks: 2, bytes returned: 82, resume reason: RESUME_KEY_LIMIT, resume span: {c-f} +num locks: 2, bytes returned: 78, resume reason: RESUME_KEY_LIMIT, resume span: {c-f} locks: range_id=3 key="b" holder=00000000-0000-0000-0000-000000000003 durability=Unreplicated duration=0s range_id=3 key="b" holder=00000000-0000-0000-0000-000000000002 durability=Unreplicated duration=0s @@ -271,7 +271,7 @@ num locks: 2, bytes returned: 82, resume reason: RESUME_KEY_LIMIT, resume span: query span=b,f uncontended max-bytes=50 ---- -num locks: 2, bytes returned: 82, resume reason: RESUME_BYTE_LIMIT, resume span: {c-f} +num locks: 2, bytes returned: 78, resume reason: RESUME_BYTE_LIMIT, resume span: {c-f} locks: range_id=3 key="b" holder=00000000-0000-0000-0000-000000000003 durability=Unreplicated duration=0s range_id=3 key="b" holder=00000000-0000-0000-0000-000000000002 durability=Unreplicated duration=0s