-
Notifications
You must be signed in to change notification settings - Fork 802
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.
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
Get rid of time.After in for loops #6303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got curious and dug around in how timers are created and scheduled, to see what the initial-0 would do, because I hadn't seen that pattern before.
Looks like it puts it into the timer heap, sorts it, and it fires ~immediately, so it's safe both before and after 1.23, though slightly wasteful (but it's a one-time tiny cost so it seems fine to ignore it).
1.23 is substantially more complex, but seems to be essentially the same. And probably has some optimizations for immediate-firing, given how the blog post talks about it more-reliably following both branches with very small delays. So safe and no worse + probably better there too 👍
Since this very nearly gets rid of ALL of our time.After
use, yea, I think the minor added complexity is fine. It's a better habit for us to be in, since we have a significant amount of fast + perpetual loops, and we can almost just ban After
now to prevent a costly leak from sneaking back in.
Check the ticker instances, since they seem kinda straightforward to me? Otherwise LGTM and seems fine either way.
What changed?
Replace time.After in for loops with NewTimer + Reset or Sleep
Why?
before go 1.23, time.After in for loops can use a lot of memory if the timeout is too long and don't expire.
after go 1.23, time.After does allocation a lot in for loops.
NewTimer + Reset gets rid of this problem.
How did you test it?
existing unit tests
Potential risks
Release notes
Documentation Changes