Improve multithreading of mmaps_generator (#25625)

* Build/Misc: Add a few *San CMake flags

Add the following flags for the related tools:
- MSAN for Memory Sanitizer
- UBSAN for Undefined Behavior Sanitizer
- TSAN for Thread Sanitizer

* Remove unused parameter

* Fix UBSan reported issue

* Disable G3D buffer pools when using Thread Sanitizer as it has its custom locking mechanisms

* Code cleanup

* Move threads from maps to tiles

* Move tile building logic to TileBuilder class

* Fix memory leak in TileBuilder

* Fix build

* Store TileBuilder as raw pointer for now, it will be changed later on to use modern C++ constructs

* Fix crash on shutdown

* Revert pvs-studio change

* Fix generating 1 single tile not closing the program

cmake/compiler/clang/settings.cmake

@@ -74,7 +74,53 @@ if(ASAN)
       -fsanitize-recover=address
       -fsanitize-address-use-after-scope)
 
-  message(STATUS "Clang: Enabled Address Sanitizer")
+  message(STATUS "Clang: Enabled Address Sanitizer ASan")
+endif()
+
+if(MSAN)
+  target_compile_options(trinity-compile-option-interface
+    INTERFACE
+      -fno-omit-frame-pointer
+      -fsanitize=memory
+      -fsanitize-memory-track-origins
+      -mllvm
+      -msan-keep-going=1)
+
+  target_link_options(trinity-compile-option-interface
+    INTERFACE
+      -fno-omit-frame-pointer
+      -fsanitize=memory
+      -fsanitize-memory-track-origins)
+
+  message(STATUS "Clang: Enabled Memory Sanitizer MSan")
+endif()
+
+if(UBSAN)
+  target_compile_options(trinity-compile-option-interface
+    INTERFACE
+      -fno-omit-frame-pointer
+      -fsanitize=undefined)
+
+  target_link_options(trinity-compile-option-interface
+    INTERFACE
+      -fno-omit-frame-pointer
+      -fsanitize=undefined)
+
+  message(STATUS "Clang: Enabled Undefined Behavior Sanitizer UBSan")
+endif()
+
+if(TSAN)
+  target_compile_options(trinity-compile-option-interface
+    INTERFACE
+      -fno-omit-frame-pointer
+      -fsanitize=thread)
+
+  target_link_options(trinity-compile-option-interface
+    INTERFACE
+      -fno-omit-frame-pointer
+      -fsanitize=thread)
+
+  message(STATUS "Clang: Enabled Thread Sanitizer TSan")
 endif()
 
 # -Wno-narrowing needed to suppress a warning in g3d

cmake/showoptions.cmake

@@ -122,6 +122,27 @@ if(ASAN)
   add_definitions(-DASAN)
 endif()
 
+if(MSAN)
+  message("")
+  message(" *** MSAN - WARNING!")
+  message(" *** Please note that this is for DEBUGGING WITH MEMORY SANITIZER only!")
+  add_definitions(-DMSAN)
+endif()
+
+if(UBSAN)
+  message("")
+  message(" *** UBSAN - WARNING!")
+  message(" *** Please note that this is for DEBUGGING WITH UNDEFINED BEHAVIOR SANITIZER only!")
+  add_definitions(-DUBSAN)
+endif()
+
+if(TSAN)
+  message("")
+  message(" *** TSAN - WARNING!")
+  message(" *** Please note that this is for DEBUGGING WITH THREAD SANITIZER only!")
+  add_definitions(-DTSAN -DNO_BUFFERPOOL)
+endif()
+
 if(PERFORMANCE_PROFILING)
   message("")
   message(" *** PERFORMANCE_PROFILING - WARNING!")

src/tools/mmaps_generator/MapBuilder.cpp

@@ -31,15 +31,43 @@
 
 namespace MMAP
 {
+    TileBuilder::TileBuilder(MapBuilder* mapBuilder, bool skipLiquid, bool bigBaseUnit, bool debugOutput) :
+        m_bigBaseUnit(bigBaseUnit),
+        m_debugOutput(debugOutput),
+        m_mapBuilder(mapBuilder),
+        m_terrainBuilder(nullptr),
+        m_workerThread(&TileBuilder::WorkerThread, this),
+        m_rcContext(nullptr)
+    {
+        m_terrainBuilder = new TerrainBuilder(skipLiquid);
+        m_rcContext = new rcContext(false);
+    }
+
+    TileBuilder::~TileBuilder()
+    {
+        WaitCompletion();
+
+        delete m_terrainBuilder;
+        delete m_rcContext;
+    }
+
+    void TileBuilder::WaitCompletion()
+    {
+        if (m_workerThread.joinable())
+            m_workerThread.join();
+    }
+
     MapBuilder::MapBuilder(Optional<float> maxWalkableAngle, Optional<float> maxWalkableAngleNotSteep, bool skipLiquid,
         bool skipContinents, bool skipJunkMaps, bool skipBattlegrounds,
-        bool debugOutput, bool bigBaseUnit, int mapid, char const* offMeshFilePath) :
+        bool debugOutput, bool bigBaseUnit, int mapid, char const* offMeshFilePath, unsigned int threads) :
         m_terrainBuilder     (nullptr),
         m_debugOutput        (debugOutput),
         m_offMeshFilePath    (offMeshFilePath),
+        m_threads            (threads),
         m_skipContinents     (skipContinents),
         m_skipJunkMaps       (skipJunkMaps),
         m_skipBattlegrounds  (skipBattlegrounds),
+        m_skipLiquid         (skipLiquid),
         m_maxWalkableAngle   (maxWalkableAngle),
         m_maxWalkableAngleNotSteep (maxWalkableAngleNotSteep),
         m_bigBaseUnit        (bigBaseUnit),
@@ -53,12 +81,24 @@ namespace MMAP
 
         m_rcContext = new rcContext(false);
 
+        // At least 1 thread is needed
+        m_threads = std::max(1u, m_threads);
+
         discoverTiles();
     }
 
     /**************************************************************************/
     MapBuilder::~MapBuilder()
     {
+        _cancelationToken = true;
+
+        _queue.Cancel();
+
+        for (auto& builder : m_tileBuilders)
+            delete builder;
+
+        m_tileBuilders.clear();
+
         for (TileList::iterator it = m_tiles.begin(); it != m_tiles.end(); ++it)
         {
             (*it).m_tiles->clear();
@@ -172,44 +212,51 @@ namespace MMAP
 
     /**************************************************************************/
 
-    void MapBuilder::WorkerThread()
+    void TileBuilder::WorkerThread()
     {
         while (1)
         {
-            uint32 mapId = 0;
+            TileInfo tileInfo;
 
-            _queue.WaitAndPop(mapId);
+            m_mapBuilder->_queue.WaitAndPop(tileInfo);
 
-            if (_cancelationToken)
+            if (m_mapBuilder->_cancelationToken)
                 return;
 
-            buildMap(mapId);
+            dtNavMesh* navMesh = dtAllocNavMesh();
+            if (!navMesh->init(&tileInfo.m_navMeshParams))
+            {
+                printf("[Map %03i] Failed creating navmesh for tile %i,%i !\n", tileInfo.m_mapId, tileInfo.m_tileX, tileInfo.m_tileY);
+                dtFreeNavMesh(navMesh);
+                return;
+            }
+
+            buildTile(tileInfo.m_mapId, tileInfo.m_tileX, tileInfo.m_tileY, navMesh);
+
+            dtFreeNavMesh(navMesh);
         }
     }
 
-    void MapBuilder::buildAllMaps(unsigned int threads)
+    void MapBuilder::buildMaps(Optional<uint32> mapID)
     {
-        printf("Using %u threads to extract mmaps\n", threads);
+        printf("Using %u threads to generate mmaps\n", m_threads);
 
-        for (unsigned int i = 0; i < threads; ++i)
+        for (unsigned int i = 0; i < m_threads; ++i)
         {
-            _workerThreads.push_back(std::thread(&MapBuilder::WorkerThread, this));
+            m_tileBuilders.push_back(new TileBuilder(this, m_skipLiquid, m_bigBaseUnit, m_debugOutput));
         }
 
-        m_tiles.sort([](MapTiles a, MapTiles b)
+        if (mapID)
         {
-            return a.m_tiles->size() > b.m_tiles->size();
-        });
-
-        for (TileList::iterator it = m_tiles.begin(); it != m_tiles.end(); ++it)
+            buildMap(*mapID);
+        }
+        else
         {
-            uint32 mapId = it->m_mapId;
-            if (!shouldSkipMap(mapId))
+            // Build all maps if no map id has been specified
+            for (TileList::iterator it = m_tiles.begin(); it != m_tiles.end(); ++it)
             {
-                if (threads > 0)
-                    _queue.Push(mapId);
-                else
-                    buildMap(mapId);
+                if (!shouldSkipMap(it->m_mapId))
+                    buildMap(it->m_mapId);
             }
         }
 
@@ -222,10 +269,10 @@ namespace MMAP
 
         _queue.Cancel();
 
-        for (auto& thread : _workerThreads)
-        {
-            thread.join();
-        }
+        for (auto& builder : m_tileBuilders)
+            delete builder;
+
+        m_tileBuilders.clear();
     }
 
     /**************************************************************************/
@@ -354,7 +401,8 @@ namespace MMAP
         getTileBounds(tileX, tileY, data.solidVerts.getCArray(), data.solidVerts.size() / 3, bmin, bmax);
 
         // build navmesh tile
-        buildMoveMapTile(mapId, tileX, tileY, data, bmin, bmax, navMesh);
+        TileBuilder tileBuilder = TileBuilder(this, m_skipLiquid, m_bigBaseUnit, m_debugOutput);
+        tileBuilder.buildMoveMapTile(mapId, tileX, tileY, data, bmin, bmax, navMesh);
         fclose(file);
     }
 
@@ -369,8 +417,15 @@ namespace MMAP
             return;
         }
 
-        buildTile(mapID, tileX, tileY, navMesh);
+        // ToDo: delete the old tile as the user clearly wants to rebuild it
+
+        TileBuilder tileBuilder = TileBuilder(this, m_skipLiquid, m_bigBaseUnit, m_debugOutput);
+        tileBuilder.buildTile(mapID, tileX, tileY, navMesh);
         dtFreeNavMesh(navMesh);
+
+        _cancelationToken = true;
+
+        _queue.Cancel();
     }
 
     /**************************************************************************/
@@ -399,21 +454,28 @@ namespace MMAP
                 // unpack tile coords
                 StaticMapTree::unpackTileID((*it), tileX, tileY);
 
-                if (!shouldSkipTile(mapID, tileX, tileY))
-                    buildTile(mapID, tileX, tileY, navMesh);
-                ++m_totalTilesProcessed;
+                TileInfo tileInfo;
+                tileInfo.m_mapId = mapID;
+                tileInfo.m_tileX = tileX;
+                tileInfo.m_tileY = tileY;
+                memcpy(&tileInfo.m_navMeshParams, navMesh->getParams(), sizeof(dtNavMeshParams));
+                _queue.Push(tileInfo);
             }
 
             dtFreeNavMesh(navMesh);
         }
-
-        printf("[Map %03i] Complete!\n", mapID);
     }
 
     /**************************************************************************/
-    void MapBuilder::buildTile(uint32 mapID, uint32 tileX, uint32 tileY, dtNavMesh* navMesh)
+    void TileBuilder::buildTile(uint32 mapID, uint32 tileX, uint32 tileY, dtNavMesh* navMesh)
     {
-        printf("%u%% [Map %03i] Building tile [%02u,%02u]\n", percentageDone(m_totalTiles, m_totalTilesProcessed), mapID, tileX, tileY);
+        if(shouldSkipTile(mapID, tileX, tileY))
+        {
+            ++m_mapBuilder->m_totalTilesProcessed;
+            return;
+        }
+
+        printf("%u%% [Map %03i] Building tile [%02u,%02u]\n", m_mapBuilder->currentPercentageDone(), mapID, tileX, tileY);
 
         MeshData meshData;
 
@@ -425,7 +487,10 @@ namespace MMAP
 
         // if there is no data, give up now
         if (!meshData.solidVerts.size() && !meshData.liquidVerts.size())
+        {
+            ++m_mapBuilder->m_totalTilesProcessed;
             return;
+        }
 
         // remove unused vertices
         TerrainBuilder::cleanVertices(meshData.solidVerts, meshData.solidTris);
@@ -437,16 +502,21 @@ namespace MMAP
         allVerts.append(meshData.solidVerts);
 
         if (!allVerts.size())
+        {
+            ++m_mapBuilder->m_totalTilesProcessed;
             return;
+        }
 
         // get bounds of current tile
         float bmin[3], bmax[3];
-        getTileBounds(tileX, tileY, allVerts.getCArray(), allVerts.size() / 3, bmin, bmax);
+        m_mapBuilder->getTileBounds(tileX, tileY, allVerts.getCArray(), allVerts.size() / 3, bmin, bmax);
 
-        m_terrainBuilder->loadOffMeshConnections(mapID, tileX, tileY, meshData, m_offMeshFilePath);
+        m_terrainBuilder->loadOffMeshConnections(mapID, tileX, tileY, meshData, m_mapBuilder->m_offMeshFilePath);
 
         // build navmesh tile
         buildMoveMapTile(mapID, tileX, tileY, meshData, bmin, bmax, navMesh);
+
+        ++m_mapBuilder->m_totalTilesProcessed;
     }
 
     /**************************************************************************/
@@ -525,7 +595,7 @@ namespace MMAP
     }
 
     /**************************************************************************/
-    void MapBuilder::buildMoveMapTile(uint32 mapID, uint32 tileX, uint32 tileY,
+    void TileBuilder::buildMoveMapTile(uint32 mapID, uint32 tileX, uint32 tileY,
         MeshData &meshData, float bmin[3], float bmax[3],
         dtNavMesh* navMesh)
     {
@@ -550,7 +620,7 @@ namespace MMAP
         const TileConfig tileConfig = TileConfig(m_bigBaseUnit);
         int TILES_PER_MAP = tileConfig.TILES_PER_MAP;
         float BASE_UNIT_DIM = tileConfig.BASE_UNIT_DIM;
-        rcConfig config = GetMapSpecificConfig(mapID, bmin, bmax, tileConfig);
+        rcConfig config = m_mapBuilder->GetMapSpecificConfig(mapID, bmin, bmax, tileConfig);
 
         // this sets the dimensions of the heightfield - should maybe happen before border padding
         rcCalcGridSize(config.bmin, config.bmax, config.cs, &config.width, &config.height);
@@ -867,7 +937,7 @@ namespace MMAP
     }
 
     /**************************************************************************/
-    void MapBuilder::getTileBounds(uint32 tileX, uint32 tileY, float* verts, int vertCount, float* bmin, float* bmax)
+    void MapBuilder::getTileBounds(uint32 tileX, uint32 tileY, float* verts, int vertCount, float* bmin, float* bmax) const
     {
         // this is for elevation
         if (verts && vertCount)
@@ -886,7 +956,7 @@ namespace MMAP
     }
 
     /**************************************************************************/
-    bool MapBuilder::shouldSkipMap(uint32 mapID)
+    bool MapBuilder::shouldSkipMap(uint32 mapID) const
     {
         if (m_mapid >= 0)
             return static_cast<uint32>(m_mapid) != mapID;
@@ -934,7 +1004,7 @@ namespace MMAP
     }
 
     /**************************************************************************/
-    bool MapBuilder::isTransportMap(uint32 mapID)
+    bool MapBuilder::isTransportMap(uint32 mapID) const
     {
         switch (mapID)
         {
@@ -973,7 +1043,7 @@ namespace MMAP
         }
     }
 
-    bool MapBuilder::isContinentMap(uint32 mapID)
+    bool MapBuilder::isContinentMap(uint32 mapID) const
     {
         switch (mapID)
         {
@@ -988,7 +1058,7 @@ namespace MMAP
     }
 
     /**************************************************************************/
-    bool MapBuilder::shouldSkipTile(uint32 mapID, uint32 tileX, uint32 tileY)
+    bool TileBuilder::shouldSkipTile(uint32 mapID, uint32 tileX, uint32 tileY) const
     {
         char fileName[255];
         sprintf(fileName, "mmaps/%03u%02i%02i.mmtile", mapID, tileY, tileX);
@@ -1011,7 +1081,7 @@ namespace MMAP
         return true;
     }
 
-    rcConfig MapBuilder::GetMapSpecificConfig(uint32 mapID, float bmin[3], float bmax[3], const TileConfig &tileConfig)
+    rcConfig MapBuilder::GetMapSpecificConfig(uint32 mapID, float bmin[3], float bmax[3], const TileConfig &tileConfig) const
     {
         rcConfig config;
         memset(&config, 0, sizeof(rcConfig));
@@ -1060,7 +1130,7 @@ namespace MMAP
     }
 
     /**************************************************************************/
-    uint32 MapBuilder::percentageDone(uint32 totalTiles, uint32 totalTilesBuilt)
+    uint32 MapBuilder::percentageDone(uint32 totalTiles, uint32 totalTilesBuilt) const
     {
         if (totalTiles)
             return totalTilesBuilt * 100 / totalTiles;
@@ -1068,4 +1138,9 @@ namespace MMAP
         return 0;
     }
 
+    uint32 MapBuilder::currentPercentageDone() const
+    {
+        return percentageDone(m_totalTiles, m_totalTilesProcessed);
+    }
+
 }

src/tools/mmaps_generator/MapBuilder.h

@@ -92,65 +92,104 @@ namespace MMAP
         int TILES_PER_MAP;
     };
 
+    struct TileInfo
+    {
+        TileInfo() : m_mapId(uint32(-1)), m_tileX(), m_tileY(), m_navMeshParams() {}
+
+        uint32 m_mapId;
+        uint32 m_tileX;
+        uint32 m_tileY;
+        dtNavMeshParams m_navMeshParams;
+    };
+
+    // ToDo: move this to its own file. For now it will stay here to keep the changes to a minimum, especially in the cpp file
+    class MapBuilder;
+    class TileBuilder
+    {
+        public:
+            TileBuilder(MapBuilder* mapBuilder,
+                bool skipLiquid,
+                bool bigBaseUnit,
+                bool debugOutput);
+
+            TileBuilder(TileBuilder&&) = default;
+            ~TileBuilder();
+
+            void WorkerThread();
+            void WaitCompletion();
+
+            void buildTile(uint32 mapID, uint32 tileX, uint32 tileY, dtNavMesh* navMesh);
+            // move map building
+            void buildMoveMapTile(uint32 mapID,
+                uint32 tileX,
+                uint32 tileY,
+                MeshData& meshData,
+                float bmin[3],
+                float bmax[3],
+                dtNavMesh* navMesh);
+
+            bool shouldSkipTile(uint32 mapID, uint32 tileX, uint32 tileY) const;
+
+        private:
+            bool m_bigBaseUnit;
+            bool m_debugOutput;
+
+            MapBuilder* m_mapBuilder;
+            TerrainBuilder* m_terrainBuilder;
+            std::thread m_workerThread;
+            // build performance - not really used for now
+            rcContext* m_rcContext;
+    };
+
     class MapBuilder
     {
+        friend class TileBuilder;
+
         public:
             MapBuilder(Optional<float> maxWalkableAngle,
                 Optional<float> maxWalkableAngleNotSteep,
-                bool skipLiquid          = false,
-                bool skipContinents      = false,
-                bool skipJunkMaps        = true,
-                bool skipBattlegrounds   = false,
-                bool debugOutput         = false,
-                bool bigBaseUnit         = false,
-                int mapid                = -1,
-                char const* offMeshFilePath = nullptr);
+                bool skipLiquid,
+                bool skipContinents,
+                bool skipJunkMaps,
+                bool skipBattlegrounds,
+                bool debugOutput,
+                bool bigBaseUnit,
+                int mapid,
+                char const* offMeshFilePath,
+                unsigned int threads);
 
             ~MapBuilder();
 
-            // builds all mmap tiles for the specified map id (ignores skip settings)
-            void buildMap(uint32 mapID);
             void buildMeshFromFile(char* name);
 
             // builds an mmap tile for the specified map and its mesh
             void buildSingleTile(uint32 mapID, uint32 tileX, uint32 tileY);
 
             // builds list of maps, then builds all of mmap tiles (based on the skip settings)
-            void buildAllMaps(unsigned int threads);
-
-            void WorkerThread();
+            void buildMaps(Optional<uint32> mapID);
 
         private:
+            // builds all mmap tiles for the specified map id (ignores skip settings)
+            void buildMap(uint32 mapID);
             // detect maps and tiles
             void discoverTiles();
             std::set<uint32>* getTileList(uint32 mapID);
 
             void buildNavMesh(uint32 mapID, dtNavMesh* &navMesh);
 
-            void buildTile(uint32 mapID, uint32 tileX, uint32 tileY, dtNavMesh* navMesh);
-
-            // move map building
-            void buildMoveMapTile(uint32 mapID,
-                uint32 tileX,
-                uint32 tileY,
-                MeshData &meshData,
-                float bmin[3],
-                float bmax[3],
-                dtNavMesh* navMesh);
-
             void getTileBounds(uint32 tileX, uint32 tileY,
                 float* verts, int vertCount,
-                float* bmin, float* bmax);
+                float* bmin, float* bmax) const;
             void getGridBounds(uint32 mapID, uint32 &minX, uint32 &minY, uint32 &maxX, uint32 &maxY) const;
 
-            bool shouldSkipMap(uint32 mapID);
-            bool isTransportMap(uint32 mapID);
-            bool isContinentMap(uint32 mapID);
-            bool shouldSkipTile(uint32 mapID, uint32 tileX, uint32 tileY);
+            bool shouldSkipMap(uint32 mapID) const;
+            bool isTransportMap(uint32 mapID) const;
+            bool isContinentMap(uint32 mapID) const;
 
-            rcConfig GetMapSpecificConfig(uint32 mapID, float bmin[3], float bmax[3], const TileConfig &tileConfig);
+            rcConfig GetMapSpecificConfig(uint32 mapID, float bmin[3], float bmax[3], const TileConfig &tileConfig) const;
 
-            uint32 percentageDone(uint32 totalTiles, uint32 totalTilesDone);
+            uint32 percentageDone(uint32 totalTiles, uint32 totalTilesDone) const;
+            uint32 currentPercentageDone() const;
 
             TerrainBuilder* m_terrainBuilder;
             TileList m_tiles;
@@ -158,9 +197,11 @@ namespace MMAP
             bool m_debugOutput;
 
             char const* m_offMeshFilePath;
+            unsigned int m_threads;
             bool m_skipContinents;
             bool m_skipJunkMaps;
             bool m_skipBattlegrounds;
+            bool m_skipLiquid;
 
             Optional<float> m_maxWalkableAngle;
             Optional<float> m_maxWalkableAngleNotSteep;
@@ -174,8 +215,8 @@ namespace MMAP
             // build performance - not really used for now
             rcContext* m_rcContext;
 
-            std::vector<std::thread> _workerThreads;
-            ProducerConsumerQueue<uint32> _queue;
+            std::vector<TileBuilder*> m_tileBuilders;
+            ProducerConsumerQueue<TileInfo> _queue;
             std::atomic<bool> _cancelationToken;
     };
 }

src/tools/mmaps_generator/PathGenerator.cpp

@@ -327,7 +327,7 @@ int main(int argc, char** argv)
         return silent ? -5 : finish("Failed to load LiquidType.dbc", -5);
 
     MapBuilder builder(maxAngle, maxAngleNotSteep, skipLiquid, skipContinents, skipJunkMaps,
-                       skipBattlegrounds, debugOutput, bigBaseUnit, mapnum, offMeshInputPath);
+                       skipBattlegrounds, debugOutput, bigBaseUnit, mapnum, offMeshInputPath, threads);
 
     uint32 start = getMSTime();
     if (file)
@@ -335,9 +335,9 @@ int main(int argc, char** argv)
     else if (tileX > -1 && tileY > -1 && mapnum >= 0)
         builder.buildSingleTile(mapnum, tileX, tileY);
     else if (mapnum >= 0)
-        builder.buildMap(uint32(mapnum));
+        builder.buildMaps(uint32(mapnum));
     else
-        builder.buildAllMaps(threads);
+        builder.buildMaps({});
 
     if (!silent)
         printf("Finished. MMAPS were built in %s\n", secsToTimeString(GetMSTimeDiffToNow(start) / 1000).c_str());