opt: use safer struct initialization pattern#58386
Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom Jan 4, 2021
Merged
opt: use safer struct initialization pattern#58386craig[bot] merged 1 commit intocockroachdb:masterfrom
craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
Member
|
This change is |
We commonly use a pattern of declaring structs and then initializing them via an `Init` method. This allows internal data structures to be reused, reducing allocations. However, the common initialization implementation is dangerous because fields are implicitly reused. This can lead to inconspicuous bugs (see cockroachdb#58306). This commit introduces a new pattern for initialization functions that requires reused fields to be explicitly listed. This should reduce the risk of these types of bugs. Release note: None
385e9bc to
be5d3d5
Compare
rytaft
approved these changes
Jan 4, 2021
Collaborator
rytaft
left a comment
There was a problem hiding this comment.
Reviewed 34 of 34 files at r1.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @andy-kimball and @RaduBerinde)
Contributor
Author
|
TFTR! bors r=rytaft |
Contributor
|
Build succeeded: |
Member
|
💯 |
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 29, 2021
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
craig bot
pushed a commit
that referenced
this pull request
Mar 30, 2021
62767: opt: speed up new init pattern r=mgartner a=mgartner 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: 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 Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>
35 tasks
mgartner
added a commit
to mgartner/cockroach
that referenced
this pull request
Mar 30, 2021
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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We commonly use a pattern of declaring structs and then initializing
them via an
Initmethod. This allows internal data structures to bereused, reducing allocations. However, the common initialization
implementation is dangerous because fields are implicitly reused. This
can lead to inconspicuous bugs (see #58306).
This commit introduces a new pattern for initialization functions that
requires reused fields to be explicitly listed. This should reduce the
risk of these types of bugs.
Release note: None