From 156a23d744e938c801c4975a7af41b0e0849224a Mon Sep 17 00:00:00 2001 From: MatthewBeshay <92357869+MatthewBeshay@users.noreply.github.com> Date: Tue, 31 Mar 2026 00:22:33 +1100 Subject: [PATCH] Remove dead _ENTITIES_RW_SECTION code and update stale critical section comments The RW section code path was never compiled. Updated remaining comments to reflect mutex/lock terminology. --- Minecraft.Client/Rendering/LevelRenderer.cpp | 10 +-- Minecraft.World/Level/LevelChunk.cpp | 85 +------------------ Minecraft.World/Level/LevelChunk.h | 6 -- .../Level/Storage/McRegionChunkStorage.cpp | 7 +- Minecraft.World/Network/Connection.cpp | 3 +- Minecraft.World/Network/Connection.h | 8 +- 6 files changed, 16 insertions(+), 103 deletions(-) diff --git a/Minecraft.Client/Rendering/LevelRenderer.cpp b/Minecraft.Client/Rendering/LevelRenderer.cpp index 540c5cb29..bc52e1de6 100644 --- a/Minecraft.Client/Rendering/LevelRenderer.cpp +++ b/Minecraft.Client/Rendering/LevelRenderer.cpp @@ -1930,11 +1930,11 @@ bool LevelRenderer::updateDirtyChunks() { chunk->clearDirty(); // Take a copy of the details that are required for chunk // rebuilding, and rebuild That instead of the original chunk - // data. This is done within the m_csDirtyChunks critical - // section, which means that any chunks can't be repositioned + // data. This is done within the m_csDirtyChunks lock, + // which means that any chunks can't be repositioned // whilst we are doing this copy. The copy will then be // guaranteed to be consistent whilst rebuilding takes place - // outside of that critical section. + // outside of that lock. permaChunk[index].makeCopyForRebuild(chunk); ++index; } @@ -2024,10 +2024,10 @@ bool LevelRenderer::updateDirtyChunks() { chunk->clearDirty(); // Take a copy of the details that are required for chunk // rebuilding, and rebuild That instead of the original chunk data. - // This is done within the m_csDirtyChunks critical section, which + // This is done within the m_csDirtyChunks lock, which // means that any chunks can't be repositioned whilst we are doing // this copy. The copy will then be guaranteed to be consistent - // whilst rebuilding takes place outside of that critical section. + // whilst rebuilding takes place outside of that lock. permaChunk.makeCopyForRebuild(chunk); dirtyChunksLock.unlock(); } diff --git a/Minecraft.World/Level/LevelChunk.cpp b/Minecraft.World/Level/LevelChunk.cpp index 751a466fb..97fe9d7bb 100644 --- a/Minecraft.World/Level/LevelChunk.cpp +++ b/Minecraft.World/Level/LevelChunk.cpp @@ -24,20 +24,11 @@ #if defined(SHARING_ENABLED) std::recursive_mutex LevelChunk::m_csSharing; #endif -#if defined(_ENTITIES_RW_SECTION) -// AP - use a RW critical section so we can have multiple threads reading the -// same data to avoid a clash -CRITICAL_RW_SECTION LevelChunk::m_csEntities; -#else std::recursive_mutex LevelChunk::m_csEntities; -#endif std::recursive_mutex LevelChunk::m_csTileEntities; bool LevelChunk::touchedSky = false; void LevelChunk::staticCtor() { -#if defined(_ENTITIES_RW_SECTION) - InitializeCriticalRWSection(&m_csEntities); -#endif } void LevelChunk::init(Level* level, int x, int z) { @@ -45,18 +36,10 @@ void LevelChunk::init(Level* level, int x, int z) { for (int i = 0; i < 16 * 16; i++) { biomes[i] = 0xff; } -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif entityBlocks = new std::vector >*[ENTITY_BLOCKS_LENGTH]; -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif terrainPopulated = 0; m_unsaved = false; @@ -76,19 +59,11 @@ void LevelChunk::init(Level* level, int x, int z) { this->z = z; MemSect(1); heightmap = byteArray(16 * 16); -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif for (int i = 0; i < ENTITY_BLOCKS_LENGTH; i++) { entityBlocks[i] = new std::vector >(); } -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif MemSect(0); @@ -1175,17 +1150,9 @@ void LevelChunk::addEntity(std::shared_ptr e) { e->yChunk = yc; e->zChunk = z; -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif entityBlocks[yc]->push_back(e); -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif } void LevelChunk::removeEntity(std::shared_ptr e) { @@ -1196,11 +1163,7 @@ void LevelChunk::removeEntity(std::shared_ptr e, int yc) { if (yc < 0) yc = 0; if (yc >= ENTITY_BLOCKS_LENGTH) yc = ENTITY_BLOCKS_LENGTH - 1; -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif // 4J - was entityBlocks[yc]->remove(e); auto it = find(entityBlocks[yc]->begin(), entityBlocks[yc]->end(), e); @@ -1213,11 +1176,7 @@ void LevelChunk::removeEntity(std::shared_ptr e, int yc) { MemSect(0); } -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif } bool LevelChunk::isSkyLit(int x, int y, int z) { @@ -1409,19 +1368,11 @@ void LevelChunk::load() { } level->addAllPendingTileEntities(values); -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif for (int i = 0; i < ENTITY_BLOCKS_LENGTH; i++) { level->addEntities(entityBlocks[i]); } -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif } else { #if defined(_LARGE_WORLDS) m_bUnloaded = false; @@ -1447,19 +1398,11 @@ void LevelChunk::unload(bool unloadTileEntities) // 4J - added parameter } } -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif for (int i = 0; i < ENTITY_BLOCKS_LENGTH; i++) { level->removeEntities(entityBlocks[i]); } -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif // app.DebugPrintf("Unloaded chunk %d, %d\n", x, z); #if defined(_LARGE_WORLDS) @@ -1520,28 +1463,16 @@ void LevelChunk::unload(bool unloadTileEntities) // 4J - added parameter } bool LevelChunk::containsPlayer() { -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, true); -#else { std::lock_guard lock(m_csEntities); -#endif for (int i = 0; i < ENTITY_BLOCKS_LENGTH; i++) { std::vector >* vecEntity = entityBlocks[i]; for (int j = 0; j < vecEntity->size(); j++) { if (vecEntity->at(j)->GetType() == eTYPE_SERVERPLAYER) { -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else -#endif return true; } } } -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, true); -#else } -#endif return false; } @@ -1559,7 +1490,7 @@ void LevelChunk::getEntities(std::shared_ptr except, AABB* bb, if (yc0 < 0) yc0 = 0; if (yc1 >= ENTITY_BLOCKS_LENGTH) yc1 = ENTITY_BLOCKS_LENGTH - 1; - // AP - RW critical sections are expensive so enter once in + // AP - locking is expensive so enter once in // Level::getEntities { std::lock_guard lock(m_csEntities); for (int yc = yc0; yc <= yc1; yc++) { @@ -1605,7 +1536,7 @@ void LevelChunk::getEntitiesOfClass(const std::type_info& ec, AABB* bb, yc1 = 0; } - // AP - RW critical sections are expensive so enter once in + // AP - locking is expensive so enter once in // Level::getEntitiesOfClass { std::lock_guard lock(m_csEntities); for (int yc = yc0; yc <= yc1; yc++) { @@ -1652,19 +1583,11 @@ void LevelChunk::getEntitiesOfClass(const std::type_info& ec, AABB* bb, int LevelChunk::countEntities() { int entityCount = 0; -#if defined(_ENTITIES_RW_SECTION) - EnterCriticalRWSection(&m_csEntities, false); -#else { std::lock_guard lock(m_csEntities); -#endif for (int yc = 0; yc < ENTITY_BLOCKS_LENGTH; yc++) { entityCount += (int)entityBlocks[yc]->size(); } -#if defined(_ENTITIES_RW_SECTION) - LeaveCriticalRWSection(&m_csEntities, false); -#else } -#endif return entityCount; } @@ -2206,7 +2129,7 @@ void LevelChunk::compressBlocks() { // server again. if (level->isClientSide && g_NetworkManager.IsHost()) { // Note - only the extraction of the pointers needs to be done in the - // critical section, since even if the data is unshared whilst we are + // lock, since even if the data is unshared whilst we are // processing this data is still valid (for the server) { std::lock_guard lock(m_csSharing); if (sharingTilesAndData) { @@ -2305,7 +2228,7 @@ void LevelChunk::compressData() { // server again. if (level->isClientSide && g_NetworkManager.IsHost()) { // Note - only the extraction of the pointers needs to be done in the - // critical section, since even if the data is unshared whilst we are + // lock, since even if the data is unshared whilst we are // processing this data is still valid (for the server) { std::lock_guard lock(m_csSharing); if (sharingTilesAndData) { diff --git a/Minecraft.World/Level/LevelChunk.h b/Minecraft.World/Level/LevelChunk.h index 0dd5df669..7796cb7a3 100644 --- a/Minecraft.World/Level/LevelChunk.h +++ b/Minecraft.World/Level/LevelChunk.h @@ -273,13 +273,7 @@ public: static std::recursive_mutex m_csSharing; // 4J added #endif // 4J added -#if defined(_ENTITIES_RW_SECTION) - static CRITICAL_RW_SECTION - m_csEntities; // AP - we're using a RW critical so we can do multiple - // reads without contention -#else static std::recursive_mutex m_csEntities; -#endif static std::recursive_mutex m_csTileEntities; // 4J added static void staticCtor(); void checkPostProcess(ChunkSource* source, ChunkSource* parent, int x, diff --git a/Minecraft.World/Level/Storage/McRegionChunkStorage.cpp b/Minecraft.World/Level/Storage/McRegionChunkStorage.cpp index c85237704..d880d410a 100644 --- a/Minecraft.World/Level/Storage/McRegionChunkStorage.cpp +++ b/Minecraft.World/Level/Storage/McRegionChunkStorage.cpp @@ -182,11 +182,10 @@ void McRegionChunkStorage::save(Level* level, LevelChunk* levelChunk) { // 4J - removed try/catch // try { - // Note - have added use of a critical section round sections of code that + // Note - have added use of a mutex round sections of code that // do a lot of memory alloc/free operations. This is because when we are - // running saves on multiple threads these sections have a lot of contention - // and thrash the memory system's critical sections Better to let each - // thread have its turn at a higher level of granularity. + // running saves on multiple threads these sections have a lot of contention. + // Better to let each thread have its turn at a higher level of granularity. MemSect(30); PIXBeginNamedEvent(0, "Getting output stream\n"); DataOutputStream* output = RegionFileCache::getChunkDataOutputStream( diff --git a/Minecraft.World/Network/Connection.cpp b/Minecraft.World/Network/Connection.cpp index e90443f25..83651a18e 100644 --- a/Minecraft.World/Network/Connection.cpp +++ b/Minecraft.World/Network/Connection.cpp @@ -38,7 +38,6 @@ void Connection::_init() { tickCount = 0; } -// 4J Jev, need to delete the critical section. Connection::~Connection() { // 4J Stu - Just to be sure, make sure the read and write threads terminate // themselves before the connection object is destroyed @@ -488,7 +487,7 @@ void Connection::tick() { } // 4J - split the following condition (used to be disconnect && - // iscoming.empty()) so we can wrap the access in a critical section + // iscoming.empty()) so we can wrap the access in a mutex if (disconnected) { bool empty; { diff --git a/Minecraft.World/Network/Connection.h b/Minecraft.World/Network/Connection.h index c9b776b9c..4cf68a8b1 100644 --- a/Minecraft.World/Network/Connection.h +++ b/Minecraft.World/Network/Connection.h @@ -56,14 +56,13 @@ private: std::queue > incoming; // 4J - was using synchronizedList... - std::mutex incoming_cs; // ... now has this critical section + std::mutex incoming_cs; // ... now has this mutex std::queue > outgoing; // 4J - was using synchronizedList - but don't think it is - // required as usage is wrapped in writeLock critical section + // required as usage is wrapped in writeLock std::queue > outgoing_slow; // 4J - was using synchronizedList - but don't think it - // is required as usage is wrapped in writeLock critical - // section + // is required as usage is wrapped in writeLock PacketListener* packetListener; bool quitting; @@ -99,7 +98,6 @@ private: std::mutex writeLock; public: - // 4J Jev, need to delete the critical section. ~Connection(); Connection(Socket* socket, const std::wstring& id, PacketListener* packetListener); // throws IOException