Files

T

2026-01-20 21:33:16 -03:00

9.3 KiB

Raw Blame History

Manager Mutex Usage Review

Date: 2025-10-24 Status: COMPREHENSIVE REVIEW - 23 MANAGERS IDENTIFIED Context: Follow-up to runtime bottleneck fix

Executive Summary

After fixing the critical ManagerRegistry global lock and GatheringManager per-instance locks, a comprehensive scan identified 23 additional managers with mutex usage that require review.

Review Status

✅ CLEAN (No Mutexes):

QuestManager
TradeManager
GroupCoordinator

❌ NEEDS REVIEW (23 Managers with Mutexes): Found 23 managers with std::lock_guard, std::recursive_mutex, or std::mutex usage that need analysis for unnecessary per-instance locks.

Managers Requiring Review

Category 1: AI/Combat Managers (11 found)

ResourceManager - src/modules/Playerbot/AI/ClassAI/ResourceManager.cpp
CooldownManager - src/modules/Playerbot/AI/ClassAI/CooldownManager.cpp
FormationManager - src/modules/Playerbot/AI/Combat/FormationManager.cpp
KitingManager - src/modules/Playerbot/AI/Combat/KitingManager.cpp
InterruptManager - src/modules/Playerbot/AI/Combat/InterruptManager.cpp
ObstacleAvoidanceManager - src/modules/Playerbot/AI/Combat/ObstacleAvoidanceManager.cpp
PositionManager - src/modules/Playerbot/AI/Combat/PositionManager.cpp
BotThreatManager - src/modules/Playerbot/AI/Combat/BotThreatManager.cpp
LineOfSightManager - src/modules/Playerbot/AI/Combat/LineOfSightManager.cpp
PathfindingManager - src/modules/Playerbot/AI/Combat/PathfindingManager.cpp
InteractionManager - src/modules/Playerbot/Interaction/Core/InteractionManager.cpp

Priority: MEDIUM - These are per-bot combat/AI state, likely unnecessary locks

Category 2: Economy/Profession Managers (2 found)

AuctionManager - src/modules/Playerbot/Economy/AuctionManager.cpp ⚠️ ALREADY DISABLED
ProfessionManager - src/modules/Playerbot/Professions/ProfessionManager.cpp

Priority: LOW - AuctionManager already disabled, ProfessionManager needs review

Category 3: Session/Lifecycle Managers (4 found)

DeathRecoveryManager - src/modules/Playerbot/Lifecycle/DeathRecoveryManager.cpp
BotSessionManager - src/modules/Playerbot/Session/BotSessionManager.cpp ⚠️ MAY NEED LOCKS
BotPriorityManager - src/modules/Playerbot/Session/BotPriorityManager.cpp
LFGBotManager - src/modules/Playerbot/LFG/LFGBotManager.cpp

Priority: HIGH - SessionManager may have legitimate cross-bot access needing locks

Category 4: Equipment/Inventory Managers (3 found)

InventoryManager - src/modules/Playerbot/Game/InventoryManager.cpp
EquipmentManager - src/modules/Playerbot/Equipment/EquipmentManager.cpp
BotLevelManager - src/modules/Playerbot/Character/BotLevelManager.cpp

Priority: MEDIUM - Per-bot inventory/equipment, likely unnecessary locks

Category 5: Companion/Mount Managers (2 found)

BattlePetManager - src/modules/Playerbot/Companion/BattlePetManager.cpp
MountManager - src/modules/Playerbot/Companion/MountManager.cpp

Priority: LOW - Per-bot companion state, likely unnecessary locks

Category 6: Performance/Memory Managers (1 found)

BotMemoryManager - src/modules/Playerbot/Performance/BotMemoryManager.cpp ⚠️ MAY NEED LOCKS

Priority: HIGH - Memory management may legitimately require synchronization

Review Methodology

For each manager, determine:

1. Instance Model

Question: Is there one manager instance per bot, or is it shared?

Check:

// BotAI.cpp or ManagerRegistry
_resourceManager = std::make_unique<ResourceManager>(this);  // ← Per-bot = locks likely unnecessary

// Singleton or shared
static ResourceManager* GetInstance();  // ← Shared = locks may be necessary

2. Data Access Pattern

Question: Does the manager access only its own bot's data, or does it read/modify other bots' data?

Per-Bot Data (Locks Unnecessary):

Own cooldowns (ResourceManager, CooldownManager)
Own position (PositionManager, FormationManager)
Own inventory (InventoryManager, EquipmentManager)
Own combat state (InterruptManager, KitingManager)

Shared Data (Locks May Be Necessary):

Global formation coordination (if FormationManager is shared)
Cross-bot session management (BotSessionManager)
Memory allocator pools (BotMemoryManager)

3. Atomic Alternative

Question: Could atomics replace the mutex?

Example:

// BEFORE: Mutex for simple flag
std::lock_guard<std::mutex> lock(_mutex);
_isActive = true;

// AFTER: Atomic flag
std::atomic<bool> _isActive;
_isActive.store(true, std::memory_order_release);

Priority Review Order

Phase 1: High Priority (Week 1)

BotSessionManager - May have legitimate cross-bot access
BotMemoryManager - Memory management synchronization
FormationManager - May coordinate multiple bots
LFGBotManager - May manage cross-bot LFG queue

Phase 2: Medium Priority (Week 2)

5-10. AI/Combat Managers - Likely all per-bot instance locks 11-13. Equipment/Inventory Managers - Per-bot item management

Phase 3: Low Priority (Week 3)

14-15. Companion/Mount Managers - Per-bot companion state 16. ProfessionManager - Per-bot profession state

Expected Findings

Based on GatheringManager pattern, we expect:

Unnecessary Locks (Estimated 18-20 managers):

Per-bot instance data protection
No cross-bot access
Defensive programming "just in case"

Necessary Locks (Estimated 2-5 managers):

BotSessionManager (cross-bot session coordination)
BotMemoryManager (shared memory pools)
Possibly FormationManager (group formation)
Possibly LFGBotManager (queue management)

Sample Review Process

Example: ResourceManager

Step 1: Check Instance Model

grep -n "ResourceManager" src/modules/Playerbot/AI/BotAI.cpp
# Look for: std::make_unique<ResourceManager> (per-bot)
# vs: ResourceManager::GetInstance() (shared)

Step 2: Find Lock Locations

grep -n "std::lock_guard\|std::recursive_mutex" \
  src/modules/Playerbot/AI/ClassAI/ResourceManager.cpp

Step 3: Analyze Data Access

// Read the locked sections:
std::lock_guard<std::mutex> lock(_resourceMutex);
_currentMana = bot->GetPower(POWER_MANA);  // ← Per-bot data!

Step 4: Determine Necessity

✅ Per-bot instance? YES (from BotAI)
✅ Per-bot data only? YES (_currentMana)
✅ No cross-bot access? YES
Conclusion: Lock UNNECESSARY, remove it

Step 5: Fix

// Remove the lock, add comment
// No lock needed - _currentMana is per-bot instance data
_currentMana = bot->GetPower(POWER_MANA);

Automation Opportunities

Batch Analysis Script

# For each manager, check if it's per-bot instance
for manager in ResourceManager CooldownManager FormationManager ...; do
  echo "=== $manager ==="

  # Check instantiation pattern
  grep -n "std::make_unique<$manager>" src/modules/Playerbot/AI/BotAI.cpp

  # Count mutex locks
  lock_count=$(grep -c "std::lock_guard" \
    src/modules/Playerbot/AI/ClassAI/${manager}.cpp 2>/dev/null || echo 0)

  echo "Lock count: $lock_count"
  echo ""
done

Risk Assessment

Low Risk Removals

Managers with:

✅ Per-bot instance model (std::make_unique per bot)
✅ Only access own bot's member variables
✅ No singleton or static access

Expected: 18-20 managers

High Risk Removals