opt: speed up new init pattern#62767
Conversation
A new initialization pattern was introduced in cockroachdb#58386 which makes inadvertently reusing struct fields less likely. This pattern incurs a slight overhead over the previous pattern which shows up in optimizer micro-benchmarks. This commit makes a few changes to reduce the overhead, but not completely eliminate it. The overhead is on the order of tens of nanoseconds, so it should not be a huge concern. The speedup of this change on the kv-read benchmark is shown below: name old time/op new time/op delta kv-read/Parse 2.82ns ± 5% 2.60ns ± 0% -7.59% (p=0.008 n=5+5) kv-read/OptBuild 8.81µs ± 1% 7.97µs ± 4% -9.57% (p=0.008 n=5+5) kv-read/Normalize 8.87µs ± 7% 7.80µs ± 2% -12.08% (p=0.008 n=5+5) kv-read/Explore 14.9µs ± 1% 14.6µs ± 1% -1.96% (p=0.008 n=5+5) kv-read/ExecBuild 16.2µs ± 1% 16.2µs ± 1% ~ (p=0.222 n=5+5) kv-read-no-prep/Parse 14.6µs ± 2% 14.4µs ± 1% ~ (p=0.222 n=5+5) kv-read-no-prep/OptBuild 40.3µs ± 4% 38.9µs ± 1% ~ (p=0.095 n=5+5) kv-read-no-prep/Normalize 42.2µs ± 1% 40.7µs ± 1% -3.44% (p=0.008 n=5+5) kv-read-no-prep/Explore 52.0µs ± 4% 50.2µs ± 1% -3.37% (p=0.008 n=5+5) kv-read-no-prep/ExecBuild 53.7µs ± 2% 51.7µs ± 1% -3.79% (p=0.008 n=5+5) kv-read-const/Parse 2.64ns ± 2% 2.62ns ± 1% ~ (p=0.135 n=5+5) kv-read-const/OptBuild 208ns ± 2% 206ns ± 2% ~ (p=0.659 n=5+5) kv-read-const/Normalize 204ns ± 1% 201ns ± 1% -1.86% (p=0.016 n=5+5) kv-read-const/Explore 225ns ± 3% 229ns ± 6% ~ (p=0.579 n=5+5) kv-read-const/ExecBuild 829ns ± 8% 810ns ± 3% ~ (p=0.841 n=5+5) Release note: None
|
This change is |
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)
Yes! Label added. |
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
|
bors r+ |
|
Build succeeded: |
yuzefovich
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/opt/metadata.go, line 154 at r1 (raw file):
// Clear the metadata objects to release memory (this clearing pattern is // optimized by Go). schemas := md.schemas
IIUC (I didn't look at the assembly or gcflags) the optimization in this PR is bounds-checks elimination for slice accesses. If that's the case, we have been using //gcassert:bce annotations to enforce during linting that BCE occur.
|
pkg/sql/opt/metadata.go, line 154 at r1 (raw file): Previously, yuzefovich (Yahor Yuzefovich) wrote…
The issue was that we were using |
A new initialization pattern was introduced in #58386 which makes
inadvertently reusing struct fields less likely. This pattern incurs a
slight overhead over the previous pattern which shows up in optimizer
micro-benchmarks. This commit makes a few changes to reduce the
overhead, but not completely eliminate it. The overhead is on the order
of tens of nanoseconds, so it should not be a huge concern.
The speedup of this change on the kv-read benchmark is shown below:
Release note: None