timeutil: stack-allocate Timer, remove pooling#119901
Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom Mar 6, 2024
Merged
timeutil: stack-allocate Timer, remove pooling#119901craig[bot] merged 3 commits intocockroachdb:masterfrom
craig[bot] merged 3 commits intocockroachdb:masterfrom
Conversation
Member
|
This change is |
cebe308 to
48341c7
Compare
mgartner
approved these changes
Mar 5, 2024
Contributor
mgartner
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, 47 of 47 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @herkolategan, @msbutler, @renatolabs, @xinhaoz, and @yuzefovich)
yuzefovich
approved these changes
Mar 5, 2024
Member
yuzefovich
left a comment
There was a problem hiding this comment.
Nice, thanks!
Reviewed 1 of 1 files at r1, 47 of 47 files at r2, 2 of 2 files at r3, all commit messages.
Reviewable status:
e2f61b6 to
7e82b7f
Compare
This commit adds a new benchmark for timeutil.Timer creation and use. ``` name time/op Timer-10 153µs ± 1% name alloc/op Timer-10 200B ± 0% name allocs/op Timer-10 3.00 ± 0% ``` Epic: None Release note: None
Informs cockroachdb#119593. Closes cockroachdb#38055. This commit removes the pooling of timeutil.Timer structs and, in doing so, permits the structs to be stack allocated so that no pooling is necessary. This superfluous (and in hindsight, harmful) memory pooling was introduced in f11ec1c, which also added very necessary pooling for the internal time.Timer structs. The pooling was harmful because it mandated a contract where Timer structs could not be used after their Stop method was called. This was surprising (time.Timer has no such limitation) and led to subtle use-after-free bugs over time (cockroachdb#61373 and cockroachdb#119595). It was also unnecessary because the outer Timer structs can be stack allocated. Ironically, the only thing that causes them to escape to the heap was the pooling mechanism itself. Removing pooling solves the issue. ``` name old time/op new time/op delta Timer-10 153µs ± 1% 152µs ± 1% ~ (p=0.589 n=10+9) name old alloc/op new alloc/op delta Timer-10 200B ± 0% 200B ± 0% ~ (all equal) name old allocs/op new allocs/op delta Timer-10 3.00 ± 0% 3.00 ± 0% ~ (all equal) ``` Epic: None Release note: None
This commit eliminates a case where a Timer's inner timer is not recycled. This was originally identified by @andreimatei in cockroachdb#13466 (review). ``` name old time/op new time/op delta Timer-10 152µs ± 1% 153µs ± 1% ~ (p=0.133 n=9+10) name old alloc/op new alloc/op delta Timer-10 200B ± 0% 0B -100.00% (p=0.000 n=10+10) name old allocs/op new allocs/op delta Timer-10 3.00 ± 0% 0.00 -100.00% (p=0.000 n=10+10) ``` Epic: None Release note: None
7e82b7f to
46a36e3
Compare
Contributor
Author
|
TFTRs! bors r+ |
Contributor
|
This PR was included in a batch that timed out, it will be automatically retried |
Contributor
|
Build succeeded: |
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
5 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.
Informs #119593.
Closes #38055.
This PR removes the pooling of
timeutil.Timerstructs and, in doing so, permits the structs to be stack allocated so that no pooling is necessary. This superfluous (and in hindsight, harmful) memory pooling was introduced in f11ec1c, which also added very necessary pooling for the internal time.Timer structs.The pooling was harmful because it mandated a contract where Timer structs could not be used after their Stop method was called. This was surprising (time.Timer has no such limitation) and led to subtle use-after-free bugs over time (#61373 and #119595). It was also unnecessary because the outer Timer structs can be stack allocated. Ironically, the only thing that causes them to escape to the heap was the pooling mechanism itself. Removing pooling solves the issue.
The PR then improves the memory pooling of the inner
time.Timerso that it is always recycled. This was originally identified by @andreimatei in #13466 (review).Doing so has a positive impact on the microbenchmark introduced in the first commit, demonstrating that timers can be stack-allocated and require zero heap allocations:
cc. @andreimatei @ajwerner
Epic: None
Release note: None