perf(broadcast): optimize Broadcaster.broadcastWeight by caching client list#39
perf(broadcast): optimize Broadcaster.broadcastWeight by caching client list#39
Conversation
- Added `clientList` field to `Broadcaster` struct to cache the slice of connected clients. - Updated `AddClient`, `RemoveClient`, and `removeAndCloseClient` to rebuild `clientList` when the `clients` map changes. - Modified `broadcastWeight` to use the cached `clientList` instead of allocating a new slice and iterating the map on every call. - Added `internal/server/broadcaster_test.go` with `BenchmarkBroadcastWeight` and `TestBroadcasterLogic` to verify performance and correctness. - Measured ~6.3% improvement in dispatch time for 1000 clients (6.45ms -> 6.04ms). Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
👋 Thanks for opening this PR, @adcondev! Here's what will happen next:
Please make sure:
|
⚡ Benchmark Results📊 Current Branch Results |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR optimizes the WebSocket weight fan-out path by introducing a copy-on-write cached client slice in the Broadcaster, avoiding per-broadcast map iteration and slice allocation in broadcastWeight.
Changes:
- Cache connected clients in
Broadcaster.clientList, rebuilding the slice only on add/remove. - Update
broadcastWeightto read the cached slice underRLockinstead of rebuilding each call. - Add a unit test to validate cache/map synchronization and a benchmark to measure broadcast performance.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/server/broadcaster.go | Adds clientList cache and rebuild logic on client add/remove to reduce hot-path overhead in broadcastWeight. |
| internal/server/broadcaster_test.go | Adds TestBroadcasterLogic for cache correctness and BenchmarkBroadcastWeight for performance measurement. |
- Added `clientList` field to `Broadcaster` struct to cache the slice of connected clients. - Updated `AddClient`, `RemoveClient`, and `removeAndCloseClient` to rebuild `clientList` when the `clients` map changes. - Modified `broadcastWeight` to use the cached `clientList` instead of allocating a new slice and iterating the map on every call. - Added `internal/server/broadcaster_test.go` with `BenchmarkBroadcastWeight` and `TestBroadcasterLogic` to verify performance and correctness. - Fixed linting errors in new test file (`bodyclose`, `errcheck`). - Measured ~6.3% improvement in dispatch time for 1000 clients (6.45ms -> 6.04ms). Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
- Added `clientList` field to `Broadcaster` struct to cache the slice of connected clients. - Updated `AddClient`, `RemoveClient`, and `removeAndCloseClient` to rebuild `clientList` when the `clients` map changes. - Modified `broadcastWeight` to use the cached `clientList` instead of allocating a new slice and iterating the map on every call. - Added `internal/server/broadcaster_test.go` with `BenchmarkBroadcastWeight` and `TestBroadcasterLogic` to verify performance and correctness. - Fixed linting errors in new test file (`bodyclose`, `errcheck`) related to `websocket.Dial` response handling. - Measured ~6.3% improvement in dispatch time for 1000 clients (6.45ms -> 6.04ms). Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Adrián Constante <ad_con.reload@proton.me>
- Added `clientList` field to `Broadcaster` struct to cache the slice of connected clients. - Updated `AddClient`, `RemoveClient`, and `removeAndCloseClient` to rebuild `clientList` when the `clients` map changes. - Modified `broadcastWeight` to use the cached `clientList` instead of allocating a new slice and iterating the map on every call. - Added `internal/server/broadcaster_test.go` with `BenchmarkBroadcastWeight` and `TestBroadcasterLogic` to verify performance and correctness. - Fixed linting errors in new test file: - Addressed `bodyclose` and `errcheck` by closing response bodies from `websocket.Dial`. - Addressed `errcheck` by explicitly ignoring (`_ =`) error returns from `c.Close()` calls. - Measured ~6.3% improvement in dispatch time for 1000 clients (6.45ms -> 6.04ms). Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
adcondev
changed the title
|
not for now |
adcondev
deleted the
perf/broadcaster-client-list-cache-16602011425520877998
branch
This PR optimizes the
broadcastWeightmethod ininternal/server/broadcaster.goby introducing a Copy-On-Write (COW) caching mechanism for the list of connected WebSocket clients.Previously,
broadcastWeightwould allocate a new slice and iterate over theclientsmap (O(N)) on every weight reading broadcast. This change maintains a cachedclientListslice that is only rebuilt when clients connect or disconnect (which are infrequent operations compared to broadcasting).The optimization reduces the per-broadcast overhead by eliminating the slice allocation and map iteration, replacing it with a simple pointer read (O(1)).
Performance Impact:
A benchmark with 1000 connected clients showed a ~6.3% improvement in dispatch time (from ~6.45ms/op to ~6.04ms/op). The primary gain is the removal of allocation pressure from the hot path.
Verification:
TestBroadcasterLogicto ensure the cached list stays in sync with the map during add/remove operations.BenchmarkBroadcastWeightto measure the performance impact.PR created automatically by Jules for task 16602011425520877998 started by @adcondev