kgo: broadcast batch finishes in one big blast #878
Merged
Conversation
This file contains 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
Previously, we would broadcast after every individual record finished in a batch. If we produced 5000 records concurrently but only allowed 1000 in flight, we would first send a few large batches, and then once they started finishing, we would: * Finish a record * Wake up a waiting record * That record would race into producing * The producing wouldn't linger even if we wanted to, because other records are blocked * That record would be produced alone in one batch immediately * Goto start This forced unnecessary synchronization and caused things to produce slowly. By doing one broadcast at the end of a batch, we actually give more "space" for the client to buffer before waking up everything waiting. The first goroutine awoken will still produce a small batch, _but_ we will reach a point where the client is buffering larger batches. Overall, this speeds things up for a niche case and is not detrimental in any way to the non-niche case.
StarpTech
reviewed
Dec 25, 2024
| if broadcast { | ||
| p.c.Broadcast() | ||
| } | ||
| }() |
There was a problem hiding this comment.
Can we validate this through a test?
There was a problem hiding this comment.
What's to validate? This is a change from doing a broadcast many times to doing it once -- the test is that things still work (which existing tests cover).
What can't really be tested is if this improves the problem you're running into.
There was a problem hiding this comment.
Fine for me. I was able to verify the fix so I thought it can be verified as a test as well. A test could validate if producing is actually fast enough.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, we would broadcast after every individual record finished in a batch. If we produced 5000 records concurrently but only allowed 1000 in flight, we would first send a few large batches, and then once they started finishing, we would:
This forced unnecessary synchronization and caused things to produce slowly.
By doing one broadcast at the end of a batch, we actually give more "space" for the client to buffer before waking up everything waiting. The first goroutine awoken will still produce a small batch, but we will reach a point where the client is buffering larger batches.
Overall, this speeds things up for a niche case and is not detrimental in any way to the non-niche case.
Closes #876.