refactor: improve memory efficiency and reduce code complexity #412
Conversation
- Added sync.Mutex to all action structs to prevent race conditions - Updated Execute methods to use mutex locking and unlocking - Ensured thread-safe access to shared resources in various action types - Improved concurrency safety for log, database, audio save, and other actions
- Implemented `getNoteValueByName` function to dynamically retrieve Note field values - Uses reflection to access struct fields by name - Provides a flexible method for accessing Note struct fields dynamically
- Implemented `getActionsForItem` to support custom actions for specific species - Added `parseCommandParams` to dynamically process command parameters - Refactored `getDefaultActions` to maintain existing default action logic - Moved action processing methods from workers.go to processor.go for better organization
- Updated `processDetections` and related methods to use pointer receivers - Modified `startDetectionProcessor` to create a copy of queue items before processing - Ensured consistent pointer usage across detection processing functions - Simplified iteration and processing of pending detections
- Extracted detection filtering into a new `shouldDiscardDetection` method - Created `processApprovedDetection` to centralize approved detection handling - Simplified `pendingDetectionsFlusher` by breaking out filtering and processing logic - Improved logging and readability of detection processing workflow
WalkthroughThe pull request introduces comprehensive thread-safety enhancements to the processor module in the analysis system. The changes focus on adding mutex synchronization to various action types, modifying method receivers to support concurrent access, and refactoring the detection processing logic. The modifications include adding Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
🧹 Nitpick comments (2)
internal/analysis/processor/processor.go (2)
388-411: Flush logic and concurrency considerations.
The flusher logic is straightforward, but be sure to minimize the time you holdpendingMutex.Lock()if you expand the loop’s complexity. Consider using a read-style lock or temporary copies if it becomes a bottleneck in high-load scenarios.
470-485: Reflection-based parameter parsing.
parseCommandParamsnicely leveragesgetNoteValueByNameto populate arguments dynamically. This design is flexible, but keep in mind reflection overhead if performance becomes critical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/analysis/processor/actions.go(11 hunks)internal/analysis/processor/execute.go(1 hunks)internal/analysis/processor/processor.go(5 hunks)internal/analysis/processor/workers.go(0 hunks)
💤 Files with no reviewable changes (1)
- internal/analysis/processor/workers.go
🔇 Additional comments (13)
internal/analysis/processor/processor.go (7)
142-143: Question the need for itemCopy.
You're making a shallow copy ofitemand then passing its pointer. This is generally okay if thequeue.Resultsstructure isn't too large, but be mindful of the memory cost if it grows. If concurrency is a concern, confirm that a shallow copy is safe, as shared references withinitemmay still lead to race conditions.
150-150: Pointer parameter usage is aligned with memory efficiency goals.
Switching from value to pointer inprocessDetectionscan help avoid large copies, especially ifqueue.Resultsis sizable. Good move for memory usage.
198-198: Pointer parameter usage is consistent and beneficial.
Likewise, the change to pass*queue.ResultsinprocessResultsensures consistency and potentially lowers memory overhead. Nice improvement.
216-217: Good modularization for dog/human detection.
Separating outhandleDogDetectionandhandleHumanDetectionimproves readability and maintainability.
368-383: Dedicated method for approved detections is clear.
The newprocessApprovedDetectionmethod cleanly separates out final actions. Ensure that resource locks are in place if any shared state is updated after you retrieve data fromitem.
431-468: Refactored action retrieval is intuitive.
CentralizinggetActionsForItemclarifies how actions are selected for each detection. This structure is flexible and fosters easy expansion of custom actions.
486-537: Default actions respect separation of concerns.
getDefaultActionsis well-structured, grouping major output destinations and services. Consider making these actions configurable through the settings to further enhance modularity.internal/analysis/processor/execute.go (1)
170-170: Reflection for dynamic parameter retrieval.
getNoteValueByNameis a convenient approach for on-the-fly lookups but can be relatively expensive or brittle for large structs. Validate that this won’t trigger performance issues under heavy load.internal/analysis/processor/actions.go (5)
13-13: Sync package import aligns with concurrency improvements.
The addition ofsyncis necessary for the newly introduced mutex usage throughout the action structures and is consistent with your thread-safety objectives.
Line range hint
34-75: Use of per-action mutex ensures thread safety.
Addingmu sync.Mutexfields protects concurrent modifications within each action. This is a good step toward thread-safe operations.
79-82: Pointer receivers enhance concurrency and clarity.
Shifting from value to pointer receivers onExecutemethods ensures mutex locking modifies the correct instance. This clarifies side effects and prevents unintentional copies. The recommended lock/unlock pair is used consistently. Keep an eye on potential panics ifais ever nil in the calling code.Also applies to: 101-104, 149-152, 191-194, 230-233, 282-285
128-128: Chaining actions is a neat approach.
InvokingSaveAudioActionafter a DB save centralizes the processing flow. This design fosters a flexible pipeline approach for each detection. Good practice.
157-157: Proper octal notation.
Switching from0755to0o755is more explicit and aligns with current Go best practices.
| // shouldDiscardDetection checks if a detection should be discarded based on various criteria | ||
| func (p *Processor) shouldDiscardDetection(item *PendingDetection, minDetections int) (shouldDiscard bool, reason string) { | ||
| // Check minimum detection count | ||
| if item.Count < minDetections { | ||
| return true, fmt.Sprintf("false positive, matched %d/%d times", item.Count, minDetections) | ||
| } | ||
|
|
||
| // Check privacy filter | ||
| if p.Settings.Realtime.PrivacyFilter.Enabled { | ||
| lastHumanDetection, exists := p.LastHumanDetection[item.Source] | ||
| if exists && lastHumanDetection.After(item.FirstDetected) { | ||
| return true, "privacy filter" | ||
| } | ||
| } | ||
|
|
||
| // Check dog bark filter | ||
| if p.Settings.Realtime.DogBarkFilter.Enabled { | ||
| if p.Settings.Realtime.DogBarkFilter.Debug { | ||
| log.Printf("Last dog detection: %s\n", p.LastDogDetection) | ||
| } | ||
| if p.CheckDogBarkFilter(item.Detection.Note.CommonName, p.LastDogDetection[item.Source]) || | ||
| p.CheckDogBarkFilter(item.Detection.Note.ScientificName, p.LastDogDetection[item.Source]) { | ||
| return true, "recent dog bark" | ||
| } | ||
| } | ||
|
|
||
| return false, "" | ||
| } |
There was a problem hiding this comment.
Possible data race on shared maps.
Access to LastHumanDetection and LastDogDetection can be concurrent, but these lines read them without explicit locking. This might cause a data race if other goroutines modify them simultaneously. Consider locking (e.g., reusing pendingMutex or a dedicated mutex) whenever reading or writing these maps.
func (p *Processor) shouldDiscardDetection(item *PendingDetection, minDetections int) (bool, string) {
...
+ p.pendingMutex.Lock()
lastHumanDetection, exists := p.LastHumanDetection[item.Source]
+ p.pendingMutex.Unlock()
if exists && lastHumanDetection.After(item.FirstDetected) {
return true, "privacy filter"
}
...
}Committable suggestion skipped: line range outside the PR's diff.
Docstrings generation was requested by @tphakala. * #412 (comment) The following files were modified: * `internal/analysis/processor/execute.go` * `internal/analysis/processor/processor.go`
|
Note Generated docstrings for this pull request, at #413 |
- Introduced `detectionMutex` to protect concurrent access to LastDogDetection and LastHumanDetection maps - Added read and write locks in dog and human detection handling methods - Ensured thread-safe retrieval and update of detection timestamps - Improved concurrency safety for detection filtering and processing
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/analysis/processor/processor.go (3)
344-378: Add documentation for the return values.Good refactoring that centralizes the filtering logic. Consider adding documentation for the return values:
// shouldDiscardDetection checks if a detection should be discarded based on various criteria +// Returns: +// - shouldDiscard: true if the detection should be discarded +// - reason: human-readable explanation for why the detection was discarded func (p *Processor) shouldDiscardDetection(item *PendingDetection, minDetections int) (shouldDiscard bool, reason string) {
400-402: Add panic recovery to protect resources.Consider adding panic recovery to ensure the ticker is properly stopped even if the goroutine panics:
go func() { + defer func() { + ticker.Stop() + if r := recover(); r != nil { + log.Printf("Recovered from panic in pendingDetectionsFlusher: %v\n", r) + } + }() ticker := time.NewTicker(1 * time.Second) - defer ticker.Stop()Also applies to: 406-425
482-496: Enhance command parameter parsing robustness.The parameter parsing could be more robust:
- Add validation for unknown parameters
- Handle type conversion errors gracefully
func parseCommandParams(params []string, detection *Detections) map[string]interface{} { commandParams := make(map[string]interface{}) for _, param := range params { - value := getNoteValueByName(&detection.Note, param) + value, ok := getNoteValueByName(&detection.Note, param) + if !ok { + log.Printf("Warning: Unknown parameter %q\n", param) + continue + } // Check if the parameter is confidence and normalize it if param == "confidence" { if confidence, ok := value.(float64); ok { value = confidence * 100 + } else { + log.Printf("Warning: Expected float64 for confidence, got %T\n", value) } } commandParams[param] = value } return commandParams }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/analysis/processor/processor.go(8 hunks)
🔇 Additional comments (3)
internal/analysis/processor/processor.go (3)
44-44: LGTM! Thread-safety improvement implemented.The addition of
detectionMutexproperly addresses the data race concerns onLastDogDetectionandLastHumanDetectionmaps.
143-144: LGTM! Memory optimization implemented.Good changes:
- Using pointer parameters reduces memory overhead by avoiding large struct copies
- Creating a copy of the queue item ensures thread-safety
Also applies to: 151-151, 199-199
380-395: Validate BeginTime before assignment.Consider validating that
item.FirstDetectedis not zero before updatingBeginTimeto prevent potential issues with time-based queries or displays.- item.Detection.Note.BeginTime = item.FirstDetected + if !item.FirstDetected.IsZero() { + item.Detection.Note.BeginTime = item.FirstDetected + } else { + log.Printf("Warning: FirstDetected time is zero for %s\n", species) + item.Detection.Note.BeginTime = time.Now() + }
The pull request introduces comprehensive thread-safety enhancements to the processor module in the analysis system. The changes focus on adding mutex synchronization to various action types, modifying method receivers to support concurrent access, and refactoring the detection processing logic. The modifications include adding sync.Mutex to multiple action structs, updating method signatures to use pointer receivers, and introducing new methods for handling detection processing and action execution.