Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

storage: reconsider struct comparisons with Go 1.20 #89199

Closed
erikgrinaker opened this issue Oct 3, 2022 · 2 comments · Fixed by #111375
Closed

storage: reconsider struct comparisons with Go 1.20 #89199

erikgrinaker opened this issue Oct 3, 2022 · 2 comments · Fixed by #111375
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-storage Storage Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Oct 3, 2022 •
edited by cockroach-jira-scripts
Loading

As described in #88818 (comment), Go 1.19 has a significant performance regression when comparing structs. Because of this, we had to introduce manual struct comparisons (explicitly listing each field) and give up certain inlining opportunities to recover the performance.

This regression will been fixed in Go 1.20, but won't be backported to 1.19. We should consider reverting back to using struct comparisons once we build with Go 1.20. Relevant changes:

Jira issue: CRDB-20169
@erikgrinaker erikgrinaker added C-performance Perf of queries or internals. Solution not expected to change functional behavior. A-storage Relating to our storage engine (Pebble) on-disk storage. T-storage Storage Team labels Oct 3, 2022
@erikgrinaker erikgrinaker changed the title storage: reconsider struct comparisons storage: reconsider struct comparisons with Go 1.20 Oct 3, 2022
@erikgrinaker erikgrinaker mentioned this issue Oct 3, 2022
Merged
@ajwerner ajwerner mentioned this issue Feb 2, 2023
Closed
jbowens added a commit to jbowens/cockroach that referenced this issue Sep 18, 2023
@jbowens
hlc: use struct comparison for Timestamp.IsEmpty
c21f763 
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
@jbowens jbowens mentioned this issue Sep 18, 2023
Merged
craig bot pushed a commit that referenced this issue Sep 18, 2023
@jbowens
Merge #110808
9371ad8 
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 <[email protected]>
@nicktrav
Copy link
Collaborator

@jbowens - is this all done now that we have #110808 landed?

@jbowens jbowens mentioned this issue Sep 27, 2023
Merged
@jbowens
Copy link
Collaborator

jbowens commented Sep 27, 2023

@jbowens - is this all done now that we have #110808 landed?

Need #111375 too

craig bot pushed a commit that referenced this issue Sep 27, 2023
@yuzefovich @jbowens
Merge #111321 #111375 #111377
fcd2f09 
111321: sql: remove references to closed multi-tenancy issues r=yuzefovich a=yuzefovich

This commit updates a few places to remove now-closed multi-tenancy
issues. Two of those places are gated behind the system tenant, so the
issue doesn't matter, and they now use existing issue (even though it
doesn't really relate to the features in question). In one place we
choose to return "tenant cluster setting" not enabled error to make it
more clear (I was just bitten by this because we returned opaque
"unimplemented" error). In two other places in comments we update the
references to existing issues tracking the corresponding work.

Informs: #75569.
Epic: None

Release note: None

111375: enginepb: use struct equality comparison in MVCCValueHeader.IsEmpty r=RaduBerinde a=jbowens

Now that we're on Go 1.20, the regression is gone.

```
goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                                 │   old.txt   │              new.txt               │
                                                                 │   sec/op    │   sec/op     vs base               │
EncodeMVCCValue/header=empty/value=tombstone-24                    4.797n ± 0%   4.809n ± 0%  +0.26% (p=0.000 n=10)
EncodeMVCCValue/header=empty/value=short-24                        4.796n ± 0%   4.809n ± 0%  +0.27% (p=0.000 n=10)
EncodeMVCCValue/header=empty/value=long-24                         4.797n ± 0%   4.809n ± 0%  +0.26% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime/value=tombstone-24           54.16n ± 1%   53.56n ± 0%  -1.10% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime/value=short-24               56.71n ± 1%   56.02n ± 1%  -1.22% (p=0.001 n=10)
EncodeMVCCValue/header=local_walltime/value=long-24                1.371µ ± 3%   1.314µ ± 1%  -4.16% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=long-24        1.346µ ± 2%   1.323µ ± 4%       ~ (p=0.160 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=tombstone-24   57.56n ± 2%   57.09n ± 1%  -0.83% (p=0.029 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=short-24       60.61n ± 1%   60.05n ± 1%  -0.92% (p=0.001 n=10)
geomean                                                            50.62n        50.10n       -1.02%
```

Epic: none
Resolves #89199.
Release note: None

111377: go.mod: bump Pebble to 725ebe297867 r=itsbilal a=jbowens

```
725ebe29 db: fix excise test
d925b88b db: add SkipPoint iterator option
b5677d86 db: propagate Comparer into levelIter
3a1391d6 metamorphic: Ignore ErrCancelledCompaction in background errors
da739ee7 db: adds tests for virtual sstable async table stats calculation
02b87adf db: improvements to data_test.go
5500da0a .*: support async stats for virtual sstables
17c625cf db: Fix FormatMajorVersion check in version_set for VSSTs
4e353e51 metamorphic: fix block property collectors integration
75888967 Revert "db: add SkipPoint iterator option"
4df2ce80 metamorphic: Add testing for ingest splits
```

Epic: none
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>
@craig craig bot closed this as completed in 969f3cc Sep 27, 2023
THardy98 pushed a commit to THardy98/cockroach that referenced this issue Oct 6, 2023
@jbowens
storage/enginepb: use struct equality comparison in MVCCValueHeader.I…
6429198 
…sEmpty

Now that we're on Go 1.20, the regression is gone.

```
goos: linux
goarch: amd64
cpu: Intel(R) Xeon(R) CPU @ 2.80GHz
                                                                 │   old.txt   │              new.txt               │
                                                                 │   sec/op    │   sec/op     vs base               │
EncodeMVCCValue/header=empty/value=tombstone-24                    4.797n ± 0%   4.809n ± 0%  +0.26% (p=0.000 n=10)
EncodeMVCCValue/header=empty/value=short-24                        4.796n ± 0%   4.809n ± 0%  +0.27% (p=0.000 n=10)
EncodeMVCCValue/header=empty/value=long-24                         4.797n ± 0%   4.809n ± 0%  +0.26% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime/value=tombstone-24           54.16n ± 1%   53.56n ± 0%  -1.10% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime/value=short-24               56.71n ± 1%   56.02n ± 1%  -1.22% (p=0.001 n=10)
EncodeMVCCValue/header=local_walltime/value=long-24                1.371µ ± 3%   1.314µ ± 1%  -4.16% (p=0.000 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=long-24        1.346µ ± 2%   1.323µ ± 4%       ~ (p=0.160 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=tombstone-24   57.56n ± 2%   57.09n ± 1%  -0.83% (p=0.029 n=10)
EncodeMVCCValue/header=local_walltime+logical/value=short-24       60.61n ± 1%   60.05n ± 1%  -0.92% (p=0.001 n=10)
geomean                                                            50.62n        50.10n       -1.02%
```

Epic: none
Resolves cockroachdb#89199.
Release note: None
@jbowens jbowens moved this to Done in [Deprecated] Storage Jun 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-storage Relating to our storage engine (Pebble) on-disk storage. C-performance Perf of queries or internals. Solution not expected to change functional behavior. T-storage Storage Team
Projects
No open projects
[Deprecated] Storage
Archived in project
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

enginepb: use struct equality comparison in MVCCValueHeader.IsEmpty jbowens/cockroach
3 participants
@erikgrinaker @nicktrav @jbowens