hlc: improve `Timestamp.IsEmpty()` performance by erikgrinaker · Pull Request #88911 · cockroachdb/cockroach

erikgrinaker · 2022-09-28T11:57:56Z

Timestamp.IsEmpty() saw a significant performance regression with Go 1.19, in particular with empty timestamps, apparently due to the compiler's use of memeqbody for struct comparisons. On EncodeMVCCKey benchmarks, the empty timestamp case showed a ~50% regression.

This patch changes IsEmpty() to avoid struct comparison, instead comparing the member fields directly. This yields a significant performance improvement (using Go 1.19):

name                          old time/op  new time/op  delta
TimestampIsEmpty/empty-24     6.90ns ± 2%  0.79ns ± 0%  -88.56%  (p=0.000 n=10+9)
TimestampIsEmpty/walltime-24  2.97ns ± 1%  0.75ns ± 1%  -74.90%  (p=0.000 n=10+10)
TimestampIsEmpty/all-24       2.97ns ± 1%  0.75ns ± 1%  -74.77%  (p=0.000 n=10+10)

This in turn significantly improves EncodeMVCCKey performance:

name                                            old time/op  new time/op  delta
EncodeMVCCKey/key=empty/ts=empty-24             22.0ns ± 0%  15.7ns ± 5%  -28.67%  (p=0.000 n=9+10)
EncodeMVCCKey/key=empty/ts=walltime-24          21.2ns ± 0%  17.8ns ± 0%  -16.42%  (p=0.000 n=10+10)
EncodeMVCCKey/key=empty/ts=walltime+logical-24  21.8ns ± 0%  18.6ns ± 0%  -15.02%  (p=0.000 n=10+10)
EncodeMVCCKey/key=empty/ts=all-24               22.2ns ± 0%  18.8ns ± 0%  -15.07%  (p=0.000 n=9+9)
EncodeMVCCKey/key=short/ts=empty-24             22.3ns ± 0%  15.8ns ± 0%  -29.06%  (p=0.000 n=8+10)
EncodeMVCCKey/key=short/ts=walltime-24          21.2ns ± 0%  18.0ns ± 0%  -15.02%  (p=0.000 n=9+8)
EncodeMVCCKey/key=short/ts=walltime+logical-24  22.6ns ± 0%  18.8ns ± 0%  -16.53%  (p=0.000 n=9+8)
EncodeMVCCKey/key=short/ts=all-24               22.6ns ± 0%  19.1ns ± 0%  -15.32%  (p=0.000 n=10+10)
EncodeMVCCKey/key=long/ts=empty-24              65.2ns ± 1%  58.6ns ± 0%  -10.11%  (p=0.000 n=9+9)
EncodeMVCCKey/key=long/ts=walltime-24           62.6ns ± 0%  59.6ns ± 1%   -4.72%  (p=0.000 n=10+10)
EncodeMVCCKey/key=long/ts=walltime+logical-24   63.5ns ± 0%  60.2ns ± 0%   -5.16%  (p=0.000 n=10+10)
EncodeMVCCKey/key=long/ts=all-24                63.9ns ± 0%  59.6ns ± 0%   -6.81%  (p=0.000 n=9+9)

It also yields a nice improvement over 22.1 with Go 1.18:

name                                            old time/op  new time/op  delta
EncodeMVCCKey/key=empty/ts=empty-24             15.7ns ± 0%  15.7ns ± 5%     ~     (p=0.497 n=8+10)
EncodeMVCCKey/key=empty/ts=walltime-24          19.5ns ± 0%  17.8ns ± 0%   -8.99%  (p=0.000 n=10+10)
EncodeMVCCKey/key=empty/ts=walltime+logical-24  20.2ns ± 0%  18.6ns ± 0%   -8.03%  (p=0.000 n=10+10)
EncodeMVCCKey/key=empty/ts=all-24               20.4ns ± 0%  18.8ns ± 0%   -7.67%  (p=0.000 n=9+9)
EncodeMVCCKey/key=short/ts=empty-24             16.0ns ± 0%  15.8ns ± 0%   -1.47%  (p=0.000 n=10+10)
EncodeMVCCKey/key=short/ts=walltime-24          20.6ns ± 0%  18.0ns ± 0%  -12.51%  (p=0.000 n=8+8)
EncodeMVCCKey/key=short/ts=walltime+logical-24  20.9ns ± 0%  18.8ns ± 0%   -9.82%  (p=0.000 n=10+8)
EncodeMVCCKey/key=short/ts=all-24               20.9ns ± 0%  19.1ns ± 0%   -8.79%  (p=0.000 n=9+10)
EncodeMVCCKey/key=long/ts=walltime-24           60.4ns ± 0%  59.6ns ± 1%   -1.30%  (p=0.000 n=9+10)
EncodeMVCCKey/key=long/ts=walltime+logical-24   61.4ns ± 0%  60.2ns ± 0%   -1.84%  (p=0.000 n=10+10)
EncodeMVCCKey/key=long/ts=all-24                61.4ns ± 0%  59.6ns ± 0%   -2.88%  (p=0.000 n=10+9)
EncodeMVCCKey/key=long/ts=empty-24              59.1ns ± 0%  58.6ns ± 0%   -0.77%  (p=0.000 n=10+9)

Resolves #88818.

Release note: None

cockroach-teamcity · 2022-09-28T11:58:05Z

This change is

`Timestamp.IsEmpty()` saw a significant performance regression with Go 1.19, in particular with empty timestamps, apparently due to the compiler's use of `memeqbody` for struct comparisons. On `EncodeMVCCKey` benchmarks, the empty timestamp case showed a ~50% regression. This patch changes `IsEmpty()` to avoid struct comparison, instead comparing the member fields directly. This yields a significant performance improvement (using Go 1.19): ``` name old time/op new time/op delta TimestampIsEmpty/empty-24 6.90ns ± 2% 0.79ns ± 0% -88.56% (p=0.000 n=10+9) TimestampIsEmpty/walltime-24 2.97ns ± 1% 0.75ns ± 1% -74.90% (p=0.000 n=10+10) TimestampIsEmpty/all-24 2.97ns ± 1% 0.75ns ± 1% -74.77% (p=0.000 n=10+10) ``` This in turn significantly improves `EncodeMVCCKey` performance: ``` name old time/op new time/op delta EncodeMVCCKey/key=empty/ts=empty-24 22.0ns ± 0% 15.7ns ± 5% -28.67% (p=0.000 n=9+10) EncodeMVCCKey/key=empty/ts=walltime-24 21.2ns ± 0% 17.8ns ± 0% -16.42% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 21.8ns ± 0% 18.6ns ± 0% -15.02% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=all-24 22.2ns ± 0% 18.8ns ± 0% -15.07% (p=0.000 n=9+9) EncodeMVCCKey/key=short/ts=empty-24 22.3ns ± 0% 15.8ns ± 0% -29.06% (p=0.000 n=8+10) EncodeMVCCKey/key=short/ts=walltime-24 21.2ns ± 0% 18.0ns ± 0% -15.02% (p=0.000 n=9+8) EncodeMVCCKey/key=short/ts=walltime+logical-24 22.6ns ± 0% 18.8ns ± 0% -16.53% (p=0.000 n=9+8) EncodeMVCCKey/key=short/ts=all-24 22.6ns ± 0% 19.1ns ± 0% -15.32% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=empty-24 65.2ns ± 1% 58.6ns ± 0% -10.11% (p=0.000 n=9+9) EncodeMVCCKey/key=long/ts=walltime-24 62.6ns ± 0% 59.6ns ± 1% -4.72% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=walltime+logical-24 63.5ns ± 0% 60.2ns ± 0% -5.16% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=all-24 63.9ns ± 0% 59.6ns ± 0% -6.81% (p=0.000 n=9+9) ``` It also yields a nice improvement over 22.1 with Go 1.18: ``` name old time/op new time/op delta EncodeMVCCKey/key=empty/ts=empty-24 15.7ns ± 0% 15.7ns ± 5% ~ (p=0.497 n=8+10) EncodeMVCCKey/key=empty/ts=walltime-24 19.5ns ± 0% 17.8ns ± 0% -8.99% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 20.2ns ± 0% 18.6ns ± 0% -8.03% (p=0.000 n=10+10) EncodeMVCCKey/key=empty/ts=all-24 20.4ns ± 0% 18.8ns ± 0% -7.67% (p=0.000 n=9+9) EncodeMVCCKey/key=short/ts=empty-24 16.0ns ± 0% 15.8ns ± 0% -1.47% (p=0.000 n=10+10) EncodeMVCCKey/key=short/ts=walltime-24 20.6ns ± 0% 18.0ns ± 0% -12.51% (p=0.000 n=8+8) EncodeMVCCKey/key=short/ts=walltime+logical-24 20.9ns ± 0% 18.8ns ± 0% -9.82% (p=0.000 n=10+8) EncodeMVCCKey/key=short/ts=all-24 20.9ns ± 0% 19.1ns ± 0% -8.79% (p=0.000 n=9+10) EncodeMVCCKey/key=long/ts=walltime-24 60.4ns ± 0% 59.6ns ± 1% -1.30% (p=0.000 n=9+10) EncodeMVCCKey/key=long/ts=walltime+logical-24 61.4ns ± 0% 60.2ns ± 0% -1.84% (p=0.000 n=10+10) EncodeMVCCKey/key=long/ts=all-24 61.4ns ± 0% 59.6ns ± 0% -2.88% (p=0.000 n=10+9) EncodeMVCCKey/key=long/ts=empty-24 59.1ns ± 0% 58.6ns ± 0% -0.77% (p=0.000 n=10+9) ``` Release note: None

sumeerbhola

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker and @nicktrav)

pkg/util/hlc/timestamp.go line 199 at r1 (raw file):

// gcassert:inline
func (t Timestamp) IsEmpty() bool {
	return t.WallTime == 0 && t.Logical == 0 && !t.Synthetic

Should we add a reminder to ourselves with a comment in timestamp.proto, that states that the code in timestamp.go relies on the set of fields being unchanged, so remember to update if adding fields?
May be overkill given that this is an extremely rarely changed part of the code.

erikgrinaker

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @nicktrav and @sumeerbhola)

pkg/util/hlc/timestamp.go line 199 at r1 (raw file):

Previously, sumeerbhola wrote…

Should we add a reminder to ourselves with a comment in timestamp.proto, that states that the code in timestamp.go relies on the set of fields being unchanged, so remember to update if adding fields?
May be overkill given that this is an extremely rarely changed part of the code.

We'd have to adapt a bunch of other methods as well, such as Add and Compare, so I think we'd be careful to audit the small set of methods here for any changes.

erikgrinaker · 2022-09-28T13:31:23Z

bors r=sumeerbhola

craig · 2022-09-28T15:05:10Z

Build succeeded:

Bazel Essential CI (Cockroach)

Use a struct comparison in Timestamp.IsEmpty. The IsEmpty implementation was modified to perform a field-by-field comparison in cockroachdb#88911 due to a regression in Go 1.19. The regression was fixed in Go 1.20. ``` goos: linux goarch: amd64 cpu: Intel(R) Xeon(R) CPU @ 2.30GHz │ old-hlc.txt │ new-hlc.txt │ │ sec/op │ sec/op vs base │ TimestampIsEmpty/empty-24 0.9341n ± 0% 0.9350n ± 0% ~ (p=0.197 n=10) TimestampIsEmpty/walltime-24 1.370n ± 2% 1.367n ± 1% ~ (p=0.617 n=10) TimestampIsEmpty/all-24 1.362n ± 1% 1.374n ± 2% ~ (p=0.092 n=10) geomean 1.203n 1.206n +0.24% goos: linux goarch: amd64 cpu: Intel(R) Xeon(R) CPU @ 2.30GHz │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ EncodeMVCCKey/key=short/ts=empty-24 17.59n ± 0% 17.59n ± 0% ~ (p=0.908 n=10) EncodeMVCCKey/key=short/ts=walltime-24 19.93n ± 0% 19.90n ± 0% -0.10% (p=0.043 n=10) EncodeMVCCKey/key=short/ts=walltime+logical-24 20.89n ± 0% 20.91n ± 0% ~ (p=1.000 n=10) EncodeMVCCKey/key=long/ts=empty-24 68.58n ± 0% 68.44n ± 0% -0.20% (p=0.037 n=10) EncodeMVCCKey/key=long/ts=walltime-24 70.99n ± 1% 70.11n ± 0% -1.24% (p=0.005 n=10) EncodeMVCCKey/key=long/ts=walltime+logical-24 70.40n ± 0% 70.49n ± 0% +0.14% (p=0.019 n=10) EncodeMVCCKey/key=empty/ts=empty-24 17.23n ± 0% 17.24n ± 0% ~ (p=0.290 n=10) EncodeMVCCKey/key=empty/ts=walltime-24 19.53n ± 0% 19.54n ± 0% ~ (p=0.362 n=10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 20.60n ± 0% 20.61n ± 0% ~ (p=0.116 n=10) geomean 29.59n 29.55n -0.13% ``` Epic: None Informs cockroachdb#89199. Release note: None

110808: hlc: use struct comparison for Timestamp.IsEmpty r=erikgrinaker a=jbowens Use a struct comparison in Timestamp.IsEmpty. The IsEmpty implementation was modified to perform a field-by-field comparison in #88911 due to a regression in Go 1.19. The regression was fixed in Go 1.20. ``` goos: linux goarch: amd64 cpu: Intel(R) Xeon(R) CPU @ 2.30GHz │ old-hlc.txt │ new-hlc.txt │ │ sec/op │ sec/op vs base │ TimestampIsEmpty/empty-24 0.9341n ± 0% 0.9350n ± 0% ~ (p=0.197 n=10) TimestampIsEmpty/walltime-24 1.370n ± 2% 1.367n ± 1% ~ (p=0.617 n=10) TimestampIsEmpty/all-24 1.362n ± 1% 1.374n ± 2% ~ (p=0.092 n=10) geomean 1.203n 1.206n +0.24% goos: linux goarch: amd64 cpu: Intel(R) Xeon(R) CPU @ 2.30GHz │ old.txt │ new.txt │ │ sec/op │ sec/op vs base │ EncodeMVCCKey/key=short/ts=empty-24 17.59n ± 0% 17.59n ± 0% ~ (p=0.908 n=10) EncodeMVCCKey/key=short/ts=walltime-24 19.93n ± 0% 19.90n ± 0% -0.10% (p=0.043 n=10) EncodeMVCCKey/key=short/ts=walltime+logical-24 20.89n ± 0% 20.91n ± 0% ~ (p=1.000 n=10) EncodeMVCCKey/key=long/ts=empty-24 68.58n ± 0% 68.44n ± 0% -0.20% (p=0.037 n=10) EncodeMVCCKey/key=long/ts=walltime-24 70.99n ± 1% 70.11n ± 0% -1.24% (p=0.005 n=10) EncodeMVCCKey/key=long/ts=walltime+logical-24 70.40n ± 0% 70.49n ± 0% +0.14% (p=0.019 n=10) EncodeMVCCKey/key=empty/ts=empty-24 17.23n ± 0% 17.24n ± 0% ~ (p=0.290 n=10) EncodeMVCCKey/key=empty/ts=walltime-24 19.53n ± 0% 19.54n ± 0% ~ (p=0.362 n=10) EncodeMVCCKey/key=empty/ts=walltime+logical-24 20.60n ± 0% 20.61n ± 0% ~ (p=0.116 n=10) geomean 29.59n 29.55n -0.13% ``` Epic: None Informs #89199. Release note: None Co-authored-by: Jackson Owens <jackson@cockroachlabs.com>

erikgrinaker requested a review from nicktrav September 28, 2022 11:57

erikgrinaker self-assigned this Sep 28, 2022

erikgrinaker force-pushed the timestamp-isempty branch from 59ed863 to c48c289 Compare September 28, 2022 11:59

erikgrinaker force-pushed the timestamp-isempty branch from c48c289 to fecf978 Compare September 28, 2022 11:59

erikgrinaker requested review from a team September 28, 2022 12:00

erikgrinaker added the backport-22.2.x label Sep 28, 2022

sumeerbhola approved these changes Sep 28, 2022

View reviewed changes

erikgrinaker commented Sep 28, 2022

View reviewed changes

craig Bot merged commit e783f14 into cockroachdb:master Sep 28, 2022

blathers-crl Bot mentioned this pull request Sep 28, 2022

release-22.2: hlc: improve Timestamp.IsEmpty() performance #88918

Merged

nicktrav mentioned this pull request Sep 28, 2022

storage: performance regression in EncodeMVCCKey and EncodeMVCCValue #88818

Closed

This was referenced Oct 3, 2022

storage: reconsider struct comparisons with Go 1.20 #89199

Closed

storage: avoid EncodeMVCCValue struct comparison #88989

Merged

nvb mentioned this pull request Oct 18, 2022

release: 22.2 microbenchmark regression investigation and sign-off #87685

Closed

16 tasks

erikgrinaker deleted the timestamp-isempty branch October 30, 2022 13:10

jbowens mentioned this pull request Sep 18, 2023

hlc: use struct comparison for Timestamp.IsEmpty #110808

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hlc: improve `Timestamp.IsEmpty()` performance#88911

hlc: improve `Timestamp.IsEmpty()` performance#88911
craig[bot] merged 1 commit into
cockroachdb:masterfrom
erikgrinaker:timestamp-isempty

erikgrinaker commented Sep 28, 2022 •

edited

Loading

Uh oh!

cockroach-teamcity commented Sep 28, 2022

Uh oh!

sumeerbhola left a comment

Uh oh!

erikgrinaker left a comment

Uh oh!

erikgrinaker commented Sep 28, 2022

Uh oh!

craig Bot commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

erikgrinaker commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Sep 28, 2022

Uh oh!

sumeerbhola left a comment

Choose a reason for hiding this comment

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

erikgrinaker commented Sep 28, 2022

Uh oh!

craig Bot commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

erikgrinaker commented Sep 28, 2022 •

edited

Loading