Fix High CPU (#189) #194
Fix High CPU (#189) #194
Conversation
|
""" WalkthroughA generic event dispatching system was introduced, replacing the previous dependency on the external Changes
Sequence Diagram(s)sequenceDiagram
participant Publisher
participant Dispatcher
participant Subscriber
Publisher->>Dispatcher: Publish(event)
Dispatcher->>Subscriber: Deliver event (async, via queue)
Subscriber-->>Dispatcher: (Processes event)
sequenceDiagram
participant Client
participant Dispatcher
Client->>Dispatcher: Subscribe(eventType, handler)
Dispatcher-->>Client: Returns CancelFunc
Client->>Dispatcher: Emit(event)
Dispatcher->>Client: Handler invoked (async)
Client->>Dispatcher: (Optionally calls CancelFunc to unsubscribe)
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
event/event_test.go (1)
1-2: Fix typos in copyright header.
🧹 Nitpick comments (4)
event/README.md (1)
1-3: Fix bare URL formattingThe documentation clearly explains the migration rationale, but the bare URL should be properly formatted.
Apply this diff to fix the bare URL:
-The code in `event` was originally a part of https://github.com/kelindar/event (v1.5.2) +The code in `event` was originally a part of [kelindar/event](https://github.com/kelindar/event) (v1.5.2)event/event_test.go (2)
137-204: Replace time.Sleep with deterministic synchronization.Using
time.Sleepfor synchronization can lead to flaky tests. Consider using channels or WaitGroups for deterministic behavior.For example, you could track completion of all subscriptions and publications using WaitGroups instead of arbitrary delays.
206-276: Replace time.Sleep with deterministic synchronization.Similar to the previous test, avoid using
time.Sleepfor synchronization to prevent flaky tests.event/event.go (1)
231-266: Refactor duplicated queue length calculation.The maximum queue length calculation is duplicated. Consider extracting it into a helper method.
+// calculateMaxQueueLen returns the maximum queue length across all consumers +func (s *group[T]) calculateMaxQueueLen() int { + maxLen := 0 + for _, sub := range s.subs { + if len(sub.queue) > maxLen { + maxLen = len(sub.queue) + } + } + return maxLen +} + // Broadcast sends an event to all consumers func (s *group[T]) Broadcast(ev T) { s.cond.L.Lock() defer s.cond.L.Unlock() // Calculate current maximum queue length - s.maxLen = 0 - for _, sub := range s.subs { - if len(sub.queue) > s.maxLen { - s.maxLen = len(sub.queue) - } - } + s.maxLen = s.calculateMaxQueueLen() // Backpressure: wait if queues are full for s.maxLen >= s.maxQueue { s.cond.Wait() - - // Recalculate after wakeup - s.maxLen = 0 - for _, sub := range s.subs { - if len(sub.queue) > s.maxLen { - s.maxLen = len(sub.queue) - } - } + s.maxLen = s.calculateMaxQueueLen() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
event/README.md(1 hunks)event/default.go(1 hunks)event/default_test.go(1 hunks)event/event.go(1 hunks)event/event_test.go(1 hunks)llama-swap.go(1 hunks)proxy/logMonitor.go(2 hunks)proxy/process.go(1 hunks)proxy/proxymanager_api.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
proxy/logMonitor.go (1)
event/event.go (1)
NewDispatcherConfig(43-54)
event/default_test.go (3)
event/event.go (2)
NewDispatcher(38-40)Subscribe(96-99)event/event_test.go (2)
MyEvent1(308-310)TypeEvent1(304-304)event/default.go (3)
On(16-18)OnType(22-24)Emit(28-30)
event/default.go (1)
event/event.go (5)
NewDispatcherConfig(43-54)Event(17-19)Subscribe(96-99)SubscribeTo(102-152)Publish(155-161)
event/event_test.go (1)
event/event.go (4)
NewDispatcher(38-40)Subscribe(96-99)Publish(155-161)SubscribeTo(102-152)
🪛 markdownlint-cli2 (0.17.2)
event/README.md
1-1: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (8)
llama-swap.go (1)
17-17: LGTM: Clean import path migrationThe import path change from external
kelindar/eventto the internalgithub.com/mostlygeek/llama-swap/eventpackage is consistent with the PR objective to address high CPU usage by replacing the ticker-based implementation.proxy/proxymanager_api.go (1)
10-10: LGTM: Consistent import path migrationThe import path update aligns with the migration to the internal event package and maintains API compatibility.
proxy/process.go (1)
17-17: LGTM: Import path migration completedThe import path change maintains consistency across the codebase and preserves the existing event emission functionality.
proxy/logMonitor.go (2)
11-11: LGTM: Import path migration completedThe import path change aligns with the codebase-wide migration to the internal event package.
43-43: Verified NewDispatcherConfig Usage
- The function
func NewDispatcherConfig(maxQueue int) *Dispatcheris defined inevent/event.goand returns a valid*Dispatcher.- Passing
1000matches the expectedintparameter and is consistent with other defaults in the codebase (e.g., 25 000 and 50 000).- No further changes are needed.
event/default_test.go (1)
31-54: LGTM!The test correctly validates the default dispatcher's event publishing mechanism with proper synchronization and atomic operations.
event/default.go (1)
10-30: LGTM!The convenience functions provide a clean API for the default dispatcher with proper delegation to the core functions.
event/event.go (1)
29-54: LGTM!Excellent design using atomic pointers and copy-on-write for lock-free reads with synchronized writes. The configurable queue size provides good flexibility.
| // Copyright (c) Roman Atachiants and contributore. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for detaile. |
There was a problem hiding this comment.
Fix typos in copyright header.
The copyright header contains typos that should be corrected.
-// Copyright (c) Roman Atachiants and contributore. All rights reserved.
-// Licensed under the MIT license. See LICENSE file in the project root for detaile.
+// Copyright (c) Roman Atachiants and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for details.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Copyright (c) Roman Atachiants and contributore. All rights reserved. | |
| // Licensed under the MIT license. See LICENSE file in the project root for detaile. | |
| // Copyright (c) Roman Atachiants and contributors. All rights reserved. | |
| // Licensed under the MIT license. See LICENSE file in the project root for details. |
🤖 Prompt for AI Agents
In event/default_test.go at lines 1 to 2, fix the typos in the copyright header
by correcting "contributore" to "contributors" and "detaile" to "details" to
ensure proper spelling and professionalism.
| // Copyright (c) Roman Atachiants and contributore. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for detaile. |
There was a problem hiding this comment.
Fix typos in copyright header.
Same typos as in the test file.
-// Copyright (c) Roman Atachiants and contributore. All rights reserved.
-// Licensed under the MIT license. See LICENSE file in the project root for detaile.
+// Copyright (c) Roman Atachiants and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for details.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Copyright (c) Roman Atachiants and contributore. All rights reserved. | |
| // Licensed under the MIT license. See LICENSE file in the project root for detaile. | |
| // Copyright (c) Roman Atachiants and contributors. All rights reserved. | |
| // Licensed under the MIT license. See LICENSE file in the project root for details. |
🤖 Prompt for AI Agents
In event/default.go at lines 1 to 2, fix the typos in the copyright header by
correcting "contributore" to "contributors" and "detaile" to "details" to match
the correct spelling used in the test file.
Fix high CPU issue in #189
The root cause was caused by the event library being imported. This PR:
time.Ticker(it was written by Deepseek R1)Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests
Chores