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

@@ -20,6 +20,7 @@
 			OutputDirectory="Debug"
 			IntermediateDirectory="Debug"
 			ConfigurationType="1"
+			CharacterSet="2"
 			>
 			<Tool
 				Name="VCPreBuildEventTool"

source/cChunkGenerator.cpp

@@ -15,116 +15,123 @@
 typedef std::pair<int, int> 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();
-}
+

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 );
 

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

source/cCriticalSection.cpp

@@ -1,5 +1,6 @@
 #include "cCriticalSection.h"
 #include "cMCLogger.h"
+#include <assert.h>
 
 #ifdef _WIN32
 #include <Windows.h>
@@ -7,6 +8,13 @@
 #include <pthread.h>
 #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();
+}
+
+
+
+

source/cCriticalSection.h

@@ -13,4 +13,43 @@ private:
 #ifndef _WIN32
 	void* m_Attributes;
 #endif
-};
+};
+
+
+
+
+/// 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();
+} ;
+
+
+
+