Reduce scope of locks #1668
Reduce scope of locks #1668
Conversation
WalkthroughThe changes update command processing in the IOThread’s StartSync method and the locking mechanism in the WatchManager’s RegisterThread method. In IOThread, multiple independent condition checks have been streamlined into an if–else if–else if chain, ensuring that only one branch executes per command. In WatchManager, the mutex is now conditionally acquired only when a thread is in "watch" mode, optimizing the locking behavior without altering public interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant I as IOThread
C->>I: Send command
alt Command is HANDSHAKE
I->>I: Process HANDSHAKE (set ClientID & Mode)
else if Command ends with ".WATCH"
I->>I: Process WATCH command
else if Command equals "UNWATCH"
I->>I: Process UNWATCH command
end
sequenceDiagram
participant T as Thread
participant WM as WatchManager
T->>WM: Request registration
alt Thread mode is "watch"
WM->>WM: Acquire lock
WM->>WM: Register thread in watch mode
WM->>WM: Release lock
else
WM->>WM: Register thread without locking
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/server/ironhawk/iothread.go(1 hunks)internal/server/ironhawk/watch_manager.go(1 hunks)
🔇 Additional comments (2)
internal/server/ironhawk/watch_manager.go (1)
37-42: Excellent optimization of lock scope.This change significantly improves performance by only acquiring the lock when necessary (thread in "watch" mode). Since
RegisterThreadis called for every command, this optimization reduces contention on the mutex for non-watch commands.internal/server/ironhawk/iothread.go (1)
61-69: Good refactoring of conditional logic.Converting separate if statements to an if-else chain clarifies that these conditions are mutually exclusive and improves code readability. This also provides a slight performance improvement by avoiding unnecessary condition checks when the first condition is met.
| } | ||
|
|
||
| if strings.HasSuffix(c.Cmd, "UNWATCH") { | ||
| } else if strings.HasSuffix(c.Cmd, "UNWATCH") { |
There was a problem hiding this comment.
Fix the UNWATCH suffix check.
There appears to be a missing dot prefix in the UNWATCH check. This should be ".UNWATCH" to be consistent with the ".WATCH" check and ensure proper command recognition.
- } else if strings.HasSuffix(c.Cmd, "UNWATCH") {
+ } else if strings.HasSuffix(c.Cmd, ".UNWATCH") {📝 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.
| } else if strings.HasSuffix(c.Cmd, "UNWATCH") { | |
| } else if strings.HasSuffix(c.Cmd, ".UNWATCH") { |
There was a problem hiding this comment.
@lucifercr07 / @arpitbbhayani Should we do this change?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| // Use RLock instead as we are not really modifying any shared maps here. | ||
| w.mu.RLock() | ||
| defer w.mu.RUnlock() |
There was a problem hiding this comment.
This change should significantly improve the through put of notifications on watches as RLocks can be acquired concurrently. The earlier code would only allow either one writer or reader to be processed at the same time.
With this change, concurrent users can read but only one writer to be processed.
Reduce scope of locks.
RegisterThreadis called for every commands and not just watch commands. Thus acquiring a lock before theif t.Mode == "watch"is wasteful ift.Mode== "command"`, thus reduce the scope of locking.Also use if else ladder as all three conditions are mutually exclusive.
Summary by CodeRabbit