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.

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

perf: Replace channel check with atomic bool in tailer.send() #12976

Merged
merged 3 commits into from
May 15, 2024

Conversation

benclive
Copy link
Contributor

What this PR does / why we need it:

  • @cyriltovena discovered that tailer.isClosed was eating a significant amount of CPU in ops (>20 CPUs worth)
    image]
  • I replaced the select on the channel to try and reduce the amount of CPU time spent on this particular code path.
  • I used an atomic bool for the send() but retained the closeChan to signal the other parts of the tailer to shut down ASAP, otherwise we'd need to check the bool periodically using (for example) time.After.

Benchmark before thte change, using select-default:

Benchmark_tailer_isClosed
Benchmark_tailer_isClosed-14    	536670529	         2.220 ns/op
PASS

Benchmark after the change, using atomic bool:

Benchmark_tailer_isClosed
Benchmark_tailer_isClosed-14    	1000000000	         0.2886 ns/op
PASS

So this small change is nearly 10x faster and therefore would save ~18-20 cores worth of CPU time in our ops cluster.

@benclive benclive requested a review from a team as a code owner May 15, 2024 15:55
@benclive benclive changed the title Replace channel check with atomic bool tailer.send(): Replace channel check with atomic bool May 15, 2024
@benclive benclive requested a review from cyriltovena May 15, 2024 15:56
@benclive benclive changed the title tailer.send(): Replace channel check with atomic bool perf: Replace channel check with atomic bool in tailer.send() May 15, 2024
Copy link
Contributor

@MasslessParticle MasslessParticle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to remove closeChan altogether? I see it's used in a few other select statements that we can't get rid of, but it would keep us from signaling closed in two different ways in the same file.

Nevermind. On closer look, those other usages rely on the close chan to interrupt selects blocking

Copy link
Contributor

@cyriltovena cyriltovena left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM !

For next time, in your PR you can use benchstat it gives a much better output.

@cyriltovena cyriltovena merged commit 4a5edf1 into main May 15, 2024
58 of 59 checks passed
@cyriltovena cyriltovena deleted the replace-channel-check-with-atomic-bool branch May 15, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants