Skip to content

Commit

Permalink
Merge #137479 #137532
Browse files Browse the repository at this point in the history 
137479: protobuf: don't serialize zero-valued std duration r=nvanbenschoten a=nvanbenschoten

Picks up cockroachdb/gogoproto#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

137532: metrics: address race in MetricVec collection code path r=kyle-a-wong a=MattWhelan

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

Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Co-authored-by: Matt Whelan <matt.whelan@cockroachlabs.com>
  • Loading branch information
@nvanbenschoten @MattWhelan
3 people committed Dec 16, 2024
3 parents 3391e41 + cabfad2 + 12dc32e commit c524fdc
Show file tree
Hide file tree
Showing 10 changed files with 36 additions and 20 deletions.
6 changes: 3 additions & 3 deletions DEPS.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion build/bazelutil/distdir_files.bzl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions go.sum
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand Down
10 changes: 10 additions & 0 deletions pkg/kv/kvpb/api_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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, 22, ba.Size())
}

func TestBatchResponseEmptySize(t *testing.T) {
br := &BatchResponse{}
require.Equal(t, 6, br.Size())
}
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/concurrency/testdata/lock_table/add_discovered
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: <nil>
num locks: 1, bytes returned: 79, resume reason: RESUME_UNKNOWN, resume span: <nil>
locks:
range_id=3 key="a" holder=<nil> durability=Unreplicated duration=0s
waiters:
Expand Down
4 changes: 2 additions & 2 deletions pkg/kv/kvserver/concurrency/testdata/lock_table/basic
View file
Original file line number Diff line number Diff line change
Expand Up @@ -702,7 +702,7 @@ topklocksbywaitduration:

query
----
num locks: 3, bytes returned: 288, resume reason: RESUME_UNKNOWN, resume span: <nil>
num locks: 3, bytes returned: 282, resume reason: RESUME_UNKNOWN, resume span: <nil>
locks:
range_id=3 key="a" holder=00000000-0000-0000-0000-000000000003 durability=Replicated duration=2.65s
waiters:
Expand Down Expand Up @@ -867,7 +867,7 @@ topklocksbywaitduration:

query
----
num locks: 3, bytes returned: 268, resume reason: RESUME_UNKNOWN, resume span: <nil>
num locks: 3, bytes returned: 258, resume reason: RESUME_UNKNOWN, resume span: <nil>
locks:
range_id=3 key="b" holder=<nil> durability=Unreplicated duration=0s
waiters:
Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/concurrency/testdata/lock_table/dup_access
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ num=3

query span=a,d
----
num locks: 1, bytes returned: 77, resume reason: RESUME_UNKNOWN, resume span: <nil>
num locks: 1, bytes returned: 73, resume reason: RESUME_UNKNOWN, resume span: <nil>
locks:
range_id=3 key="c" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s
waiters:
Expand Down
18 changes: 9 additions & 9 deletions pkg/kv/kvserver/concurrency/testdata/lock_table/query
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ num locks: 0, bytes returned: 0, resume reason: RESUME_UNKNOWN, resume span: <ni

query span=a,d uncontended
----
num locks: 1, bytes returned: 41, resume reason: RESUME_UNKNOWN, resume span: <nil>
num locks: 1, bytes returned: 39, resume reason: RESUME_UNKNOWN, resume span: <nil>
locks:
range_id=3 key="c" holder=00000000-0000-0000-0000-000000000001 durability=Unreplicated duration=0s

Expand Down Expand Up @@ -82,22 +82,22 @@ 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

# ensure that we return at least one lock, even if it exceed the limits.

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

Expand Down Expand Up @@ -238,21 +238,21 @@ 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


# 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
Expand All @@ -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
Expand All @@ -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
6 changes: 6 additions & 0 deletions pkg/util/metric/metric.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1355,6 +1355,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 {
Expand Down Expand Up @@ -1453,6 +1455,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 {
Expand Down Expand Up @@ -1530,6 +1534,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 {
Expand Down

0 comments on commit c524fdc

Please sign in to comment.