fix: Flush once per replayed batch instead of once per event#64
Draft
keelerm84 wants to merge 1 commit into
Draft
fix: Flush once per replayed batch instead of once per event#64keelerm84 wants to merge 1 commit into
keelerm84 wants to merge 1 commit into
Conversation
The SSE handler previously called flusher.Flush() after every replayed event, emitting one HTTP chunk (and typically one write syscall) per event. A large replay therefore cost as many network writes as it had events. Replayed batch events are now encoded one per main select-loop iteration and flushed only when the batch channel closes. net/http's chunked writer flushes its buffer through to the connection as it fills, so the batch streams out as a few large chunks rather than one chunk per event -- far fewer write syscalls for an identical payload, which cuts replay CPU and wall-clock substantially on both buffered and streaming repositories. Memory stays bounded because the response buffer writes through as it fills and a slow client blocks the encode. Encoding through the select loop keeps the handler responsive: connection close and MaxConnTime are evaluated between every event, so no cap on events-per-flush is needed to bound time away from the loop. Includes a replay benchmark and a regression test that floods an unbounded batch and asserts MaxConnTime is still honored mid-replay.
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
1 participant
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.
Summary
The SSE server handler previously called
flusher.Flush()after every replayed event. On a chunked HTTP response each flush emits its own HTTP chunk (and typically its own write syscall), so replaying a large data set to a newly connected client cost roughly one network write per event.This change encodes replayed batch events one per iteration of the handler's main
selectloop and flushes only when the batch channel closes.net/http's chunked writer already flushes its buffer through to the connection as it fills, so the batch now streams out as a handful of large chunks instead of one chunk per event -- the same payload with far fewer write syscalls. Memory stays bounded: the response buffer writes through as it fills, and a slow client blocks the encode.Because every event still passes through the
selectloop, connection close andMaxConnTimeare evaluated between events. That removes the need for any cap on how many events are encoded between flushes (there is no longer a window where the handler is "away" from the loop), so the previousmaxEventsPerFlushbound is gone.Benchmarking a replay over loopback HTTP (counting real
Flushcalls) shows flushes drop from N+1 to a small constant for an N-event replay, cutting replay CPU and wall-clock substantially on both buffered and streaming repositories, with identical payload bytes and encoder writes.A regression test floods an unbounded replay batch and asserts the handler still honors
MaxConnTimemid-replay.