From e2ad02f50ac69f3789b7e475dbce469eeec29468 Mon Sep 17 00:00:00 2001 From: "madmaxoft@gmail.com" Date: Sun, 29 Jan 2012 14:29:26 +0000 Subject: [PATCH] ChunkGenerator: rewritten thread-locking using the new RAII CSLock class git-svn-id: http://mc-server.googlecode.com/svn/trunk@186 0a769ca7-a7f5-676a-18bf-c427514a06d6 --- VC2008/MCServer.vcproj | 1 + source/cChunkGenerator.cpp | 146 +++++++++++++++++------------------- source/cChunkGenerator.h | 3 - source/cChunkMap.cpp | 7 +- source/cCriticalSection.cpp | 97 ++++++++++++++++++++++++ source/cCriticalSection.h | 41 +++++++++- 6 files changed, 212 insertions(+), 83 deletions(-) diff --git a/VC2008/MCServer.vcproj b/VC2008/MCServer.vcproj index 0522b7ea5..1dc6129e8 100644 --- a/VC2008/MCServer.vcproj +++ b/VC2008/MCServer.vcproj @@ -20,6 +20,7 @@ OutputDirectory="Debug" IntermediateDirectory="Debug" ConfigurationType="1" + CharacterSet="2" > ChunkCoord; typedef std::list< ChunkCoord > ChunkCoordList; -#define MAX_SEMAPHORES 1000 + + + + +/// If the generation queue size exceeds this number, a warning will be output +#define QUEUE_WARNING_LIMIT 1000 + + + + struct cChunkGenerator::sChunkGeneratorState { - sChunkGeneratorState() - : pCriticalSection( 0 ) - , pSemaphore( 0 ) + cCriticalSection m_CriticalSection; // For protecting the variables in this struct + + ChunkCoordList GenerateQueue; + ChunkCoord CurrentlyGeneratingCoords; + cChunk* pCurrentlyGenerating; + bool bCurrentlyGenerating; + + cSemaphore m_Semaphore; + cThread * pThread; + + bool bStop; + + sChunkGeneratorState(void) + : m_Semaphore(1, 0) , pThread( 0 ) , bStop( false ) , bCurrentlyGenerating( false ) , pCurrentlyGenerating( false ) - , pChunkCriticalSection( 0 ) {} - ChunkCoordList GenerateQueue; // Protected by pCriticalSection - ChunkCoord CurrentlyGeneratingCoords; // Protected by pCriticalSection - cChunk* pCurrentlyGenerating; // Protected by pCriticalSection - bool bCurrentlyGenerating; // Protected by pCriticalSection - - cCriticalSection* pCriticalSection; - cSemaphore* pSemaphore; - cThread* pThread; - - cCriticalSection* pChunkCriticalSection;// Specially for protecting the actual chunk that is currently being generated, and not just the variables in this struct - - bool bStop; }; + + + + cChunkGenerator::cChunkGenerator( cChunkMap* a_pChunkMap ) : m_pState( new sChunkGeneratorState ) , m_pChunkMap( a_pChunkMap ) { - m_pState->pCriticalSection = new cCriticalSection(); - m_pState->pSemaphore = new cSemaphore( MAX_SEMAPHORES, 0 ); - - m_pState->pChunkCriticalSection = new cCriticalSection(); - m_pState->pThread = new cThread( GenerateThread, this, "cChunkGenerator::GenerateThread" ); m_pState->pThread->Start( true ); } + + + + cChunkGenerator::~cChunkGenerator() { m_pState->bStop = true; - m_pState->pSemaphore->Signal(); // Signal so thread can continue and exit + m_pState->m_Semaphore.Signal(); // Signal so thread can continue and exit delete m_pState->pThread; - delete m_pState->pSemaphore; - delete m_pState->pCriticalSection; - delete m_pState->pChunkCriticalSection; - delete m_pState; m_pState = 0; + delete m_pState; } + + + + void cChunkGenerator::GenerateChunk( int a_X, int a_Z ) { - m_pState->pCriticalSection->Lock(); + cCSLock Lock(&m_pState->m_CriticalSection); - if( m_pState->bCurrentlyGenerating ) + if (m_pState->bCurrentlyGenerating) { - if( m_pState->CurrentlyGeneratingCoords.first == a_X && m_pState->CurrentlyGeneratingCoords.second == a_Z ) + if ((m_pState->CurrentlyGeneratingCoords.first == a_X) && (m_pState->CurrentlyGeneratingCoords.second == a_Z)) { - m_pState->pCriticalSection->Unlock(); return; // Already generating this chunk, so ignore } } - int SizeBefore = m_pState->GenerateQueue.size(); - m_pState->GenerateQueue.remove( ChunkCoord(a_X, a_Z) ); - if( m_pState->GenerateQueue.size() >= MAX_SEMAPHORES ) + if (m_pState->GenerateQueue.size() >= QUEUE_WARNING_LIMIT) { - LOGWARN("WARNING: Can't add chunk (%i, %i) to generation queue: Queue is too big! (%i)", a_X, a_Z, m_pState->GenerateQueue.size() ); - m_pState->pCriticalSection->Unlock(); - return; + LOGWARN("WARNING: Adding chunk (%i, %i) to generation queue; Queue is too big! (%i)", a_X, a_Z, m_pState->GenerateQueue.size() ); } m_pState->GenerateQueue.push_back( ChunkCoord(a_X, a_Z) ); - int SizeAfter = m_pState->GenerateQueue.size(); - m_pState->pCriticalSection->Unlock(); - if( SizeBefore < SizeAfter ) - m_pState->pSemaphore->Signal(); + Lock.Unlock(); + + m_pState->m_Semaphore.Signal(); } + + + + void cChunkGenerator::GenerateThread( void* a_Params ) { // Cache some values for easy access (they are all references/pointers) - cChunkGenerator* self = (cChunkGenerator*)a_Params; - sChunkGeneratorState* m_pState = self->m_pState; - ChunkCoordList& GenerateQueue = m_pState->GenerateQueue; - cChunkMap& ChunkMap = *self->m_pChunkMap; - cCriticalSection& CriticalSection = *m_pState->pCriticalSection; - cSemaphore& Semaphore = *m_pState->pSemaphore; + cChunkGenerator * self = (cChunkGenerator*)a_Params; + sChunkGeneratorState * m_pState = self->m_pState; + ChunkCoordList & GenerateQueue = m_pState->GenerateQueue; + cChunkMap & ChunkMap = *self->m_pChunkMap; + cCriticalSection * CriticalSection = &m_pState->m_CriticalSection; + cSemaphore & Semaphore = m_pState->m_Semaphore; - while( !m_pState->bStop ) + while (!m_pState->bStop) { - Semaphore.Wait(); - if( m_pState->bStop ) break; - - CriticalSection.Lock(); - if( GenerateQueue.size() == 0 ) + cCSLock Lock(CriticalSection); + if (GenerateQueue.size() == 0) { - if( !m_pState->bStop ) LOGERROR("ERROR: Semaphore was signaled while GenerateQueue.size == 0"); - CriticalSection.Unlock(); - continue; + cCSUnlock Unlock(Lock); + Semaphore.Wait(); } + if (m_pState->bStop) break; + ChunkCoord coord = *GenerateQueue.begin(); // Get next coord from queue GenerateQueue.erase( GenerateQueue.begin() ); // Remove coordinate from queue m_pState->bCurrentlyGenerating = true; m_pState->CurrentlyGeneratingCoords = coord; - CriticalSection.Unlock(); // Unlock ASAP + Lock.Unlock(); // Unlock ASAP ChunkMap.GetWorld()->LockChunks(); if( ChunkMap.GetChunk( coord.first, 0, coord.second ) ) // Make sure it has not been loaded in the meantime. Don't want to generate the same chunk twice @@ -137,36 +144,23 @@ void cChunkGenerator::GenerateThread( void* a_Params ) LOGINFO("cChunkGenerator generating chunk %i %i", coord.first, coord.second ); cChunk* Chunk = new cChunk( coord.first, 0, coord.second, ChunkMap.GetWorld() ); - CriticalSection.Lock(); + Lock.Lock(); m_pState->pCurrentlyGenerating = Chunk; - CriticalSection.Unlock(); + Lock.Unlock(); - self->Lock(); // Protect the actual chunk Chunk->Initialize(); // Generate the chunk - self->Unlock(); ChunkMap.GetWorld()->LockChunks(); ChunkMap.AddChunk( Chunk ); ChunkMap.GetWorld()->UnlockChunks(); - CriticalSection.Lock(); + Lock.Lock(); m_pState->bCurrentlyGenerating = false; m_pState->pCurrentlyGenerating = 0; - CriticalSection.Unlock(); - } + Lock.Unlock(); + } // while (!bStop) } -cChunk* cChunkGenerator::GetCurrentlyGenerating() -{ - return m_pState->pCurrentlyGenerating; -} -void cChunkGenerator::Lock() -{ - m_pState->pChunkCriticalSection->Lock(); -} -void cChunkGenerator::Unlock() -{ - m_pState->pChunkCriticalSection->Unlock(); -} \ No newline at end of file + diff --git a/source/cChunkGenerator.h b/source/cChunkGenerator.h index df29a3e60..bdba4da98 100644 --- a/source/cChunkGenerator.h +++ b/source/cChunkGenerator.h @@ -10,9 +10,6 @@ public: void GenerateChunk( int a_X, int a_Z ); - cChunk* GetCurrentlyGenerating(); // WARNING - Be sure to Lock() before calling, and Unlock() after done with the chunk! - void Lock(); - void Unlock(); private: static void GenerateThread( void* a_Params ); diff --git a/source/cChunkMap.cpp b/source/cChunkMap.cpp index a6bf6bda0..3cfeb0485 100644 --- a/source/cChunkMap.cpp +++ b/source/cChunkMap.cpp @@ -396,14 +396,15 @@ cChunk* cChunkMap::GetChunk( int a_X, int a_Y, int a_Z ) delete [] Data->m_Compressed; Data->m_Compressed = 0; Data->m_CompressedSize = 0; return Chunk; } - - } } - return 0; } + + + + void cChunkMap::Tick( float a_Dt ) { /* // OLD diff --git a/source/cCriticalSection.cpp b/source/cCriticalSection.cpp index 85f89c195..bf84b7c2f 100644 --- a/source/cCriticalSection.cpp +++ b/source/cCriticalSection.cpp @@ -1,5 +1,6 @@ #include "cCriticalSection.h" #include "cMCLogger.h" +#include #ifdef _WIN32 #include @@ -7,6 +8,13 @@ #include #endif + + + + +/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// cCriticalSection: + cCriticalSection::cCriticalSection() { #ifdef _WIN32 @@ -25,6 +33,10 @@ cCriticalSection::cCriticalSection() #endif } + + + + cCriticalSection::~cCriticalSection() { #ifdef _WIN32 @@ -41,6 +53,10 @@ cCriticalSection::~cCriticalSection() #endif } + + + + void cCriticalSection::Lock() { #ifdef _WIN32 @@ -50,6 +66,10 @@ void cCriticalSection::Lock() #endif } + + + + void cCriticalSection::Unlock() { #ifdef _WIN32 @@ -58,3 +78,80 @@ void cCriticalSection::Unlock() pthread_mutex_unlock( (pthread_mutex_t*)m_CriticalSectionPtr ); #endif } + + + + + +/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// cCSLock + +cCSLock::cCSLock(cCriticalSection * a_CS) : + m_CS(a_CS), + m_IsLocked(false) +{ + Lock(); +} + + + + + +cCSLock::~cCSLock() +{ + Unlock(); +} + + + + + +void cCSLock::Lock(void) +{ + #ifdef _DEBUG + assert(!m_IsLocked); + m_IsLocked = true; + #endif // _DEBUG + + m_CS->Lock(); +} + + + + + +void cCSLock::Unlock(void) +{ + #ifdef _DEBUG + assert(m_IsLocked); + m_IsLocked = false; + #endif // _DEBUG + + m_CS->Unlock(); +} + + + + + +/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +// cCSUnlock: + +cCSUnlock::cCSUnlock(cCSLock & a_Lock) : + m_Lock(a_Lock) +{ + m_Lock.Unlock(); +} + + + + + +cCSUnlock::~cCSUnlock() +{ + m_Lock.Lock(); +} + + + + diff --git a/source/cCriticalSection.h b/source/cCriticalSection.h index fd1d34e46..60602e821 100644 --- a/source/cCriticalSection.h +++ b/source/cCriticalSection.h @@ -13,4 +13,43 @@ private: #ifndef _WIN32 void* m_Attributes; #endif -}; \ No newline at end of file +}; + + + + +/// RAII for cCriticalSection - locks the CS on creation, unlocks on destruction +class cCSLock +{ + cCriticalSection * m_CS; + + #ifdef _DEBUG + // Unlike a cCriticalSection, this object should be used from a single thread, therefore access to m_IsLocked is not threadsafe + bool m_IsLocked; + #endif // _DEBUG + +public: + cCSLock(cCriticalSection * a_CS); + ~cCSLock(); + + // Temporarily unlock or re-lock: + void Lock(void); + void Unlock(void); +} ; + + + + + +/// Temporary RAII unlock for a cCSLock. Useful for unlock-wait-relock scenarios +class cCSUnlock +{ + cCSLock & m_Lock; +public: + cCSUnlock(cCSLock & a_Lock); + ~cCSUnlock(); +} ; + + + +