fix(spatial): Fix data race and performance issues in SpatialGridManager
PROBLEM: Extreme lag with 100+ bots caused by SpatialGridManager issues: 1. DATA RACE BUG in GetGrid(): - Writing lastAccessTime under shared_lock using const_cast - Multiple threads writing same memory location concurrently = undefined behavior - Could cause crashes, memory corruption, or silent data corruption 2. PERFORMANCE ISSUE in GetGrid(): - Called thousands of times per second (100+ bots × multiple calls per update) - Each call: acquire shared_lock + chrono::steady_clock::now() - Unnecessary overhead for a keep-alive timestamp that's rarely checked 3. Same issues in UpdateGrid() and TouchGrid() SOLUTION: 1. GetGrid(): Remove lastAccessTime update entirely - This method is for READ-ONLY grid access - lastAccessTime is only used for cleanup of inactive grids - No need to update on every access 2. UpdateGrid(): Split into 3 phases - Phase 1: Get grid pointer under shared_lock (fast) - Phase 2: Update grid WITHOUT holding manager lock - Phase 3: Update lastAccessTime under exclusive lock 3. TouchGrid(): Use unique_lock (exclusive) for write operation - This method explicitly updates lastAccessTime - Must use exclusive lock for write operations IMPACT: - Eliminates ~1000+ chrono::now() calls per second - Fixes potential memory corruption from data race - Reduces lock contention on SpatialGridManager Co-Authored-By: Claude Opus 4.5 <[email protected]> Signed-off-by: luis <[email protected]>
This commit is contained in:
committed by
luis
co-authored by
Claude Opus 4.5
parent
5e9d455b5c
commit
22e6d29ddc
@@ -86,8 +86,17 @@ DoubleBufferedSpatialGrid* SpatialGridManager::GetGrid(uint32 mapId)
|
||||
if (it == _grids.end())
|
||||
return nullptr;
|
||||
|
||||
// Update access time (const_cast needed for shared lock access)
|
||||
const_cast<GridInfo&>(it->second).lastAccessTime = ::std::chrono::steady_clock::now();
|
||||
// CRITICAL FIX: Removed lastAccessTime update from GetGrid()
|
||||
// PROBLEM: This was a DATA RACE - writing under shared_lock is unsafe!
|
||||
// Also caused severe performance issues with 100+ bots calling GetGrid
|
||||
// thousands of times per second (each call: acquire lock + chrono::now())
|
||||
//
|
||||
// SOLUTION: lastAccessTime is now only updated when:
|
||||
// - CreateGrid() is called (exclusive lock, grid creation)
|
||||
// - UpdateGrid() is called (from spatial grid update cycle)
|
||||
// - TouchGrid() is explicitly called (rare, for manual keep-alive)
|
||||
//
|
||||
// This eliminates ~1000+ chrono::now() calls per second and fixes the data race.
|
||||
|
||||
return it->second.grid.get();
|
||||
}
|
||||
@@ -124,16 +133,30 @@ void SpatialGridManager::DestroyAllGrids()
|
||||
|
||||
void SpatialGridManager::UpdateGrid(uint32 mapId)
|
||||
{
|
||||
::std::shared_lock lock(_mutex); // Shared read lock
|
||||
DoubleBufferedSpatialGrid* grid = nullptr;
|
||||
|
||||
auto it = _grids.find(mapId);
|
||||
if (it == _grids.end())
|
||||
return; // No grid for this map
|
||||
// Phase 1: Get the grid pointer under shared lock (fast path)
|
||||
{
|
||||
::std::shared_lock lock(_mutex);
|
||||
auto it = _grids.find(mapId);
|
||||
if (it == _grids.end())
|
||||
return; // No grid for this map
|
||||
grid = it->second.grid.get();
|
||||
}
|
||||
|
||||
it->second.grid->Update();
|
||||
// Phase 2: Update the grid WITHOUT holding the manager lock
|
||||
// Grid has its own internal locking (try_to_lock pattern)
|
||||
if (grid)
|
||||
grid->Update();
|
||||
|
||||
// Update access time
|
||||
const_cast<GridInfo&>(it->second).lastAccessTime = ::std::chrono::steady_clock::now();
|
||||
// Phase 3: Update access time under exclusive lock (write operation)
|
||||
// This is called infrequently (once per update cycle, not per bot)
|
||||
{
|
||||
::std::unique_lock lock(_mutex);
|
||||
auto it = _grids.find(mapId);
|
||||
if (it != _grids.end())
|
||||
it->second.lastAccessTime = ::std::chrono::steady_clock::now();
|
||||
}
|
||||
}
|
||||
|
||||
void SpatialGridManager::UpdateGrid(Map* map)
|
||||
@@ -298,13 +321,14 @@ void SpatialGridManager::LogMemoryStats() const
|
||||
|
||||
void SpatialGridManager::TouchGrid(uint32 mapId)
|
||||
{
|
||||
::std::shared_lock lock(_mutex); // Shared read lock
|
||||
// CRITICAL FIX: Use exclusive lock for write operation
|
||||
// Previously used shared_lock with const_cast which was a data race
|
||||
::std::unique_lock lock(_mutex);
|
||||
|
||||
auto it = _grids.find(mapId);
|
||||
if (it != _grids.end())
|
||||
{
|
||||
// Update access time (const_cast needed for shared lock access)
|
||||
const_cast<GridInfo&>(it->second).lastAccessTime = ::std::chrono::steady_clock::now();
|
||||
it->second.lastAccessTime = ::std::chrono::steady_clock::now();
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user